2:08 p.m.
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 -=|
12:05 p.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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(a)redhat.com>
12:16 p.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:36 p.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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 -=|
1:23 p.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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 -=|
3:44 a.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:57 a.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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 -=|
7:58 a.m.
New subject: [Libvir] Supporting new Xen 3.0.3 blktap backend for file devices
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 -=|
6651
days inactive
6661
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Karel Zak