Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
> 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 commit 9541fea6230439d04688843fff010ea3250a5ed4 Author: pk221555 Date: Mon May 11 16:45:05 2009 +0200 libvirt: networking API for hostonly networks diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c index 7db437c..6b782b1 100644 --- a/src/vbox/vbox_driver.c +++ b/src/vbox/vbox_driver.c @@ -39,19 +39,23 @@ extern virDriver vbox22Driver; +extern virNetworkDriver vbox22NetworkDriver; #if 0 extern virDriver vbox25Driver; +extern virNetworkDriver vbox25NetworkDriver; #endif int vboxRegister(void) { virDriverPtrdriver; +virNetworkDriverPtr networkDriver; uint32_tuVersion; /* vboxRegister() shouldn't fail as that will render libvirt unless. * So, we use the v2.2 driver as a fallback/dummy. */ driver= &vbox22Driver; +networkDriver = &vbox22NetworkDriver; /* Init the glue and get the API version. */ if (VBoxCGlueInit() == 0) { @@ -69,10 +73,12 @@ int vboxRegister(void) { if (uVersion >= 2001052 && uVersion < 2002051) { DEBUG0("VirtualBox API version: 2.2"); driver= &vbox22Driver; +networkDriver = &vbox22NetworkDriver; #if 0 } else if (uVersion >= 2002051 && uVersion < 2005051) { DEBUG0("VirtualBox API version: 2.5"); driver= &vbox25Driver; +networkDriver = &vbox25NetworkDriver; #endif } else { DEBUG0("Unsupport VirtualBox API version"); @@ -84,6 +90,8 @@ int vboxRegister(void) { if (virRegisterDriver(driver) < 0) return -1; +if (virRegisterNetworkDriver(networkDriver) < 0) +return -1; return 0; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index a27eada..e2d0f03 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3633,6 +3633,1021 @@ cleanup: return ret; } +/** + * The Network Functions here on + */ +static virDrvOpenStatus vboxNetworkOpen(virConnectPtr conn, +virConnectAuthPtr auth ATTRIBUTE_UNUSED, +int flags ATTRIBUTE_UNUSED) { +vboxGlobalData *data = conn->privateData; + +if (STRNEQ(conn->driver->name, "VBOX")) +goto cleanup; + +if ((data->pFuncs == NULL) || +(data->vboxObj == NULL) || +(data->vboxSession == NULL)) +goto cleanup; + +DEBUG0("network intialized"); +/* conn->networkPrivateData = some network specific data */ +return VIR_DRV_OPEN_SUCCESS; + +cleanup: +return VIR_DRV_OPEN_DECLINED; +} + +static int vboxNetworkClose(virConnectPtr conn) { +DEBUG0("network unintialized"); +conn->networkPrivateData = NULL; +return 0; +} + +static int vboxNumOfNetworks(virConnectPtr conn) { +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; 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) { +numActive++; +} +} + +networkInterfaces[i]->vtbl->nsisupports.Release((nsISupports *) networkInterfaces[i]); +} +} + +host->vtbl->nsisupports.Release((nsISupports *) host); +} +} + +DEBUG("numActive: %d", numActive); +return numActive; +} + +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; +IHostNetwor
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
> 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. commit ec631aa294a08bca13543289368449886d5398b0 Author: pk221555 Date: Mon May 11 14:15:31 2009 +0200 libvirt: networking API for hostonly networks diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c index 7db437c..6b782b1 100644 --- a/src/vbox/vbox_driver.c +++ b/src/vbox/vbox_driver.c @@ -39,19 +39,23 @@ extern virDriver vbox22Driver; +extern virNetworkDriver vbox22NetworkDriver; #if 0 extern virDriver vbox25Driver; +extern virNetworkDriver vbox25NetworkDriver; #endif int vboxRegister(void) { virDriverPtrdriver; +virNetworkDriverPtr networkDriver; uint32_tuVersion; /* vboxRegister() shouldn't fail as that will render libvirt unless. * So, we use the v2.2 driver as a fallback/dummy. */ driver= &vbox22Driver; +networkDriver = &vbox22NetworkDriver; /* Init the glue and get the API version. */ if (VBoxCGlueInit() == 0) { @@ -69,10 +73,12 @@ int vboxRegister(void) { if (uVersion >= 2001052 && uVersion < 2002051) { DEBUG0("VirtualBox API version: 2.2"); driver= &vbox22Driver; +networkDriver = &vbox22NetworkDriver; #if 0 } else if (uVersion >= 2002051 && uVersion < 2005051) { DEBUG0("VirtualBox API version: 2.5"); driver= &vbox25Driver; +networkDriver = &vbox25NetworkDriver; #endif } else { DEBUG0("Unsupport VirtualBox API version"); @@ -84,6 +90,8 @@ int vboxRegister(void) { if (virRegisterDriver(driver) < 0) return -1; +if (virRegisterNetworkDriver(networkDriver) < 0) +return -1; return 0; } diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index a27eada..8e42958 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -3633,6 +3633,1001 @@ cleanup: return ret; } +/** + * The Network Functions here on + */ +static virDrvOpenStatus vboxNetworkOpen(virConnectPtr conn, +virConnectAuthPtr auth ATTRIBUTE_UNUSED, +int flags ATTRIBUTE_UNUSED) { +vboxGlobalData *data = conn->privateData; + +if (STRNEQ(conn->driver->name, "VBOX")) +goto cleanup; + +if ((data->pFuncs == NULL) || +(data->vboxObj == NULL) || +(data->vboxSession == NULL)) +goto cleanup; + +DEBUG0("network intialized"); +/* conn->networkPrivateData = some network specific data */ +return VIR_DRV_OPEN_SUCCESS; + +cleanup: +return VIR_DRV_OPEN_DECLINED; +} + +static int vboxNetworkClose(virConnectPtr conn) { +DEBUG0("network unintialized"); +conn->networkPrivateData = NULL; +return 0; +} + +static int vboxNumOfNetworks(virConnectPtr conn) { +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; 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) { +numActive++; +} +} + +networkInterfaces[i]->vtbl->nsisupports.Release((nsISupports *) networkInterfaces[i]); +} +} + +
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
> > + > > +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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] networking API for hostonly networks in VirtualBox driver in libvirt
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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list