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 -=|
Index: xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.66
diff -u -r1.66 xend_internal.c
--- xend_internal.c 29 Sep 2006 12:00:58 -0000 1.66
+++ xend_internal.c 6 Oct 2006 13:52:03 -0000
@@ -1451,7 +1451,7 @@
/**
* xend_parse_sexp_desc:
- * @domain: the domain associated with the XML
+ * @conn: the connection associated with the XML
* @root: the root of the parsed S-Expression
*
* Parse the xend sexp description and turn it into the XML format similar
@@ -1487,7 +1487,7 @@
tmp = sexpr_node(root, "domain/name");
if (tmp == NULL) {
- virXendError(NULL, VIR_ERR_INTERNAL_ERROR,
+ virXendError(conn, VIR_ERR_INTERNAL_ERROR,
_("domain information incomplete, missing name"));
goto error;
}
@@ -1498,10 +1498,11 @@
int i, j;
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];
- else if ((tmp[j] >= 'A') && (tmp[j] <= 'F'))
+ ((tmp[j] >= 'a') && (tmp[j] <= 'f'))) {
+ compact[i++] = tmp[j];
+ } else if ((tmp[j] >= 'A') && (tmp[j] <= 'F')) {
compact[i++] = tmp[j] + 'a' - 'A';
+ }
}
compact[i] = 0;
if (i > 0)
@@ -1509,7 +1510,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 +1523,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,105 +1547,150 @@
/* 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;
- }
+ /* Normally disks are in a (device (vbd ...)) block
+ but blktap disks ended up in a differently named
+ (device (tap ....)) block.... */
+ 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;
+
+ /* Again dealing with (vbd...) vs (tap ...) differences */
+ 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(conn, VIR_ERR_INTERNAL_ERROR,
+ _("domain information incomplete, vbd has no src"));
+ goto bad_parse;
+ }
+
+ if (dst == NULL) {
+ virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("domain information incomplete, vbd has no dev"));
+ goto bad_parse;
+ }
+
+
+ offset = strchr(src, ':');
+ if (!offset) {
+ virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse vbd filename, missing driver name"));
+ goto bad_parse;
+ }
+
+ drvName = malloc((offset-src)+1);
+ if (!drvName) {
+ virXendError(conn, VIR_ERR_NO_MEMORY,
+ _("allocate new buffer"));
+ goto bad_parse;
+ }
+ strncpy(drvName, src, (offset-src));
+ drvName[offset-src] = '\0';
+
+ src = offset + 1;
+
+ if (!strcmp(drvName, "tap")) {
+ offset = strchr(src, ':');
+ if (!offset) {
+ virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse vbd filename, missing driver type"));
+ 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;
+ drvType = malloc((offset-src)+1);
+ if (!drvType) {
+ virXendError(conn, VIR_ERR_NO_MEMORY,
+ _("allocate new buffer"));
+ goto bad_parse;
}
+ strncpy(drvType, src, (offset-src));
+ drvType[offset-src] = '\0';
+ src = offset + 1;
+ /* Its possible to use blktap driver for block devs
+ too, but kinda pointless because blkback is better,
+ so we assume common case here. If blktap becomes
+ omnipotent, we can revisit this, perhaps stat()'ing
+ the src file in question */
+ isBlock = 0;
+ } else if (!strcmp(drvName, "phy")) {
+ isBlock = 1;
+ } else if (!strcmp(drvName, "file")) {
+ isBlock = 0;
+ }
+
+ if (!strncmp(dst, "ioemu:", 6))
+ dst += 6;
- 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';
+ /* 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")) {
+ /* The default anyway */
+ } else {
+ /* Unknown, lets pretend its a disk too */
}
+ 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;
+ const char *tmp2;
tmp = sexpr_node(node, "device/vif/bridge");
- tmp2 = sexpr_node(node, "device/vif/script");
+ tmp2 = sexpr_node(node, "device/vif/script");
if ((tmp != NULL) || (strstr(tmp2, "bridge"))) {
virBufferVSprintf(&buf, " <interface type='bridge'>\n");
- if (tmp != NULL)
- virBufferVSprintf(&buf, " <source bridge='%s'/>\n",
- tmp);
+ if (tmp != NULL)
+ virBufferVSprintf(&buf, " <source bridge='%s'/>\n",
+ tmp);
tmp = sexpr_node(node, "device/vif/vifname");
if (tmp != NULL)
virBufferVSprintf(&buf, " <target dev='%s'/>\n",
@@ -1688,10 +1734,11 @@
}
/* Old style cdrom config from Xen <= 3.0.2 */
- if (xendConfigVersion == 1) {
+ if (xendConfigVersion == 1) {
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);
@@ -1699,24 +1746,24 @@
}
}
}
-
+
/* Graphics device */
tmp = sexpr_fmt_node(root, "domain/image/%s/vnc", hvm ? "hvm" : "linux");
if (tmp != NULL) {
if (tmp[0] == '1') {
int port = xenStoreDomainGetVNCPort(conn, domid);
- if (port == -1)
+ if (port == -1)
port = 5900 + domid;
virBufferVSprintf(&buf, " <graphics type='vnc' port='%d'/>\n", port);
}
}
-
+
tmp = sexpr_fmt_node(root, "domain/image/%s/sdl", hvm ? "hvm" : "linux");
if (tmp != NULL) {
if (tmp[0] == '1')
virBufferAdd(&buf, " <graphics type='sdl'/>\n", 27 );
}
-
+
tty = xenStoreDomainGetConsolePath(conn, domid);
if (tty) {
virBufferVSprintf(&buf, " <console tty='%s'/>\n", tty);
Index: xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.41
diff -u -r1.41 xml.c
--- xml.c 21 Sep 2006 15:24:37 -0000 1.41
+++ xml.c 6 Oct 2006 13:52:03 -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;
@@ -934,7 +936,7 @@
xmlFree(type);
}
device = xmlGetProp(node, BAD_CAST "device");
-
+
cur = node->children;
while (cur != NULL) {
if (cur->type == XML_ELEMENT_NODE) {
@@ -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 (drvName && !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);
+ /* Normally disks are in a (device (vbd ...)) block
+ but blktap disks ended up in a differently named
+ (device (tap ....)) block.... */
+ 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);