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.

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;

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;
     }