It is possible for disks to be listed without a source file against them, eg a CDROM device with no media loaded. The XenD driver handles this, but the XM driver incorrectly generates XML with a <source file=''/> element instead of omitting the element entirely. This causes a bogus SXEXPR to be sent to XenD when starting the domain. This patch does three things - Makes the generic domain_conf.c XML parser accept XML docs with a bogus <source file=''/> and convert the source to NULL, instead of passing along the empty string "". This protects against broken apps - Makes the XM driver correctly generate XML in the first place, so omitting the <source> tag entirely. This is the root cause fix - Adds a test case for the XM driver to validate handling of devices with a source file src/domain_conf.c | 8 +++ src/xm_internal.c | 70 ++++++++++++++++------------ tests/xmconfigdata/test-no-source-cdrom.cfg | 23 +++++++++ tests/xmconfigdata/test-no-source-cdrom.xml | 46 ++++++++++++++++++ tests/xmconfigtest.c | 1 5 files changed, 120 insertions(+), 28 deletions(-) Daniel Index: src/xm_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xm_internal.c,v retrieving revision 1.102 diff -u -p -r1.102 xm_internal.c --- src/xm_internal.c 25 Nov 2008 11:18:08 -0000 1.102 +++ src/xm_internal.c 27 Nov 2008 12:14:57 -0000 @@ -828,7 +828,7 @@ xenXMDomainConfigParse(virConnectPtr con while (list) { char *head; char *offset; - char *tmp, *tmp1; + char *tmp; if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) goto skipdisk; @@ -850,10 +850,15 @@ xenXMDomainConfigParse(virConnectPtr con goto skipdisk; if ((offset - head) >= (PATH_MAX-1)) goto skipdisk; - if (VIR_ALLOC_N(disk->src, (offset - head) + 1) < 0) - goto no_memory; - strncpy(disk->src, head, (offset - head)); - disk->src[(offset-head)] = '\0'; + + if (offset == head) { + disk->src = NULL; /* No source file given, eg CDROM with no media */ + } else { + if (VIR_ALLOC_N(disk->src, (offset - head) + 1) < 0) + goto no_memory; + strncpy(disk->src, head, (offset - head)); + disk->src[(offset-head)] = '\0'; + } head = offset + 1; /* Remove legacy ioemu: junk */ @@ -871,32 +876,41 @@ xenXMDomainConfigParse(virConnectPtr con /* Extract source driver type */ - if (disk->src && - (tmp = strchr(disk->src, ':')) != NULL) { - if (VIR_ALLOC_N(disk->driverName, (tmp - disk->src) + 1) < 0) - goto no_memory; - strncpy(disk->driverName, disk->src, (tmp - disk->src)); - disk->driverName[tmp - disk->src] = '\0'; - } else { - if (!(disk->driverName = strdup("phy"))) - goto no_memory; - tmp = disk->src; - } + if (disk->src) { + /* The main type phy:, file:, tap: ... */ + if ((tmp = strchr(disk->src, ':')) != NULL) { + if (VIR_ALLOC_N(disk->driverName, (tmp - disk->src) + 1) < 0) + goto no_memory; + strncpy(disk->driverName, disk->src, (tmp - disk->src)); + disk->driverName[tmp - disk->src] = '\0'; - /* And the source driver sub-type */ - if (STRPREFIX(disk->driverName, "tap")) { - if (!(tmp1 = strchr(tmp+1, ':')) || !tmp1[0]) - goto skipdisk; - if (VIR_ALLOC_N(disk->driverType, (tmp1-(tmp+1))) < 0) - goto no_memory; - strncpy(disk->driverType, tmp+1, (tmp1-(tmp+1))); - memmove(disk->src, disk->src+(tmp1-disk->src)+1, strlen(disk->src)-(tmp1-disk->src)); - } else { - disk->driverType = NULL; - if (disk->src[0] && tmp) - memmove(disk->src, disk->src+(tmp-disk->src)+1, strlen(disk->src)-(tmp-disk->src)); + /* Strip the prefix we found off the source file name */ + memmove(disk->src, disk->src+(tmp-disk->src)+1, + strlen(disk->src)-(tmp-disk->src)); + } + + /* And the sub-type for tap:XXX: type */ + if (disk->driverName && + STREQ(disk->driverName, "tap")) { + if (!(tmp = strchr(disk->src, ':'))) + goto skipdisk; + if (VIR_ALLOC_N(disk->driverType, (tmp - disk->src) + 1) < 0) + goto no_memory; + strncpy(disk->driverType, disk->src, (tmp - disk->src)); + disk->driverType[tmp - disk->src] = '\0'; + + /* Strip the prefix we found off the source file name */ + memmove(disk->src, disk->src+(tmp-disk->src)+1, + strlen(disk->src)-(tmp-disk->src)); + } } + /* No source, or driver name, so fix to phy: */ + if (!disk->driverName && + !(disk->driverName = strdup("phy"))) + goto no_memory; + + /* phy: type indicates a block device */ disk->type = STREQ(disk->driverName, "phy") ? VIR_DOMAIN_DISK_TYPE_BLOCK : VIR_DOMAIN_DISK_TYPE_FILE; Index: src/domain_conf.c =================================================================== RCS file: /data/cvs/libvirt/src/domain_conf.c,v retrieving revision 1.39 diff -u -p -r1.39 domain_conf.c --- src/domain_conf.c 21 Nov 2008 11:42:51 -0000 1.39 +++ src/domain_conf.c 27 Nov 2008 12:15:06 -0000 @@ -546,6 +546,14 @@ virDomainDiskDefParseXML(virConnectPtr c source = virXMLPropString(cur, "file"); else source = virXMLPropString(cur, "dev"); + + /* People sometimes pass a bogus '' source path + when they mean to omit the source element + completely. eg CDROM without media. This is + just a little compatability check to help + those broken apps */ + if (source && STREQ(source, "")) + VIR_FREE(source); } else if ((target == NULL) && (xmlStrEqual(cur->name, BAD_CAST "target"))) { target = virXMLPropString(cur, "dev"); Index: tests/xmconfigtest.c =================================================================== RCS file: /data/cvs/libvirt/tests/xmconfigtest.c,v retrieving revision 1.24 diff -u -p -r1.24 xmconfigtest.c --- tests/xmconfigtest.c 24 Nov 2008 19:23:39 -0000 1.24 +++ tests/xmconfigtest.c 27 Nov 2008 12:15:13 -0000 @@ -229,6 +229,7 @@ mymain(int argc, char **argv) DO_TEST("fullvirt-sound", 2); DO_TEST("escape-paths", 2); + DO_TEST("no-source-cdrom", 2); virCapabilitiesFree(caps); Index: tests/xmconfigdata/test-no-source-cdrom.cfg =================================================================== RCS file: tests/xmconfigdata/test-no-source-cdrom.cfg diff -N tests/xmconfigdata/test-no-source-cdrom.cfg --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-no-source-cdrom.cfg 27 Nov 2008 12:15:27 -0000 @@ -0,0 +1,23 @@ +name = "test" +uuid = "cc2315e7-d26a-307a-438c-6d188ec4c09c" +maxmem = 382 +memory = 350 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "c" +pae = 1 +acpi = 1 +apic = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "destroy" +on_crash = "destroy" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +disk = [ "phy:/dev/sda8,hda,w", ",hdc:cdrom,r" ] +vif = [ "mac=00:16:3e:0a:7b:39,bridge=xenbr0,type=ioemu" ] +parallel = "none" +serial = "pty" Index: tests/xmconfigdata/test-no-source-cdrom.xml =================================================================== RCS file: tests/xmconfigdata/test-no-source-cdrom.xml diff -N tests/xmconfigdata/test-no-source-cdrom.xml --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ tests/xmconfigdata/test-no-source-cdrom.xml 27 Nov 2008 12:15:27 -0000 @@ -0,0 +1,46 @@ +<domain type='xen'> + <name>test</name> + <uuid>cc2315e7-d26a-307a-438c-6d188ec4c09c</uuid> + <memory>391168</memory> + <currentMemory>358400</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='xenfv'>hvm</type> + <loader>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/sda8'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='phy'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:0a:7b:39'/> + <source bridge='xenbr0'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes'/> + </devices> +</domain> -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|