On Wed, Apr 13, 2011 at 2:51 AM, Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx> wrote:
Long summary line. You could reformat it like this:
esx: Support virsh net-list and net-info
Add mapping of Network object and implementation of select
networking functions.
This makes virsh net-list and net-info commands work for ESX.
Right, can do. I didn't realize it was just taking the commit message and stuffing it into the subject line.
2011/4/12 jbarkley <james.barkley@xxxxxxxxx>:
> From: jbarkely <jbarkley@willo.(none)>You should tell git your name and email address to get it properly
recorded in a patch. See git config for this:
git config --global user.name "your name"
git config --global user.email "your email"
whoops - thought I had done that.
As I'm a bit short of time right now just some general comments.
> ---
> src/esx/esx_network_driver.c | 181 +++++++++++++++++++++++++++++++++++++++-
> src/esx/esx_vi.c | 104 ++++++++++++++++++++++-
> src/esx/esx_vi.h | 9 ++
> src/esx/esx_vi_generator.input | 78 +++++++++++++++++
> 4 files changed, 366 insertions(+), 6 deletions(-)
- C style comments (/* */) are preferred over C++ style ones (//).
Noted
- Remove code that you have commented out.
Got it
- I'm not sure if using the MD5 sum of the name of a network it's UUID
is the stablest possible source. I'd like to find a better option for
this, but I'm not sure that it exists.
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.
- Instead of adding many empty objects for the members of
HostNetworkSystem and HostNetworkInfo that aren't used yet you could
mark them as ignored (i) instead in the .input file.
Cool - didn't realize I could do this.
Overall the patch looks okay. I'll give it a detailed review and test
it over the weekend.
In the meantime I'd like to delay the inclusion of this patch into the
codebase until we are confident that this approach is the correct one.
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.
Matthias
Thanks for taking the time
-jb
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list