qemu: don't 'remove' hostdev objects from domain if operation fails

There were certain paths through the hostdev detach code that could
lead to the lower level function failing (and not removing the object
from the domain's hostdevs list), but the higher level function
free'ing the hostdev object anyway. This would leave a stale
hostdevdef pointer in the list, which would surely cause a problem
eventually.

This patch relocates virDomainHostdevRemove from the lower level
functions qemuDomainDetachThisHostDevice and
qemuDomainDetachHostPciDevice, to their caller
qemuDomainDetachThisHostDevice, placing it just before the call to
virDomainHostdevDefFree. This makes it easy to verify that either both
operations are done, or neither.

NB: The "dangling pointer" part of this problem was introduced in
commit 13d5a6, so it is not present in libvirt versions prior to
0.9.9. Earlier versions would return failure in certain cases even
though the the device object was removed/deleted, but the removal and
deletion operations would always both happen or neither.

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index c5ed9f76f579829f1ac05d5488d67061b78a3378..22d0231a925e9752b123b24b96fe768410496a95 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1852,8 +1852,7 @@ cleanup:
 static int
 qemuDomainDetachHostPciDevice(struct qemud_driver *driver,
                               virDomainObjPtr vm,
-                              virDomainHostdevDefPtr detach,
-                              int idx)
+                              virDomainHostdevDefPtr detach)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virDomainHostdevSubsysPtr subsys = &detach->source.subsys;
@@ -1914,15 +1913,13 @@ qemuDomainDetachHostPciDevice(struct qemud_driver *driver,
                                         detach->info->addr.pci.slot) < 0)
         VIR_WARN("Unable to release PCI address on host device");
 
-    virDomainHostdevRemove(vm->def, idx);
     return ret;
 }
 
 static int
 qemuDomainDetachHostUsbDevice(struct qemud_driver *driver,
                               virDomainObjPtr vm,
-                              virDomainHostdevDefPtr detach,
-                              int idx)
+                              virDomainHostdevDefPtr detach)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virDomainHostdevSubsysPtr subsys = &detach->source.subsys;
@@ -1956,8 +1953,6 @@ qemuDomainDetachHostUsbDevice(struct qemud_driver *driver,
         VIR_WARN("Unable to find device %03d.%03d in list of used USB devices",
                  subsys->u.usb.bus, subsys->u.usb.device);
     }
-
-    virDomainHostdevRemove(vm->def, idx);
     return ret;
 }
 
@@ -1987,10 +1982,10 @@ int qemuDomainDetachThisHostDevice(struct qemud_driver *driver,
 
     switch (detach->source.subsys.type) {
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
-        ret = qemuDomainDetachHostPciDevice(driver, vm, detach, idx);
-       break;
+        ret = qemuDomainDetachHostPciDevice(driver, vm, detach);
+        break;
     case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
-        ret = qemuDomainDetachHostUsbDevice(driver, vm, detach, idx);
+        ret = qemuDomainDetachHostUsbDevice(driver, vm, detach);
         break;
     default:
         qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1999,13 +1994,14 @@ int qemuDomainDetachThisHostDevice(struct qemud_driver *driver,
         return -1;
     }
 
-    if (ret == 0 &&
-        virSecurityManagerRestoreHostdevLabel(driver->securityManager,
-                                              vm->def, detach) < 0) {
-        VIR_WARN("Failed to restore host device labelling");
+    if (!ret) {
+        if (virSecurityManagerRestoreHostdevLabel(driver->securityManager,
+                                                  vm->def, detach) < 0) {
+            VIR_WARN("Failed to restore host device labelling");
+        }
+        virDomainHostdevRemove(vm->def, idx);
+        virDomainHostdevDefFree(detach);
     }
-
-    virDomainHostdevDefFree(detach);
     return ret;
 }