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 -=|
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];
else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
compact[i++] = tmp[j] + 'a' - 'A';
}
@@ -1509,7 +1509,7 @@
}
tmp = sexpr_node(root, "domain/bootloader");
if (tmp != NULL)
- virBufferVSprintf(&buf, " <bootloader>%s</bootloader>\n", tmp);
+ virBufferVSprintf(&buf, " <bootloader>%s</bootloader>\n", tmp);
if (sexpr_lookup(root, "domain/image")) {
hvm = sexpr_lookup(root, "domain/image/hvm") ? 1 : 0;
@@ -1522,13 +1522,13 @@
sexpr_int(root, "domain/vcpus"));
tmp = sexpr_node(root, "domain/on_poweroff");
if (tmp != NULL)
- virBufferVSprintf(&buf, " <on_poweroff>%s</on_poweroff>\n", tmp);
+ virBufferVSprintf(&buf, " <on_poweroff>%s</on_poweroff>\n", tmp);
tmp = sexpr_node(root, "domain/on_reboot");
if (tmp != NULL)
- virBufferVSprintf(&buf, " <on_reboot>%s</on_reboot>\n", tmp);
+ virBufferVSprintf(&buf, " <on_reboot>%s</on_reboot>\n", tmp);
tmp = sexpr_node(root, "domain/on_crash");
if (tmp != NULL)
- virBufferVSprintf(&buf, " <on_crash>%s</on_crash>\n", tmp);
+ virBufferVSprintf(&buf, " <on_crash>%s</on_crash>\n", tmp);
if (hvm) {
virBufferAdd(&buf, " <features>\n", 13);
@@ -1546,95 +1546,116 @@
/* in case of HVM we have devices emulation */
tmp = sexpr_node(root, "domain/image/hvm/device_model");
if ((tmp != NULL) && (tmp[0] != 0))
- virBufferVSprintf(&buf, " <emulator>%s</emulator>\n", tmp);
+ virBufferVSprintf(&buf, " <emulator>%s</emulator>\n", tmp);
for (cur = root; cur->kind == SEXPR_CONS; cur = cur->cdr) {
node = cur->car;
- if (sexpr_lookup(node, "device/vbd")) {
- tmp = sexpr_node(node, "device/vbd/uname");
- if (tmp == NULL)
- continue;
- if (!memcmp(tmp, "file:", 5)) {
- int cdrom = 0;
- const char *src = tmp+5;
- const char *dst = sexpr_node(node, "device/vbd/dev");
-
- if (dst == NULL) {
- virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
- _("domain information incomplete, vbd has no dev"));
- goto error;
- }
+ if (sexpr_lookup(node, "device/vbd") ||
+ sexpr_lookup(node, "device/tap")) {
+ char *offset;
+ int isBlock = 0;
+ int cdrom = 0;
+ char *drvName = NULL;
+ char *drvType = NULL;
+ const char *src = NULL;
+ const char *dst = NULL;
+ const char *mode = NULL;
+ /* Why, oh why did this need to change as well as the
+ specifying tap in the (uname..) block ??!!?! Crazy
+ Xen formats :-( */
+ if (sexpr_lookup(node, "device/vbd")) {
+ src = sexpr_node(node, "device/vbd/uname");
+ dst = sexpr_node(node, "device/vbd/dev");
+ mode = sexpr_node(node, "device/vbd/mode");
+ } else {
+ src = sexpr_node(node, "device/tap/uname");
+ dst = sexpr_node(node, "device/tap/dev");
+ mode = sexpr_node(node, "device/tap/mode");
+ }
- if (!strncmp(dst, "ioemu:", 6))
- dst += 6;
- /* New style disk config from Xen >= 3.0.3 */
- if (xendConfigVersion > 1) {
- char *offset = rindex(dst, ':');
- if (offset) {
- if (!strcmp(offset, ":cdrom")) {
- cdrom = 1;
- } else if (!strcmp(offset, ":disk")) {
- /* defualt anyway */
- } else {
- /* Unknown, lets pretend its a disk */
- }
- offset[0] = '\0';
- }
- }
+ if (src == NULL) {
+ virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+ _("domain information incomplete, vbd has no src"));
+ goto bad_parse;
+ }
- virBufferVSprintf(&buf, " <disk type='file' device='%s'>\n", cdrom ? "cdrom" : "disk");
- virBufferVSprintf(&buf, " <source file='%s'/>\n", src);
- virBufferVSprintf(&buf, " <target dev='%s'/>\n", dst);
- tmp = sexpr_node(node, "device/vbd/mode");
- /* XXX should we force mode == r, if cdrom==1, or assume
- xend has already done this ? */
- if ((tmp != NULL) && (!strcmp(tmp, "r")))
- virBufferVSprintf(&buf, " <readonly/>\n");
- virBufferAdd(&buf, " </disk>\n", 12);
- } else if (!memcmp(tmp, "phy:", 4)) {
- int cdrom = 0;
- const char *src = tmp+4;
- const char *dst = sexpr_node(node, "device/vbd/dev");
-
- if (dst == NULL) {
- virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
- _("domain information incomplete, vbd has no dev"));
- goto error;
- }
+ if (dst == NULL) {
+ virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+ _("domain information incomplete, vbd has no dev"));
+ goto bad_parse;
+ }
+
+
+ offset = strchr(src, ':');
+ if (!offset)
+ goto bad_parse;
+
+ drvName = malloc((offset-src)+1);
+ 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);
+ strncpy(drvType, src, (offset-src));
+ drvType[offset-src] = '\0';
+ src = offset + 1;
+ } else if (!strcmp(drvName, "phy")) {
+ isBlock = 1;
+ } else if (!strcmp(drvName, "file")) {
+ isBlock = 0;
+ }
- if (!strncmp(dst, "ioemu:", 6))
- dst += 6;
- /* New style cdrom config from Xen >= 3.0.3 */
- if (xendConfigVersion > 1) {
- char *offset = rindex(dst, ':');
- if (offset) {
- if (!strcmp(offset, ":cdrom")) {
- cdrom = 1;
- } else if (!strcmp(offset, ":disk")) {
- /* defualt anyway */
- } else {
- /* Unknown, lets pretend its a disk */
- }
- offset[0] = '\0';
+ if (!strncmp(dst, "ioemu:", 6))
+ dst += 6;
+
+ /* New style disk config from Xen >= 3.0.3 */
+ if (xendConfigVersion > 1) {
+ offset = rindex(dst, ':');
+ if (offset) {
+ if (!strcmp(offset, ":cdrom")) {
+ cdrom = 1;
+ } else if (!strcmp(offset, ":disk")) {
+ /* defualt anyway */
+ } else {
+ /* Unknown, lets pretend its a disk */
}
+ offset[0] = '\0';
}
+ }
- virBufferVSprintf(&buf, " <disk type='block' device='%s'>\n", cdrom ? "cdrom" : "disk");
+ virBufferVSprintf(&buf, " <disk type='%s' device='%s'>\n",
+ isBlock ? "block" : "file",
+ cdrom ? "cdrom" : "disk");
+ if (drvType) {
+ virBufferVSprintf(&buf, " <driver name='%s' type='%s'/>\n", drvName, drvType);
+ } else {
+ virBufferVSprintf(&buf, " <driver name='%s'/>\n", drvName);
+ }
+ if (isBlock) {
virBufferVSprintf(&buf, " <source dev='%s'/>\n", src);
- virBufferVSprintf(&buf, " <target dev='%s'/>\n", dst);
- tmp = sexpr_node(node, "device/vbd/mode");
- /* XXX should we force mode == r, if cdrom==1, or assume
- xend has already done this ? */
- if ((tmp != NULL) && (!strcmp(tmp, "r")))
- virBufferVSprintf(&buf, " <readonly/>\n");
- virBufferAdd(&buf, " </disk>\n", 12);
} else {
- char serial[1000];
+ virBufferVSprintf(&buf, " <source file='%s'/>\n", src);
+ }
+ virBufferVSprintf(&buf, " <target dev='%s'/>\n", dst);
- TODO sexpr2string(node, serial, 1000);
- virBufferVSprintf(&buf, "<!-- Failed to parse %s -->\n",
- serial);
- TODO}
+
+ /* XXX should we force mode == r, if cdrom==1, or assume
+ xend has already done this ? */
+ if ((mode != NULL) && (!strcmp(mode, "r")))
+ virBufferVSprintf(&buf, " <readonly/>\n");
+ virBufferAdd(&buf, " </disk>\n", 12);
+
+ bad_parse:
+ if (drvName)
+ free(drvName);
+ if (drvType)
+ free(drvType);
} else if (sexpr_lookup(node, "device/vif")) {
const char *tmp2;
@@ -1692,6 +1713,7 @@
tmp = sexpr_node(root, "domain/image/hvm/cdrom");
if ((tmp != NULL) && (tmp[0] != 0)) {
virBufferAdd(&buf, " <disk type='file' device='cdrom'>\n", 38);
+ virBufferAdd(&buf, " <driver name='file'/>\n", 28);
virBufferVSprintf(&buf, " <source file='%s'/>\n", tmp);
virBufferAdd(&buf, " <target dev='hdc'/>\n", 26);
virBufferAdd(&buf, " <readonly/>\n", 18);
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.41
diff -u -r1.41 xml.c
--- src/xml.c 21 Sep 2006 15:24:37 -0000 1.41
+++ src/xml.c 5 Oct 2006 18:12:34 -0000
@@ -921,6 +921,8 @@
xmlChar *device = NULL;
xmlChar *source = NULL;
xmlChar *target = NULL;
+ xmlChar *drvName = NULL;
+ xmlChar *drvType = NULL;
int ro = 0;
int typ = 0;
int cdrom = 0;
@@ -948,6 +950,11 @@
} else if ((target == NULL) &&
(xmlStrEqual(cur->name, BAD_CAST "target"))) {
target = xmlGetProp(cur, BAD_CAST "dev");
+ } else if ((drvName == NULL) &&
+ (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
+ drvName = xmlGetProp(cur, BAD_CAST "name");
+ 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 :-( */
+ if (drvName && !strcmp((const char *)drvName, "tap")) {
+ virBufferAdd(buf, "(tap ", 5);
+ } else {
+ virBufferAdd(buf, "(vbd ", 5);
+ }
if (hvm) {
char *tmp = (char *)target;
@@ -1000,19 +1014,32 @@
/* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */
if (xendConfigVersion == 1)
- virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *) tmp);
+ virBufferVSprintf(buf, "(dev 'ioemu:%s')", (const char *)tmp);
else /* But newer does not */
- virBufferVSprintf(buf, "(dev '%s%s')", (const char *) tmp, cdrom ? ":cdrom" : ":disk");
+ virBufferVSprintf(buf, "(dev '%s%s')", (const char *)tmp, cdrom ? ":cdrom" : ":disk");
} else
- virBufferVSprintf(buf, "(dev '%s')", (const char *) target);
+ virBufferVSprintf(buf, "(dev '%s')", (const char *)target);
- if (typ == 0)
- virBufferVSprintf(buf, "(uname 'file:%s')", source);
- else if (typ == 1) {
- if (source[0] == '/')
- virBufferVSprintf(buf, "(uname 'phy:%s')", source);
- else
- virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+ if (drvName) {
+ if (!strcmp((const char *)drvName, "tap")) {
+ virBufferVSprintf(buf, "(uname '%s:%s:%s')",
+ (const char *)drvName,
+ (drvType ? (const char *)drvType : "aio"),
+ (const char *)source);
+ } else {
+ virBufferVSprintf(buf, "(uname '%s:%s')",
+ (const char *)drvName,
+ (const char *)source);
+ }
+ } else {
+ if (typ == 0)
+ virBufferVSprintf(buf, "(uname 'file:%s')", source);
+ else if (typ == 1) {
+ if (source[0] == '/')
+ virBufferVSprintf(buf, "(uname 'phy:%s')", source);
+ else
+ virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
+ }
}
if (ro == 0)
virBufferVSprintf(buf, "(mode 'w')");
@@ -1021,6 +1048,9 @@
virBufferAdd(buf, ")", 1);
virBufferAdd(buf, ")", 1);
+ xmlFree(drvType);
+ xmlFree(drvName);
+ xmlFree(device);
xmlFree(target);
xmlFree(source);
return (0);