On 01/25/2016 11:20 AM, Andrea Bolognani wrote:
This function mirrors virHostdevOnlyDetachPCIDevice().
The handling of active and inactive devices is updated and made more explicit, which means virHostdevPreparePCIDevices() has to be updated as well. --- src/util/virhostdev.c | 87 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 26 deletions(-)
I think parts of patch 7 to adjust comments should just be inlined here
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f40d636..6f14574 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -594,6 +594,56 @@ virHostdevOnlyReattachPCIDevice(virHostdevManagerPtr mgr, return ret; }
+/** + * virHostdevOnlyDetachPCIDevice:
Similar to 1/9 comments, you could drop the "Only"
+ * @mgr: hostdev manager + * @pci: PCI device to be detached
... Copy of a 'hostdev' - expected to be added to inactivelist...
+ * @skipUnmanaged: whether to skip unmanaged devices + * + * Detach a PCI device from the host. + * + * This function only performs the base detach steps that are required + * regardless of whether the device is being attached to a domain or + * is simply being detached from the host for later use. + * + * The PCI related parts of @mgr (inactivePCIHostdevs, activePCIHostdevs) + * must have been locked beforehand using virObjectLock(). + * + * Returns: 0 on success, <0 on failure + */ +static int +virHostdevOnlyDetachPCIDevice(virHostdevManagerPtr mgr, + virPCIDevicePtr pci, + bool skipUnmanaged) +{ + int ret = -1; + + /* Skip unmanaged devices if asked to do so */ + if (!virPCIDeviceGetManaged(pci) && skipUnmanaged) { + VIR_DEBUG("Not detaching unmanaged PCI device %s", + virPCIDeviceGetName(pci)); + ret = 0; + goto out; + } + + VIR_DEBUG("Detaching PCI device %s", virPCIDeviceGetName(pci)); + if (virPCIDeviceDetach(pci, + mgr->activePCIHostdevs, + mgr->inactivePCIHostdevs) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to detach PCI device %s: %s"), + virPCIDeviceGetName(pci), + err ? err->message : _("unknown error")); + virResetError(err); + goto out; + } + + ret = 0; + + out: + return ret; +} + int virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -664,17 +714,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- if (virPCIDeviceGetManaged(dev)) { - VIR_DEBUG("Detaching managed PCI device %s", - virPCIDeviceGetName(dev)); - if (virPCIDeviceDetach(dev, - hostdev_mgr->activePCIHostdevs, - hostdev_mgr->inactivePCIHostdevs) < 0) - goto reattachdevs; - } else { - VIR_DEBUG("Not detaching unmanaged PCI device %s", - virPCIDeviceGetName(dev)); - } + if (virHostdevOnlyDetachPCIDevice(hostdev_mgr, dev, true) < 0) + goto reattachdevs; }
/* At this point, all devices are attached to the stub driver and have @@ -704,23 +745,21 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, last_processed_hostdev_vf = i; }
- /* Loop 5: Now mark all the devices as active */ + /* Step 5: move all devices from the inactive list to the active list */
I know patch 7 adjusts other comments, but things are still described as loops in other comments - probably should follow... Or of course, make the comment changes now because [1a & 1b]
for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); + virPCIDevicePtr actual;
- VIR_DEBUG("Adding PCI device %s to active list", + VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(dev)); - if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, dev) < 0) + if (!(actual = virPCIDeviceListSteal(hostdev_mgr->inactivePCIHostdevs, + dev))) goto inactivedevs; - } - - /* Loop 6: Now remove the devices from inactive list. */ - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
[1a] This removes Loop 6 (or Step 6), but does not renumber the following loops/steps nor does it address the comment much earlier "We have to use 9 loops here."
- virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
- VIR_DEBUG("Removing PCI device %s from inactive list", - virPCIDeviceGetName(dev)); - virPCIDeviceListDel(hostdev_mgr->inactivePCIHostdevs, dev); + VIR_DEBUG("Adding PCI device %s to active list", + virPCIDeviceGetName(actual)); + if (virPCIDeviceListAdd(hostdev_mgr->activePCIHostdevs, actual) < 0) + goto inactivedevs; }
This seems more efficient than the former code (and of course why I also suggest in 1/9 to not make a second pass).
/* Loop 7: Now set the used_by_domain of the device in @@ -771,10 +810,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virPCIDeviceFree(dev); }
- /* Loop 9: Now steal all the devices from pcidevs */
[1b] Delete another loop/step.
- while (virPCIDeviceListCount(pcidevs) > 0) - virPCIDeviceListStealIndex(pcidevs, 0); -
Hmmm.. interesting looks like this would have been a memory leak since the code ignoree the return.... Normal when *Steal is called from *ListDel, a *DeviceFree is called on the returned stolen device. John
ret = 0; goto cleanup;