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?

I've updated the code for the ESX driver to be able to handle the following functions : - virNetworkLookupByName - virConnectNumOfDefinedNetworks - virConnectNumOfNetworks - virConnectListNetworks

I basically mapped the VMware Managed Object Reference for networks into a few data structures and added the following functions to the internal driver

API: - esxNumOfNetworks - esxListNetworks - esxNumOfDefinedNetworks

- 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

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(a)googlemail.com&gt; wrote: > > Sorry, I forgot to reply to your follow up question on the users list. > > 2011/4/6 James Barkley <james.barkley(a)gmail.com&gt;: > > 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