[libvirt] [PATCH v2 4/9] hostdev: Add virHostdevOnlyDetachPCIDevice()

[John Ferlan]

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