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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/