On Wed, Apr 13, 2011 at 2:51 AM, Matthias Bolte < [email protected]> 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 <[email protected]>: > > 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. > > > --- > > 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
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
