[Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
When defining a guest's block device in the libvirt XML we currently allow the dev to be backed by either a file or block device in the host system. This is done by specifying the 'type' attribute when defing the block device XML. eg <disk type='file'> Or <disk type='block'> This is all fine & dandy for Xen 3.0.2, but in 3.0.3 we now have a choice of drivers for the 'file' backed block devices. We can either use the traditional loopback mounted system, or the new blktap system. To complicate things further still, blktap itself has man different drivers - one uses a raw file with Async-IO, another uses the QCow sparse format with AIO, and all sort of other possibilities are available... This means the easy option of just making all file devices use blktap in 3.0.3 is not practical. This in turns means we need to come up with a way to express the new driver methods in libvirt XML. There are a few options I can think of : - Allow more values in the 'type' attribute, eg file, block, blktap:aio, blktap:qcow, etc. This feels wrong because the blktap:* entries are really still 'file' types. - Introduce a new attribute 'driver' where you can specify 'loop', 'blktap:aio', 'blktap:qcow', etc - Introduce a new element 'driver' where you can specify the same <disk type="file"> <driver type='blktap:aio'/> </disk> - Same as above, but normalize the driver type further <disk type="file"> <driver type='blktap' backend='aio' /> </disk> My preference is probably option 3 or 4. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Tue, Sep 26, 2006 at 08:08:33PM +0100, Daniel P. Berrange wrote:
This means the easy option of just making all file devices use blktap in 3.0.3 is not practical. This in turns means we need to come up with a way to express the new driver methods in libvirt XML. There are a few options I can think of :
- Allow more values in the 'type' attribute, eg file, block, blktap:aio, blktap:qcow, etc. This feels wrong because the blktap:* entries are really still 'file' types.
- Introduce a new attribute 'driver' where you can specify 'loop', 'blktap:aio', 'blktap:qcow', etc
- Introduce a new element 'driver' where you can specify the same
<disk type="file"> <driver type='blktap:aio'/> </disk>
- Same as above, but normalize the driver type further
<disk type="file"> <driver type='blktap' backend='aio' /> </disk>
My preference is probably option 3 or 4.
My preference is definitely 4. (well, I'm affected by Database Normalization where it's bad merge two information into one attribute -- so 'blktap:aio' is wrong way ;-) Karel -- Karel Zak <kzak@redhat.com>
On Wed, Sep 27, 2006 at 07:05:38PM +0200, Karel Zak wrote:
On Tue, Sep 26, 2006 at 08:08:33PM +0100, Daniel P. Berrange wrote:
This means the easy option of just making all file devices use blktap in 3.0.3 is not practical. This in turns means we need to come up with a way to express the new driver methods in libvirt XML. There are a few options I can think of :
- Allow more values in the 'type' attribute, eg file, block, blktap:aio, blktap:qcow, etc. This feels wrong because the blktap:* entries are really still 'file' types.
- Introduce a new attribute 'driver' where you can specify 'loop', 'blktap:aio', 'blktap:qcow', etc
- Introduce a new element 'driver' where you can specify the same
<disk type="file"> <driver type='blktap:aio'/> </disk>
- Same as above, but normalize the driver type further
<disk type="file"> <driver type='blktap' backend='aio' /> </disk>
My preference is probably option 3 or 4.
My preference is definitely 4.
(well, I'm affected by Database Normalization where it's bad merge two information into one attribute -- so 'blktap:aio' is wrong way ;-)
I would agree that rather than glueing it's better to separate the 2 strings as separate attributes. But should the second be named 'backend' or something more generic like 'subtype'. That's the only slight concern I have but it's probably better to be explicit about and the suggested form is just fine. Let's go for 4/ :-) 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/
On Wed, Sep 27, 2006 at 01:16:17PM -0400, Daniel Veillard wrote:
On Wed, Sep 27, 2006 at 07:05:38PM +0200, Karel Zak wrote:
On Tue, Sep 26, 2006 at 08:08:33PM +0100, Daniel P. Berrange wrote:
This means the easy option of just making all file devices use blktap in 3.0.3 is not practical. This in turns means we need to come up with a way to express the new driver methods in libvirt XML. There are a few options I can think of :
- Allow more values in the 'type' attribute, eg file, block, blktap:aio, blktap:qcow, etc. This feels wrong because the blktap:* entries are really still 'file' types.
- Introduce a new attribute 'driver' where you can specify 'loop', 'blktap:aio', 'blktap:qcow', etc
- Introduce a new element 'driver' where you can specify the same
<disk type="file"> <driver type='blktap:aio'/> </disk>
- Same as above, but normalize the driver type further
<disk type="file"> <driver type='blktap' backend='aio' /> </disk>
My preference is probably option 3 or 4.
My preference is definitely 4.
(well, I'm affected by Database Normalization where it's bad merge two information into one attribute -- so 'blktap:aio' is wrong way ;-)
I would agree that rather than glueing it's better to separate the 2 strings as separate attributes. But should the second be named 'backend' or something more generic like 'subtype'. That's the only slight concern I have but it's probably better to be explicit about and the suggested form is just fine. Let's go for 4/ :-)
Ok, looks like we have total agreement. I'll knock up a patch implementing option #4 and post it for review. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Wed, Sep 27, 2006 at 06:36:38PM +0100, Daniel P. Berrange wrote:
On Wed, Sep 27, 2006 at 01:16:17PM -0400, Daniel Veillard wrote:
On Wed, Sep 27, 2006 at 07:05:38PM +0200, Karel Zak wrote:
On Tue, Sep 26, 2006 at 08:08:33PM +0100, Daniel P. Berrange wrote:
This means the easy option of just making all file devices use blktap in 3.0.3 is not practical. This in turns means we need to come up with a way to express the new driver methods in libvirt XML. There are a few options I can think of :
- Allow more values in the 'type' attribute, eg file, block, blktap:aio, blktap:qcow, etc. This feels wrong because the blktap:* entries are really still 'file' types.
- Introduce a new attribute 'driver' where you can specify 'loop', 'blktap:aio', 'blktap:qcow', etc
- Introduce a new element 'driver' where you can specify the same
<disk type="file"> <driver type='blktap:aio'/> </disk>
- Same as above, but normalize the driver type further
<disk type="file"> <driver type='blktap' backend='aio' /> </disk>
My preference is probably option 3 or 4.
My preference is definitely 4.
(well, I'm affected by Database Normalization where it's bad merge two information into one attribute -- so 'blktap:aio' is wrong way ;-)
I would agree that rather than glueing it's better to separate the 2 strings as separate attributes. But should the second be named 'backend' or something more generic like 'subtype'. That's the only slight concern I have but it's probably better to be explicit about and the suggested form is just fine. Let's go for 4/ :-)
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. 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. 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. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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/
On Fri, Oct 06, 2006 at 04:44:17AM -0400, Daniel Veillard wrote:
On Thu, Oct 05, 2006 at 07:23:15PM +0100, Daniel P. Berrange wrote:
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.
Will do.
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
Don't know what I was thinking. Clearly I should be checking malloc() calls for failure. Will fix.
+ 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.
Will fix.
+ } 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
Yes, its fine for this to be NULL - i guess I was just lucky that this next strcmp didn't crash when passed NULL. Will fix.
+ if (!strcmp((const char *)drvName, "tap")) + drvType = xmlGetProp(cur, BAD_CAST "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { ro = 1; }
Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Fri, Oct 06, 2006 at 04:44:17AM -0400, Daniel Veillard wrote:
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.
Attached is an update version of the patch addressing the issues mentioned. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Karel Zak