[libvirt] [PATCH] - added mapping to Network object - added implementation of select networking functions - virsh net-list and net-info commands now work for esx

[James Barkley]

On Wed, Apr 13, 2011 at 2:51 AM, Matthias Bolte <
matthias.bolte at googlemail.com> 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 at gmail.com>:
> > From: jbarkely <jbarkley at 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.


>
> > ---
> >  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(-)
>
> As I'm a bit short of time right now just some general comments.
>
> - 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110413/afd6324d/attachment-0001.htm>