On Thu, Oct 05, 2006 at 07:23:15PM +0100, Daniel P. Berrange wrote:
On Wed, Sep 27, 2006 at 06:36:38PM +0100, Daniel P. Berrange wrote:
Ok, looks like we have total agreement. I'll knock up a patch implementing option #4 and post it for review.
Attached is a patch which implements option 4. If no <driver> block is supplied, then we continue existing behavior of using file: for file backed disks, and phy: for block backed disks. Any of the following are then valid:
<driver name='file'/> -> file: <driver name='phy'/> -> phy: <driver name='tap'/> -> tap:aio: <driver name='tap' type='aio'/> -> tap:aio: <driver name='tap' type='qcow'/> -> tap:qcow:
The only supported names are 'file', 'phy' & 'tap'. If the name is 'tap' then it is valid to use an additional 'type' attributte. We don't do any checking on cotents of 'type' attribute, it is just passed straight through to xend, since the blktap driver has a wide variety of types available. When fetching XML from libvirt you are now guarenteed to be given a <driver> block in each disk.
Okay,
The patch was rather tedious, because not only did the blktap stuff change the prefix used on file paths in the (uname) SEXPR block, but it actually changed the entire enclosing block from (vbd ...) to (tap ...) for some crazy reason.
Oh well ... at some point we will need to look at the new interfaces for xend too, but one thing at a time :-)
I'm not attaching them here because it would bloat the patch, but I've written a tonne more testfiles for the xml <-> sexpr conversions to validate the patch is operating correctly.
Great !
Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.66 diff -u -r1.66 xend_internal.c --- src/xend_internal.c 29 Sep 2006 12:00:58 -0000 1.66 +++ src/xend_internal.c 5 Oct 2006 18:12:33 -0000 @@ -1499,7 +1499,7 @@ for (i = 0, j = 0;(i < 32) && (tmp[j] != 0);j++) { if (((tmp[j] >= '0') && (tmp[j] <= '9')) || ((tmp[j] >= 'a') && (tmp[j] <= 'f'))) - compact[i++] = tmp[j]; + compact[i++] = tmp[j];
maybe we should just add the full set of { } for the innner constructs too if reformatting.
else if ((tmp[j] >= 'A') && (tmp[j] <= 'F')) compact[i++] = tmp[j] + 'a' - 'A'; } @@ -1546,95 +1546,116 @@ + drvName = malloc((offset-src)+1);
I agree that if we OOM there it's gonna be messy anyway but let's catch NULL returns on allocs as much as possible
+ strncpy(drvName, src, (offset-src)); + drvName[offset-src] = '\0'; + + src = offset + 1; + + if (!strcmp(drvName, "tap")) { + offset = strchr(src, ':'); + if (!offset) + goto bad_parse; + + drvType = malloc((offset-src)+1);
Same here. If failing a libvirt error and going to bad_parse should be sufficient I guess.
+ } else if (!strcmp(offset, ":disk")) { + /* defualt anyway */
/* default */ :-)
+ } else if ((drvName == NULL) && + (xmlStrEqual(cur->name, BAD_CAST "driver"))) { + drvName = xmlGetProp(cur, BAD_CAST "name");
testing for NULL would be good, if the attribute is missing we should not crash
+ if (!strcmp((const char *)drvName, "tap")) + drvType = xmlGetProp(cur, BAD_CAST "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { ro = 1; } @@ -972,14 +979,14 @@ /* Xend (all versions) put the floppy device config * under the hvm (image (os)) block */ - if (hvm && + if (hvm && device && !strcmp((const char *)device, "floppy")) { return 0; }
/* Xend <= 3.0.2 doesn't include cdrom config here */ - if (hvm && + if (hvm && device && !strcmp((const char *)device, "cdrom")) { if (xendConfigVersion == 1) @@ -990,7 +997,14 @@
virBufferAdd(buf, "(device ", 8); - virBufferAdd(buf, "(vbd ", 5); + /* Why, oh why did this need to change as well as the + specifying tap in the (uname..) block ??!!?! Crazy + Xen formats :-( */
I undestand the frustration, but 3 years from now the comment will lack relevance, unless you give the full picture :-) thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/