<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=us-ascii"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1>-----Original Message-----<o:p></o:p>From: libvir-list-bounces@redhat.com [mailto:libvir-list-bounces@redhat.com] On Behalf Of Laine Stump<o:p></o:p>Sent: Monday, July 23, 2012 12:43 PM<o:p></o:p>To: libvir-list@redhat.com<o:p></o:p>Subject: Re: [libvirt] [PATCH 1/2] ESX: Add routines to interface driver<o:p></o:p><o:p> </o:p>On 07/20/2012 05:20 PM, Ata E Husain Bohra wrote:<o:p></o:p>> Add following routines to esx_interface_driver:<o:p></o:p>> esxNumOfInterfaces,<o:p></o:p>> esxNumOfDefinedInterfaces,<o:p></o:p>> esxListInterfaces,<o:p></o:p>> esxListDefinedInterfaces,<o:p></o:p>> esxInterfaceLookupByMACString,<o:p></o:p>> esxInterfaceGetXMLDesc,<o:p></o:p>> esxInterfaceUndefine,<o:p></o:p>> esxInterfaceCreate,<o:p></o:p>> esxInterfaceDestroy<o:p></o:p>><o:p> </o:p>> Signed-off-by: Ata E Husain Bohra <<a href="mailto:ata.husain@hotmail.com">ata.husain@hotmail.com</a>><o:p></o:p>> ---<o:p></o:p>> src/esx/esx_interface_driver.c | 506 +++++++++++++++++++++++++++++++++++++++-<o:p></o:p>> src/esx/esx_vi.c | 126 ++++++++++<o:p></o:p>> src/esx/esx_vi.h | 10 +<o:p></o:p>> src/esx/esx_vi_generator.input | 227 ++++++++++++++++++<o:p></o:p>> src/esx/esx_vi_generator.py | 31 ++-<o:p></o:p>> src/esx/esx_vi_types.c | 18 +-<o:p></o:p>> 6 files changed, 913 insertions(+), 5 deletions(-)<o:p></o:p>><o:p> </o:p>> diff --git a/src/esx/esx_interface_driver.c <o:p></o:p>> b/src/esx/esx_interface_driver.c index 5713137..b1ba5e2 100644<o:p></o:p>> --- a/src/esx/esx_interface_driver.c<o:p></o:p>> +++ b/src/esx/esx_interface_driver.c<o:p></o:p>> @@ -23,6 +23,9 @@<o:p></o:p>> */<o:p></o:p>> +static int<o:p></o:p>> +esxNumOfInterfaces(virConnectPtr conn) {<o:p></o:p>> + esxPrivate *priv = conn->interfacePrivateData;<o:p></o:p>> + esxVI_HostVirtualNic *virtualNicList = NULL;<o:p></o:p>> + const esxVI_HostVirtualNic *virtualNic = NULL;<o:p></o:p><o:p> </o:p>> It's a bit disconcerting to see these called "virtual" NICs, when <o:p></o:p>> virInterface is all about configuring interfaces on the *physical* host.<o:p></o:p><o:p> </o:p>>This i my biggest concern here too, and also that's the reason why I didn't implement the network driver yet. Because I'm not sure what is the proper mapping between libvirt and vSphere API here.<o:p></o:p>>As far as I understand this, a HostVirtualNic seems to be the best match for a virInterface. A HostVirtualNic is connected to HostVirtualSwitch, that is connected to the physical network via a PhysicalNic. >There is not much one can do about the PhysicalNic. It just sits there and is connected to the HostVirtualSwitch. The HostVirtualNic is also the one that has the external IP address of the ESX server >assigned.<o:p></o:p>>Also a HostVirtualNic is not part of the virtual hardware of a VM. A VirtualEthernetCard is the type in the vSphere API representing the virtual network interface of a virtual machine<o:p></o:p><o:p> </o:p>[AB]: I have developed the driver based on the same understanding; the only thing that I could not map vSphere API <--> Libvirt API was how to allow user application to configure end-end networking on ESX server that includes: creating vSwitches, attaching them to physical network and then configuring HostVirtual Nics. I am still working on the "interfaceCreateXML" that would allow addition of HostVirutalNic and associated portGroup if not present, but could not find any mapping API to add vSwitches. <o:p></o:p><o:p> </o:p>> Being unfamiliar with the vmware API, I'm not sure which of these <o:p></o:p>> functions are things you've added to libvirt and which are part of <o:p></o:p>> vmware, but please rename any that are defined in libvirt to use <o:p></o:p>> "physical" or "host" or "p/h" as appropriate, rather than "virtual"<o:p></o:p>> and/or "v". We want to make sure nobody gets the wrong idea about <o:p></o:p>> these functions.<o:p></o:p><o:p> </o:p>>I don't think that this is a good idea, we should really stick exactly to the naming of things in the vSphere API otherwise there will be much more confusion.<o:p></o:p>>Therefore,<o:p></o:p>> esxVI_HostVirtualNic *virtualNicList = NULL;<o:p></o:p>>should be<o:p></o:p>> esxVI_HostVirtualNic *hostVirtualNicList = NULL;<o:p></o:p>> Apart from that I'll do a more detailed review soon. <o:p></o:p><o:p> </o:p>[AB]: Will modify the names such that HostVirtualNic should read same in vSphere API documentation.<o:p></o:p><o:p> </o:p>> +static int<o:p></o:p>> +esxNumOfDefinedInterfaces(virConnectPtr conn) {<o:p></o:p>> + conn->interfacePrivateData = NULL;<o:p></o:p><o:p> </o:p>>Setting interfacePrivateData to NULL is equivalent to closing the driver. I don't think you really want to do this. Possibly it was an error in cut/paste from somewhere else (esxInterfaceClose()?).<o:p></o:p>[AB]: Patch 2/2 rectified this issue.<o:p></o:p><o:p> </o:p>> +<o:p></o:p>> + // ESX interfaces are always active<o:p></o:p>> + return 0;<o:p></o:p>> +}<o:p></o:p>> +<o:p></o:p>> +<o:p></o:p>> +<o:p></o:p>> +static int<o:p></o:p>> +esxListDefinedInterfaces(virConnectPtr conn, char **names, int <o:p></o:p>> +maxnames) {<o:p></o:p>> + conn->interfacePrivateData = conn->privateData;<o:p></o:p>> + *names = NULL;<o:p></o:p>> +<o:p></o:p>> + /* keeps compiler happy */<o:p></o:p>> + VIR_DEBUG("max interfaces: %d", maxnames);<o:p></o:p>> +<o:p></o:p>> + // ESX interfaces are always active<o:p></o:p>> + return 0;<o:p></o:p>> +}<o:p></o:p><o:p> </o:p><o:p> </o:p>>As I'll be mentioning in PATCH 2/2, just do this right the first time, rather than having a followup patch to fix code you've just added.<o:p></o:p>[AB]: I will be more careful in the future to avoid such mistakes. Thanks for the advice.<o:p></o:p><o:p> </o:p>> +<o:p></o:p>> +static int<o:p></o:p>> +esxInterfaceCreate(virInterfacePtr iface, unsigned int flags) {<o:p></o:p>> + iface->conn->interfacePrivateData = NULL;<o:p></o:p><o:p> </o:p>>Again - you don't want to NULL this out - it's the same as closing the driver.<o:p></o:p>[AB]: Will remove it.<o:p></o:p><o:p> </o:p>> +<o:p></o:p>> + virCheckFlags(0, -1);<o:p></o:p>> +<o:p></o:p>> + /* ESX interfaces are always active */<o:p></o:p>> + return 0;<o:p></o:p>> +}<o:p></o:p>> +<o:p></o:p>> +<o:p></o:p>> +<o:p></o:p>> +static int<o:p></o:p>> +esxInterfaceDestroy(virInterfacePtr iface, unsigned int flags) {<o:p></o:p>> + iface->conn->interfacePrivateData = NULL;<o:p></o:p><o:p> </o:p>>And again - don't NULL this.<o:p></o:p>[AB]: Ditto.<o:p></o:p><o:p> </o:p>> * A required property must be != 0 (NULL for pointers, "undefined" <o:p></o:p>> == 0 for<o:p></o:p><o:p> </o:p>>A general comment - I didn't see any kind of mutexes in here. Is this code really threadsafe? (it looks like the toplevel functions just call into your utility functions to get a snapshot of the current state of >interfaces in vmware, then work with that snapshot during the rest of the call; I didn't look into the utility functions in detail, because the chance of me understanding them is fairly low anyway :-)<o:p></o:p><o:p> </o:p>[AB]: I do not think mutex is needed to protect the routines, as ESX server can be operated from multiple vSphere Client interface (management console using vCenter or standalone) and best I know it allows the operation simultaneously. As said this is something I would also like to be discussed in the community, so far reading ESX storage driver where the workflow and driver model remains the same, I do not see any use of mutex to protect such operations. The only places mutexes are used are while using CURL sessions.<o:p></o:p><o:p> </o:p>--<o:p></o:p>libvir-list mailing list<o:p></o:p><a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a><o:p></o:p><a href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a><o:p></o:p></div></body></html>