5:58 p.m.
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;