[libvirt] PATCH: Don't blindly reorder disk drives

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

 



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
---
 src/domain_conf.c        |   68 ++++++++++++++++++++++++++++++++++-----------
 src/domain_conf.h        |    7 +++--
 src/libvirt_private.syms |    3 +-
 src/qemu_driver.c        |   30 +++++++++++---------
 src/xend_internal.c      |    2 -
 src/xm_internal.c        |    7 +----
 6 files changed, 74 insertions(+), 43 deletions(-)

Daniel


diff --git a/src/domain_conf.c b/src/domain_conf.c
index bad53f7..29a02c1 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -627,19 +627,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
  */
@@ -2329,14 +2318,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,
@@ -2643,8 +2679,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 44302be..c710986 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -683,9 +683,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 23fa01b..32961ca 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -80,7 +80,8 @@ virDomainDeviceTypeToString;
 virDomainDiskBusTypeToString;
 virDomainDiskDefFree;
 virDomainDiskDeviceTypeToString;
-virDomainDiskQSort;
+virDomainDiskInsert;
+virDomainDiskInsertPreAlloced;
 virDomainFindByID;
 virDomainFindByName;
 virDomainFindByUUID;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 4b1aeea..3ff9f9c 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -5030,9 +5030,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;
 }
@@ -5090,9 +5088,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);
@@ -5626,17 +5622,19 @@ try_command:
     }
 
     if (vm->def->ndisks > 1) {
-        vm->def->disks[i] = vm->def->disks[--vm->def->ndisks];
+        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) {
-            virReportOOMError(conn);
-            goto cleanup;
+            /* ignore, harmless */
         }
-        qsort(vm->def->disks, vm->def->ndisks, sizeof(*vm->def->disks),
-              virDomainDiskQSort);
     } else {
         VIR_FREE(vm->def->disks[0]);
         vm->def->ndisks = 0;
     }
+    virDomainDiskDefFree(detach);
     ret = 0;
 
 cleanup:
@@ -5725,15 +5723,19 @@ qemudDomainDetachNetDevice(virConnectPtr conn,
     DEBUG("%s: host_net_remove reply: %s", vm->def->name,  reply);
 
     if (vm->def->nnets > 1) {
-        vm->def->nets[i] = vm->def->nets[--vm->def->nnets];
+        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) {
-            virReportOOMError(conn);
-            goto cleanup;
+            /* ignore, harmless */
         }
     } else {
         VIR_FREE(vm->def->nets[0]);
         vm->def->nnets = 0;
     }
+    virDomainNetDefFree(detach);
     ret = 0;
 
 cleanup:
diff --git a/src/xend_internal.c b/src/xend_internal.c
index 7bcee7d..99847b0 100644
--- 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 71b852e..dd44ce5 100644
--- 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;
 
-- 
1.6.2.5


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

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]