pci: make virPCIDeviceDetach consistent in behavior

virPCIDeviceDetach would previously sometimes consume the input device
object (to put it on the inactive list) and sometimes not. Avoiding
memory leaks required checking beforehand to see if the device was
already on the list, and freeing the device object in the caller only
if there wasn't already an identical object on the inactive list.

This patch makes it consistent - virPCIDeviceDetach will *never*
consume the input virPCIDevice object; if it needs to put one on the
inactive list, it will create a copy and put *that* on the list. This
way the caller knows that it is always their responsibility to free
the device object they created.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2533ec84d1850021d4f21414a1b416b0a7c789ab..efee62f40e15c006fc1966aa5f7b7c7234412053 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10578,7 +10578,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
     virPCIDevicePtr pci = NULL;
     unsigned domain, bus, slot, function;
     int ret = -1;
-    bool in_inactive_list = true;
     virNodeDeviceDefPtr def = NULL;
     char *xml = NULL;
 
@@ -10616,19 +10615,18 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
 
     virObjectLock(driver->activePciHostdevs);
     virObjectLock(driver->inactivePciHostdevs);
-    in_inactive_list = virPCIDeviceListFind(driver->inactivePciHostdevs, pci);
 
     if (virPCIDeviceDetach(pci, driver->activePciHostdevs,
-                           driver->inactivePciHostdevs, NULL) < 0)
+                           driver->inactivePciHostdevs, NULL) < 0) {
         goto out;
+    }
 
     ret = 0;
 out:
     virObjectUnlock(driver->inactivePciHostdevs);
     virObjectUnlock(driver->activePciHostdevs);
 cleanup:
-    if (in_inactive_list)
-        virPCIDeviceFree(pci);
+    virPCIDeviceFree(pci);
     virNodeDeviceDefFree(def);
     VIR_FREE(xml);
     return ret;

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 1108ef2bd533a111a29a054f57626f7cdfd87817..2f4032f516f568130e543c435a617f085b62cc03 100644 (file)
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1217,6 +1217,24 @@ cleanup:
     return result;
 }
 
+/* virPCIDeviceDetach:
+ *
+ * Detach this device from the host driver, attach it to the stub
+ * driver (previously set with virPCIDeviceSetStubDriver(), and add *a
+ * copy* of the object to the inactiveDevs list (if provided). This
+ * function will *never* consume dev, so the caller should free it.
+ *
+ * Returns 0 on success, -1 on failure (will fail if the device is
+ * already in the activeDevs list, but will be a NOP if the device is
+ * already bound to the stub).
+ *
+ * GENERAL NOTE: activeDevs should be a list of all PCI devices
+ * currently in use by a domain. inactiveDevs is a list of all PCI
+ * devices that libvirt has detached from the host driver + attached
+ * to the stub driver, but hasn't yet assigned to a domain. Any device
+ * that is still attached to its host driver should not be on either
+ * list.
+ */
 int
 virPCIDeviceDetach(virPCIDevicePtr dev,
                    virPCIDeviceList *activeDevs,
@@ -1241,9 +1259,13 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
     if (virPCIDeviceBindToStub(dev, driver) < 0)
         return -1;
 
-    /* Add the dev into list inactiveDevs */
+    /* Add *a copy of* the dev into list inactiveDevs, if
+     * it's not already there.
+     */
     if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) {
-        if (virPCIDeviceListAdd(inactiveDevs, dev) < 0)
+        virPCIDevicePtr copy = virPCIDeviceCopy(dev);
+
+        if ((!copy) || virPCIDeviceListAdd(inactiveDevs, copy) < 0)
             return -1;
     }