[libvirt] [PATCH 05/22] 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 | 8 +++----- src/util/virpci.c | 26 ++++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6efec74..20edf45 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10274,7 +10274,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, virPCIDevicePtr pci; unsigned domain, bus, slot, function; int ret = -1; - bool in_inactive_list = false; virCheckFlags(0, -1); @@ -10299,18 +10298,17 @@ 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); - if (in_inactive_list) - virPCIDeviceFree(pci); + virPCIDeviceFree(pci); return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 1108ef2..2f4032f 100644 --- 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; } -- 1.7.11.7