[libvirt] PATCH: Don't blindly reorder disk drives
Daniel P. Berrange
berrange at redhat.com
Mon Aug 17 14:34:38 UTC 2009
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 :|
More information about the libvir-list
mailing list