[libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
Hi All, As discussed on the list resending the networking patch's. the patch's are as below: [PATCH 1/3]: contains support for "Internal" network in libvirt [PATCH 2/3]: contains support for "Host only" and "Internal" networks in VirtualBox driver [PATCH 3/3]: contains networking API for hostonly networks in VirtualBox driver in libvirt (it contains all the fix's proposed on list along with two extra *DefinedNetworks functions) The patches work as per cvs checkin today. (git SHA1 ID: 9136ae2056b45ea83854d3fe31d860f645b8c883) Regards, Pritesh
On Wed, May 06, 2009 at 06:12:21PM +0200, Pritesh Kothari wrote:
+ +static virNetworkPtr vboxNetworkCreateXML(virConnectPtr conn, const char *xml) { + vboxGlobalData *data = conn->privateData; + virNetworkDefPtr def = NULL; + virNetworkPtr ret = NULL; + nsID *iid = NULL; + char *networkNameUtf8 = NULL; + int i = 0; + + if ((def = virNetworkDefParseString(conn, xml)) == NULL) + goto cleanup; + + if (VIR_ALLOC(iid) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (VIR_ALLOC_N(networkNameUtf8, sizeof("HostInterfaceNetworking-") + + sizeof(def->name) + 1) < 0) {
Fairly sure that should be 'strlen()' there.
+ virReportOOMError(conn); + goto cleanup; + } + + strcpy (networkNameUtf8, "HostInterfaceNetworking-"); + strcat (networkNameUtf8, def->name);
That said, how about just using virAsprintf(&nmetworkNameUtf8.. instead of alloc+strcpy
+ + nsIDFromChar(iid, def->uuid); + + DEBUG("Network Name: %s", def->name); + DEBUG("Network UUID: " + "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}", + (unsigned)iid->m0, (unsigned)iid->m1, + (unsigned)iid->m2, (unsigned)iid->m3[0], + (unsigned)iid->m3[1], (unsigned)iid->m3[2], + (unsigned)iid->m3[3], (unsigned)iid->m3[4], + (unsigned)iid->m3[5], (unsigned)iid->m3[6], + (unsigned)iid->m3[7]); + DEBUG("bridge : %s", def->bridge); + DEBUG("domain : %s", def->domain); + DEBUG("forwardType : %d", def->forwardType); + DEBUG("forwardDev : %s", def->forwardDev); + DEBUG("ipAddress : %s", def->ipAddress); + DEBUG("netmask : %s", def->netmask); + DEBUG("network : %s", def->network); + for (i = 0; i < def->nranges; i++) { + DEBUG("DHCP(%d) start: %s", i, def->ranges[i].start); + DEBUG("DHCP(%d) end : %s", i, def->ranges[i].end); + } + for (i = 0; i < def->nhosts; i++) { + DEBUG("DHCP Host(%d) mac : %s", i, def->hosts[i].mac); + DEBUG("DHCP Host(%d) name: %s", i, def->hosts[i].name); + DEBUG("DHCP Host(%d) ip : %s", i, def->hosts[i].ip); + }
This is rather overkill, since we already log the entire XML document passed into the public API which has all this info
+ +static virNetworkPtr vboxNetworkDefineXML(virConnectPtr conn, const char *xml) { + vboxGlobalData *data = conn->privateData; + virNetworkDefPtr def = NULL; + virNetworkPtr ret = NULL; + nsID *iid = NULL; + char *networkNameUtf8 = NULL; + int i = 0; + + /* vboxNetworkDefineXML() is not exactly "network definition" + * as the network is up and running, only the DHCP server is off, + * so you can always assign static IP and get the network running. + */ + if ((def = virNetworkDefParseString(conn, xml)) == NULL) + goto cleanup; + + if (VIR_ALLOC(iid) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + if (VIR_ALLOC_N(networkNameUtf8, sizeof("HostInterfaceNetworking-") + + sizeof(def->name) + 1) < 0) { + virReportOOMError(conn); + goto cleanup; + } + + strcpy (networkNameUtf8, "HostInterfaceNetworking-"); + strcat (networkNameUtf8, def->name);
Same note here, as before, and is most other methods that follow Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
+ + strcpy (networkNameUtf8, "HostInterfaceNetworking-"); + strcat (networkNameUtf8, def->name);
That said, how about just using virAsprintf(&nmetworkNameUtf8.. instead of alloc+strcpy
Done.
+ DEBUG("Network Name: %s", def->name); + DEBUG("Network UUID: " + "{%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}", + (unsigned)iid->m0, (unsigned)iid->m1, + (unsigned)iid->m2, (unsigned)iid->m3[0], + (unsigned)iid->m3[1], (unsigned)iid->m3[2], + (unsigned)iid->m3[3], (unsigned)iid->m3[4], + (unsigned)iid->m3[5], (unsigned)iid->m3[6], + (unsigned)iid->m3[7]);
This is rather overkill, since we already log the entire XML document passed into the public API which has all this info
just keeping the above two because atleast the debug logs should show the UUID user wants to set and the one which we have, the reason being the default hostonly adaptor in VirtualBox 2.2.* is created while install and can't be changed.
+ + strcpy (networkNameUtf8, "HostInterfaceNetworking-"); + strcat (networkNameUtf8, def->name);
Same note here, as before, and is most other methods that follow
Done. Will post a patch soon with other changes suggested on the list. Regards, Pritesh
On Wed, May 06, 2009 at 06:12:21PM +0200, Pritesh Kothari wrote:
Hi All,
As discussed on the list resending the networking patch's. the patch's are as below: [PATCH 3/3]: contains networking API for hostonly networks in VirtualBox driver in libvirt (it contains all the fix's proposed on list along with two extra *DefinedNetworks functions)
Hum, I have a doubt here:
+/** + * The Network Functions here on + */ +static virDrvOpenStatus vboxNetworkOpen(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED) { + vboxGlobalData *data = conn->privateData; [...] + DEBUG0("network intialized"); + /* conn->networkPrivateData = some network specific data */ + return VIR_DRV_OPEN_SUCCESS; [...] +static int vboxNetworkClose(virConnectPtr conn) { + DEBUG0("network unintialized"); + conn->networkPrivateData = NULL; + return 0; +}
You really don't need to keep any data about the networking driver in libvirt(d) itself ? [...]
+ /* Currently support only one dhcp server per network + * with contigious address space from start to end + */ + if ((def->nranges == 1) && + (def->ranges[0].start) && + (def->ranges[0].end)) {
Hum, what about at least allowing the first range if multiple were defined instead of disabling DHCP completely. I would suggest to start with if ((def->nranges >= 1) && instead. [...]
+ /* Currently support only one dhcp server per network + * with contigious address space from start to end + */ + if ((def->nranges == 1) && + (def->ranges[0].start) && + (def->ranges[0].end)) {
Same here. The patch is large but reuses the common routines for conversion to/from XML, a lot of the code is vbox internal structures peek/poke which are a bit hard to follow but this looks regular, the patch looks fine to me but feedback on previous points would be appreciated :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, May 11, 2009 at 12:17:17PM +0200, Daniel Veillard wrote:
On Wed, May 06, 2009 at 06:12:21PM +0200, Pritesh Kothari wrote:
+/** + * The Network Functions here on + */ +static virDrvOpenStatus vboxNetworkOpen(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + int flags ATTRIBUTE_UNUSED) { + vboxGlobalData *data = conn->privateData; [...] + DEBUG0("network intialized"); + /* conn->networkPrivateData = some network specific data */ + return VIR_DRV_OPEN_SUCCESS; [...] +static int vboxNetworkClose(virConnectPtr conn) { + DEBUG0("network unintialized"); + conn->networkPrivateData = NULL; + return 0; +}
You really don't need to keep any data about the networking driver in libvirt(d) itself ?
Nah, this is fine in the virtualbox case. The driver is essentially a stateless driver, the only info being the RPC function tables which are stored in conn->privateData. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
You really don't need to keep any data about the networking driver in libvirt(d) itself ?
as danpb explained it :)
+ /* Currently support only one dhcp server per network + * with contigious address space from start to end + */ + if ((def->nranges == 1) && + (def->ranges[0].start) && + (def->ranges[0].end)) {
Fixed this to def->nranges >= 1 instead of above.
The patch is large but reuses the common routines for conversion to/from XML, a lot of the code is vbox internal structures peek/poke which are a bit hard to follow but this looks regular, the patch looks fine to me but feedback on previous points would be appreciated :-)
Reposting the patch as below with all the fix's mentioned on the list. Regards, Pritesh.
On Mon, May 11, 2009 at 02:19:06PM +0200, Pritesh Kothari wrote:
You really don't need to keep any data about the networking driver in libvirt(d) itself ?
+static int vboxListNetworks(virConnectPtr conn, char **const names, int nnames) { + vboxGlobalData *data = conn->privateData; + int numActive = 0; + + if (data->vboxObj) { + IHost *host = NULL; + + data->vboxObj->vtbl->GetHost(data->vboxObj, &host); + if (host) { + int i = 0; + PRUint32 networkInterfacesSize = 0; + IHostNetworkInterface **networkInterfaces = NULL; + + host->vtbl->GetNetworkInterfaces(host, &networkInterfacesSize, &networkInterfaces); + + for (i = 0; (numActive < nnames) && (i < networkInterfacesSize); i++) { + if (networkInterfaces[i]) { + PRUint32 interfaceType = 0; + + networkInterfaces[i]->vtbl->GetInterfaceType(networkInterfaces[i], &interfaceType); + + if (interfaceType == HostNetworkInterfaceType_HostOnly) { + PRUint32 status = HostNetworkInterfaceStatus_Unknown; + + networkInterfaces[i]->vtbl->GetStatus(networkInterfaces[i], &status); + + if (status == HostNetworkInterfaceStatus_Up) { + char *nameUtf8 = NULL; + PRUnichar *nameUtf16 = NULL; + + networkInterfaces[i]->vtbl->GetName(networkInterfaces[i], &nameUtf16); + data->pFuncs->pfnUtf16ToUtf8(nameUtf16, &nameUtf8); + + DEBUG("nnames[%d]: %s", numActive, nameUtf8); + names[numActive++] = strdup(nameUtf8); + + data->pFuncs->pfnUtf8Free(nameUtf8); + data->pFuncs->pfnUtf16Free(nameUtf16); + } + } + } + } + + for (i = 0; i < networkInterfacesSize; i++) { + if (networkInterfaces[i]) { + networkInterfaces[i]->vtbl->nsisupports.Release((nsISupports *) networkInterfaces[i]); + } + } + + host->vtbl->nsisupports.Release((nsISupports *) host); + } + } + + return numActive; +}
There's a strdup() call here that's not checked for failure, and quite a few more in other functions in the patch. This reminds me that we really need to make a wrapper for strdup, as we did for malloc() to validate failure checking at compile time. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
There's a strdup() call here that's not checked for failure, and quite a few more in other functions in the patch.
Hopefully covered all the strdup's and fixed it Reposting the patch.
This reminds me that we really need to make a wrapper for strdup, as we did for malloc() to validate failure checking at compile time.
that would be very helpful :) Regards, Pritesh
On Mon, May 11, 2009 at 04:53:25PM +0200, Pritesh Kothari wrote:
There's a strdup() call here that's not checked for failure, and quite a few more in other functions in the patch.
Hopefully covered all the strdup's and fixed it
Reposting the patch.
Patch didn't applied cleanly due to other changes and conn being used while not declared, but I could fix those easilly. So that last patch is commited too, I think there is no VBox specific patch pending anymore, so please grab CVS head (or git head once it synchronize) and double check the status, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Pritesh Kothari