From 2d6adabd53c8f1858191d521dc9b4948d1205955 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 14 Aug 2009 10:31:36 +0100 Subject: [PATCH] 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 --- src/domain_conf.c | 70 ++++++++++++++----- src/domain_conf.h | 7 +- src/libvirt_private.syms | 3 +- src/qemu_driver.c | 50 +++++++------ src/xend_internal.c | 2 - src/xm_internal.c | 7 +- tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr | 2 +- .../xml2sexprdata/xml2sexpr-fv-vncunused.xml | 8 +-- tests/xml2sexprdata/xml2sexpr-fv.xml | 8 +-- 9 files changed, 96 insertions(+), 61 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index 46acf5e9ac..ade3eb40b6 100644 --- 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) -{ - const virDomainDiskDefPtr *da = a; - const virDomainDiskDefPtr *db = b; - return virDomainDiskCompare(*da, *db); +int virDomainDiskInsert(virDomainDefPtr def, + virDomainDiskDefPtr disk) +{ + + 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 30e2dea98c..0854a0d3ba 100644 --- 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 8db9fcd96d..ead3390e7b 100644 --- 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 d45d33a085..c095a2966d 100644 --- 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 7bcee7d971..99847b014c 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 71b852e048..dd44ce5f7d 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; diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr b/tests/sexpr2xmldata/sexpr2xml-fv-v2.sexpr index 019fe47a2e..bd719586ab 100644 --- 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 8ccbf5fbba..398506ed11 100644 --- a/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml +++ b/tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml @@ -21,15 +21,15 @@