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;
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list