11:12 a.m.
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
5:15 a.m.
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 :|
6:42 a.m.
> +
> + 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
5:17 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5:42 a.m.
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 :|
7:19 a.m.
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.
8:43 a.m.
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 :|
9:53 a.m.
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
9:02 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5704
days inactive
5713
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Pritesh Kothari