[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] PATCH: Fix XM driver handling of disks without source file



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]