Don't blindly reorder disk drives

Calling qsort() on the disks array causes disk to be
unneccessarily re-ordered, potentially breaking the
ability to boot if the boot disk gets moved later in
the list. The new algorithm will insert a new disk as
far to the end of the list as possible, while being
ordered correctly wrt other disks on the same bus.

* src/domain_conf.c, src/domain_conf.h: Remove disk sorting
  routines. Add API to insert a disk into existing list at
  the optimal position, without resorting disks
* src/libvirt_private.syms: Export virDomainDiskInsert
* src/xend_internal.c, src/xm_internal.c: Remove calls to
  qsort, use virDomainDiskInsert instead.
* src/qemu_driver.c: Remove calls to qsort, use virDoaminDiskInsert
  instead. Fix reordering bugs when hotunplugging disks and
  networks. Fix memory leak in disk/net unplug

diff --git a/src/domain_conf.c b/src/domain_conf.c
index 46acf5e9ac6a47382b48fa038b8253f09bb3afcf..ade3eb40b69a09f817079a2ad12d80da118ddac4 100644 (file)
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -631,19 +631,8 @@ void virDomainRemoveInactive(virDomainObjListPtr doms,
     }
 
 }
-#endif /* ! PROXY */
-
-
-int virDomainDiskCompare(virDomainDiskDefPtr a,
-                         virDomainDiskDefPtr b) {
-    if (a->bus == b->bus)
-        return virDiskNameToIndex(a->dst) - virDiskNameToIndex(b->dst);
-    else
-        return a->bus - b->bus;
-}
 
 
-#ifndef PROXY
 /* Parse the XML definition for a disk
  * @param node XML nodeset to parse for disk definition
  */
@@ -2343,14 +2332,61 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virConnectPtr conn,
 }
 #endif
 
-int virDomainDiskQSort(const void *a, const void *b)
+
+int virDomainDiskInsert(virDomainDefPtr def,
+                        virDomainDiskDefPtr disk)
 {
-    const virDomainDiskDefPtr *da = a;
-    const virDomainDiskDefPtr *db = b;
 
-    return virDomainDiskCompare(*da, *db);
+    if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0)
+        return -1;
+
+    virDomainDiskInsertPreAlloced(def, disk);
+
+    return 0;
 }
 
+void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
+                                   virDomainDiskDefPtr disk)
+{
+    int i;
+    /* Tenatively plan to insert disk at the end. */
+    int insertAt = -1;
+
+    /* Then work backwards looking for disks on
+     * the same bus. If we find a disk with a drive
+     * index greater than the new one, insert at
+     * that position
+     */
+    for (i = (def->ndisks - 1) ; i >= 0 ; i--) {
+        /* If bus matches and current disk is after
+         * new disk, then new disk should go here */
+        if (def->disks[i]->bus == disk->bus &&
+            (virDiskNameToIndex(def->disks[i]->dst) >
+             virDiskNameToIndex(disk->dst))) {
+            insertAt = i;
+        } else if (def->disks[i]->bus == disk->bus &&
+                   insertAt == -1) {
+            /* Last disk with match bus is before the
+             * new disk, then put new disk just after
+             */
+            insertAt = i + 1;
+        }
+    }
+
+    /* No disks with this bus yet, so put at end of list */
+    if (insertAt == -1)
+        insertAt = def->ndisks;
+
+    if (insertAt < def->ndisks)
+        memmove(def->disks + insertAt + 1,
+                def->disks + insertAt,
+                (sizeof(def->disks[0]) * (def->ndisks-insertAt)));
+
+    def->disks[insertAt] = disk;
+    def->ndisks++;
+}
+
+
 #ifndef PROXY
 static char *virDomainDefDefaultEmulator(virConnectPtr conn,
                                          virDomainDefPtr def,
@@ -2657,8 +2693,6 @@ static virDomainDefPtr virDomainDefParseXML(virConnectPtr conn,
 
         def->disks[def->ndisks++] = disk;
     }
-    qsort(def->disks, def->ndisks, sizeof(*def->disks),
-          virDomainDiskQSort);
     VIR_FREE(nodes);
 
     /* analysis of the filesystems */

diff --git a/src/domain_conf.h b/src/domain_conf.h
index 30e2dea98c620f4706043e32a1347c9ad723bb49..0854a0d3bab5025bcf72eeb972604bca5714d183 100644 (file)
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -685,9 +685,10 @@ char *virDomainCpuSetFormat(virConnectPtr conn,
                             char *cpuset,
                             int maxcpu);
 
-int virDomainDiskQSort(const void *a, const void *b);
-int virDomainDiskCompare(virDomainDiskDefPtr a,
-                         virDomainDiskDefPtr b);
+int virDomainDiskInsert(virDomainDefPtr def,
+                        virDomainDiskDefPtr disk);
+void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
+                                   virDomainDiskDefPtr disk);
 
 int virDomainSaveXML(virConnectPtr conn,
                      const char *configDir,

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8db9fcd96d9a2c38398af68076012217e1c8ae6d..ead3390e7b63e44f982fa38ac2eb497ae6e21400 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -82,7 +82,8 @@ virDomainDeviceTypeToString;
 virDomainDiskBusTypeToString;
 virDomainDiskDefFree;
 virDomainDiskDeviceTypeToString;
-virDomainDiskQSort;
+virDomainDiskInsert;
+virDomainDiskInsertPreAlloced;
 virDomainFindByID;
 virDomainFindByName;
 virDomainFindByUUID;

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index d45d33a08550026f23d9f23c8038e044188112c6..c095a2966db8a1955dfbd78bcfec617a5a345145 100644 (file)
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -5169,9 +5169,7 @@ try_command:
     dev->data.disk->pci_addr.bus    = bus;
     dev->data.disk->pci_addr.slot   = slot;
 
-    vm->def->disks[vm->def->ndisks++] = dev->data.disk;
-    qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks),
-          virDomainDiskQSort);
+    virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
 
     return 0;
 }
@@ -5229,9 +5227,7 @@ static int qemudDomainAttachUsbMassstorageDevice(virConnectPtr conn,
         return -1;
     }
 
-    vm->def->disks[vm->def->ndisks++] = dev->data.disk;
-    qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks),
-          virDomainDiskQSort);
+    virDomainDiskInsertPreAlloced(vm->def, dev->data.disk);
 
     VIR_FREE(reply);
     VIR_FREE(cmd);
@@ -5772,16 +5768,20 @@ try_command:
         goto cleanup;
     }
 
-    if (i != --vm->def->ndisks)
-        memmove(&vm->def->disks[i],
-                &vm->def->disks[i+1],
-                sizeof(*vm->def->disks) * (vm->def->ndisks-i));
-    if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks) < 0) {
-        virReportOOMError(conn);
-        goto cleanup;
+    if (vm->def->ndisks > 1) {
+        memmove(vm->def->disks + i,
+                vm->def->disks + i + 1,
+                sizeof(*vm->def->disks) *
+                (vm->def->ndisks - (i + 1)));
+        vm->def->ndisks--;
+        if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks) < 0) {
+            /* ignore, harmless */
+        }
+    } else {
+        VIR_FREE(vm->def->disks[0]);
+        vm->def->ndisks = 0;
     }
-    qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks),
-          virDomainDiskQSort);
+    virDomainDiskDefFree(detach);
 
     ret = 0;
 
@@ -5870,14 +5870,20 @@ qemudDomainDetachNetDevice(virConnectPtr conn,
 
     DEBUG("%s: host_net_remove reply: %s", vm->def->name,  reply);
 
-    if (i != --vm->def->nnets)
-        memmove(&vm->def->nets[i],
-                &vm->def->nets[i+1],
-                sizeof(*vm->def->nets) * (vm->def->nnets-i));
-    if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets) < 0) {
-        virReportOOMError(conn);
-        goto cleanup;
+    if (vm->def->nnets > 1) {
+        memmove(vm->def->nets + i,
+                vm->def->nets + i + 1,
+                sizeof(*vm->def->nets) *
+                (vm->def->nnets - (i + 1)));
+        vm->def->nnets--;
+        if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets) < 0) {
+            /* ignore, harmless */
+        }
+    } else {
+        VIR_FREE(vm->def->nets[0]);
+        vm->def->nnets = 0;
     }
+    virDomainNetDefFree(detach);
 
     ret = 0;
 

diff --git a/src/xend_internal.c b/src/xend_internal.c
index 7bcee7d971f73286f4266ac5f130de6669948171..99847b014c5dbbe096cef4ede1f2ab09db2c779e 100644 (file)
--- a/src/xend_internal.c
+++ b/src/xend_internal.c
@@ -2540,8 +2540,6 @@ xenDaemonParseSxpr(virConnectPtr conn,
             }
         }
     }
-    qsort(def->disks, def->ndisks, sizeof(*def->disks),
-          virDomainDiskQSort);
 
     /* in case of HVM we have USB device emulation */
     if (hvm &&

diff --git a/src/xm_internal.c b/src/xm_internal.c
index 71b852e048517c2b9178f9abe16d0cc998e8bec8..dd44ce5f7d39efe25e213dd158fe55fa9e120458 100644 (file)
--- a/src/xm_internal.c
+++ b/src/xm_internal.c
@@ -990,8 +990,6 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
             disk = NULL;
         }
     }
-    qsort(def->disks, def->ndisks, sizeof(*def->disks),
-          virDomainDiskQSort);
 
     list = virConfGetValue(conf, "vif");
     if (list && list->type == VIR_CONF_LIST) {
@@ -2839,14 +2837,11 @@ xenXMDomainAttachDevice(virDomainPtr domain, const char *xml) {
     switch (dev->type) {
     case VIR_DOMAIN_DEVICE_DISK:
     {
-        if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) {
+        if (virDomainDiskInsert(def, dev->data.disk) < 0) {
             virReportOOMError(domain->conn);
             goto cleanup;
         }
-        def->disks[def->ndisks++] = dev->data.disk;
         dev->data.disk = NULL;
-        qsort(def->disks, def->ndisks, sizeof(*def->disks),
-              virDomainDiskQSort);
     }
     break;
 

diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr b/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr
index 019fe47a2e200a0167fe392b937cd852cea69320..bd719586abf9eeb7e5b79b89f2254e01cabcda20 100644 (file)
--- a/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr
+++ b/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr
@@ -1 +1 @@
-(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(acpi 1)(vnc 1)(keymap ja)))(device (vbd (dev 'hdc:cdrom')(uname 'file:/root/boot.iso')(mode 'r')))(device (vbd (dev 'hda:disk')(uname 'file:/root/foo.img')(mode 'w')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))
+(domain (domid 3)(name 'fvtest')(memory 400)(maxmem 400)(vcpus 1)(uuid 'b5d70dd275cdaca517769660b059d8bc')(on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')(image (hvm (kernel '/usr/lib/xen/boot/hvmloader')(device_model '/usr/lib64/xen/bin/qemu-dm')(boot c)(acpi 1)(vnc 1)(keymap ja)))(device (vbd (dev 'hda:disk')(uname 'file:/root/foo.img')(mode 'w')))(device (vbd (dev 'hdc:cdrom')(uname 'file:/root/boot.iso')(mode 'r')))(device (vif (mac '00:16:3e:1b:b1:47')(bridge 'xenbr0')(script 'vif-bridge')(type ioemu))))

diff --git a/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml b/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml
index 8ccbf5fbbaf7ebcc422dab657f40db4edf3573c5..398506ed11c798b4225c3346cd43a49782effcc8 100644 (file)
--- a/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml
+++ b/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml
@@ -21,15 +21,15 @@
       <mac address='00:16:3e:1b:b1:47'/>
       <script path='vif-bridge'/>
     </interface>
+    <disk type='file'>
+      <source file='/root/foo.img'/>
+      <target dev='ioemu:hda'/>
+    </disk>
     <disk type='file' device='cdrom'>
       <source file='/root/boot.iso'/>
       <target dev='hdc'/>
       <readonly/>
     </disk>
-    <disk type='file'>
-      <source file='/root/foo.img'/>
-      <target dev='ioemu:hda'/>
-    </disk>
     <graphics type='vnc' port='-1' keymap='ja'/>
   </devices>
 </domain>

diff --git a/tests/xml2sexprdata/xml2sexpr-fv.xml b/tests/xml2sexprdata/xml2sexpr-fv.xml
index b094c044929e38459c479dcecc462ce1c09dddea..378e5a73445afe276990e4b9d7c7842abe6255da 100644 (file)
--- a/tests/xml2sexprdata/xml2sexpr-fv.xml
+++ b/tests/xml2sexprdata/xml2sexpr-fv.xml
@@ -21,15 +21,15 @@
       <mac address='00:16:3e:1b:b1:47'/>
       <script path='vif-bridge'/>
     </interface>
+    <disk type='file'>
+      <source file='/root/foo.img'/>
+      <target dev='ioemu:hda'/>
+    </disk>
     <disk type='file' device='cdrom'>
       <source file='/root/boot.iso'/>
       <target dev='hdc'/>
       <readonly/>
     </disk>
-    <disk type='file'>
-      <source file='/root/foo.img'/>
-      <target dev='ioemu:hda'/>
-    </disk>
     <graphics type='vnc' port='5917' keymap='ja'/>
   </devices>
 </domain>