<br><br><div class="gmail_quote">On Wed, Apr 13, 2011 at 2:51 AM, Matthias Bolte <span dir="ltr"><<a href="mailto:matthias.bolte@googlemail.com">matthias.bolte@googlemail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Long summary line. You could reformat it like this:<br>
<br>
<br>
esx: Support virsh net-list and net-info<br>
<br>
Add mapping of Network object and implementation of select<br>
networking functions.<br>
<br>
This makes virsh net-list and net-info commands work for ESX.<br>
<br></blockquote><div><br></div><div>Right, can do. I didn't realize it was just taking the commit message and stuffing it into the subject line. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
2011/4/12 jbarkley <<a href="mailto:james.barkley@gmail.com">james.barkley@gmail.com</a>>:<br>
<div class="im">> From: jbarkely <jbarkley@willo.(none)><br>
<br>
</div>You should tell git your name and email address to get it properly<br>
recorded in a patch. See git config for this:<br>
<br>
git config --global <a href="http://user.name" target="_blank">user.name</a> "your name"<br>
git config --global user.email "your email"<br></blockquote><div><br></div><div>whoops - thought I had done that.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
> ---<br>
> src/esx/esx_network_driver.c | 181 +++++++++++++++++++++++++++++++++++++++-<br>
> src/esx/esx_vi.c | 104 ++++++++++++++++++++++-<br>
> src/esx/esx_vi.h | 9 ++<br>
> src/esx/esx_vi_generator.input | 78 +++++++++++++++++<br>
> 4 files changed, 366 insertions(+), 6 deletions(-)<br>
<br>
</div>As I'm a bit short of time right now just some general comments.<br>
<br>
- C style comments (/* */) are preferred over C++ style ones (//).<br></blockquote><div><br></div><div>Noted</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- Remove code that you have commented out.<br></blockquote><div><br></div><div>Got it</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- I'm not sure if using the MD5 sum of the name of a network it's UUID</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">is the stablest possible source. I'd like to find a better option for<br>
this, but I'm not sure that it exists.<br></blockquote><div>Agreed - this was a hack placeholder to get *something* in place for the UUID. But a hash of the network name won't be UU, but just U since different hosts can have networks with the same name (and in fact do by default with ESX - the "VM Network"). Not sure if there is a better alternative, but I'll keep looking.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
- Instead of adding many empty objects for the members of<br>
HostNetworkSystem and HostNetworkInfo that aren't used yet you could<br>
mark them as ignored (i) instead in the .input file.<br></blockquote><div><br></div><div>Cool - didn't realize I could do this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Overall the patch looks okay. I'll give it a detailed review and test<br>
it over the weekend.<br>
<br>
In the meantime I'd like to delay the inclusion of this patch into the<br>
codebase until we are confident that this approach is the correct one.<br>
<font color="#888888"><br></font></blockquote><div><br></div><div>Sure - whenever you're comfortable. I needed the functionality for some software we're building so we're using the added functionality now. I just thought it'd be nice to give you a chance to add it back in. Let me know how your in-depth review turns out - in the meantime I may clean up the other things you mentioned (comments, ignore marks, etc.). I doubt I'll get to the functionality for adding a network for another week or even two, but I'll let you know how that goes.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><font color="#888888">
Matthias<br></font></blockquote><div><br></div><div>Thanks for taking the time </div><div><br></div><div>-jb </div></div><br>