Matthias,Thanks for the terrific feedback. In answer to your questions:
>Is this sufficient to make virsh net-list work?
Yes, virsh net-list and also virsh net-info. I also checked both functions against freshly compiled ruby bindings. I also checked the sufficient error is given in virsh net-list when an invalid network is given.
>You mean the managed object called Network? They _seem_ to be the
>natural fit. But I'm not sure if that's the correct approach, as you
>cannot directly create/destroy such objects and they are bound in some
>way to the port groups on a virtual switch. Also I'm not sure about
>he exact semantics of networks and port groups.
Yes, I used the managed object called Network. It does get a little dicey with VMware due to the addition of port groups and virtual switches (admittedly, not sure how this works in most other hypervisors). You certainly can't have transient network objects, and any creation/destruction of a network object seems like it must necessarily involve either creation/destruction of a port group or creation/destruction of a vswitch. I believe using the Network managed object is the right approach, but am not yet 100% confident. My next task is to facilitate the creation of a new network through virNetworkDefineXML or virNetworkCreateXML. If you'd prefer, I can wait until I've completed this functionality to submit the patch, as it may drive me to adopt a different approach.
>I assume you made esxNumOfDefinedNetworks return 0 to get virsh
>net-list working as there network in the VMware context are always
>active.
Correct. I was debating on whether or not to have it loop through the network list and check the "active/inactive" just for thoroughness, but if you're fine with just returning 0 and commenting why, then I'll leave it.
>The recommended way for sending patches is git send-email.
Barring any other recommendations I'll figure out git send email and submit the patch to the list shortly.
thanks,
-jb
On Wed, Apr 6, 2011 at 2:37 AM, Matthias Bolte
<matthias.bolte@googlemail.com> wrote:
Sorry, I forgot to reply to your follow up question on the users list.
2011/4/6 James Barkley <james.barkley@gmail.com>:
> Greetings:
> I've added code to the ESX driver to support some basic network
> functionality. I'm pretty new to this list, so please tell me how to proceed
> with code review and patch submission (yes I've read the contributor
> guidelines on the wiki). It seems like people are emailing a patch file for
> every file they've changed, each in separate emails with [1 of N] in the
> subject, is that right? Or is it better to paste in code, get some feedback,
> and eventually attach the patch to the bug tracker item?
The normal approach is to have one commit/patch per logical
self-contained change. After each commit/patch the codebase has to be
in a compilable stage. For example you cannot add the code using the
generated SOAP bindings, before you actually added them to the
esx_vi_generator.input file.
The [1 of N] style patch series are typically used for large changes
that are split in several logical, self-contained parts. Splitting
like this simplifies reviewing and later on figuring out bugs using
git bisect.
You typically don't split on a per file basis.
In your case I'd suggest to create a single commit/patch for your
addition, as it is one logical piece of work. You _could_ (but I don't
recommend to) split it in multiple patches. For example one for the
esx_vi_generator.input addition, one for the VI helper function
additions and one for the actual driver functions, but I'd consider
this to be too fine grained.
> I've updated the code for the ESX driver to be able to handle the following
> functions :
> - virNetworkLookupByName
> - virConnectNumOfDefinedNetworks
> - virConnectNumOfNetworks
> - virConnectListNetworks
Is this sufficient to make virsh net-list work?
> I basically mapped the VMware Managed Object Reference for networks into a
> few data structures and added the following functions to the internal driver
You mean the managed object called Network? They _seem_ to be the
natural fit. But I'm not sure if that's the correct approach, as you
cannot directly create/destroy such objects and they are bound in some
way to the port groups on a virtual switch. Also I'm not sure about
the exact semantics of networks and port groups.
That's what I meant as I said about the mapping between libvirt and
VMware. We need to be sure to use the right approach from the start to
avoid making breaking changes across releases later on.
> API:
> - esxNumOfNetworks
> - esxListNetworks
> - esxNumOfDefinedNetworks
I assume you made esxNumOfDefinedNetworks return 0 to get virsh
net-list working as there network in the VMware context are always
active.
> - esxNetworkLookupByName
> - esxVI_LookupNetworkList
> - esxVI_LookupNetworkByName
> These functions were modeled after existing functions from the domain and
> storage libraries. The following files were touched:
> - esx_network_driver.c
> - esx_vi.c
> - esx_vi.h
> - esx_vi_generator.input
Apart from the question whether the Network managed object is the
correct match, the next step is to send your patch to this list to get
a review. The recommended way for sending patches is git send-email.
Matthias