[libvirt] [PATCH 19/24] hostdev: Add more comments

[John Ferlan]

On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> These comments explain the difference between a virPCIDevice
> instance used for lookups and an actual device instance; some
> information is also provided for specific uses.
> ---
>  src/util/virhostdev.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 10d1c1a..a431f0a 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -60,6 +60,27 @@ struct virHostdevIsPCINodeDeviceUsedData {
>      const bool usesVFIO;
>  };
>  
> +/* This module makes heavy use of bookkeeping lists, contained inside a
                                                      ^
no comma.

> + * virHostdevManager instance, to keep track of the devices' status. To make
                                ^
same

> + * it easy to spot potential ownership errors when moving devices from one
> + * list to the other, variable names should comply with the following
> + * conventions when it comes to virPCIDevice and virPCIDeviceList instances:
> + *
> + *   pci - a short-lived virPCIDevice whose purpose is usually just to look
> + *         up the actual PCI device in one of the bookkeeping lists; basically
> + *         little more than a fancy virPCIDeviceAddress
> + *
> + *   pcidevs - a list containing a bunch of the above
> + *
> + *   actual - a virPCIDevice instance that has either been retrieved from one
> + *            of the bookkeeping lists, or is intended to be added or copied
> + *            to one at some point
> + *
> + * Passing an 'actual' to a function that requires a 'pci' is fine, but the
> + * opposite is usually not true; as a rule of thumb, functions in the virpci
> + * module usually expect an 'actual'. Even with these conventions in place,
> + * adding comments to highlight ownership-related issues is recommended */
> +

s//Free beer for anyone that reads this and adheres to it. You will need
it./

>  static int virHostdevIsPCINodeDeviceUsed(virPCIDeviceAddressPtr devAddr, void *opaque)
>  {
>      virPCIDevicePtr actual;
> @@ -544,6 +565,10 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>          virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>  
>          if (virPCIDeviceGetManaged(pci)) {

Just for consistency/readability - add an extra line between the if and
comment open.

ACK

John

> +            /* We can't look up the actual device because it has not been
> +             * created yet: virPCIDeviceDetach() will insert a copy of 'pci'
> +             * into the list of inactive devices, and that copy will be the
> +             * actual device going forward */
>              VIR_DEBUG("Detaching managed PCI device %s",
>                        virPCIDeviceGetName(pci));
>              if (virPCIDeviceDetach(pci,
> @@ -564,6 +589,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>  
> +        /* We can avoid looking up the actual device here, because performing
> +         * a PCI reset on a device doesn't require any information other than
> +         * the address, which 'pci' already contains */
>          VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
>          if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
>                                mgr->inactivePCIHostdevs) < 0)
> @@ -608,6 +636,9 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr pci, actual;
>  
> +        /* We need to look up the actual device and set the information
> +         * there because 'pci' only contain address information and will
> +         * be released at the end of the function */
>          pci = virPCIDeviceListGet(pcidevs, i);
>          actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
>  
> @@ -775,6 +806,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
>          virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>          virPCIDevicePtr actual = NULL;
>  
> +        /* We need to look up the actual device, which is the one containing
> +         * information such as by which domain and driver it is used. As a
> +         * side effect, by looking it up we can also tell whether it was
> +         * really active in the first place */
>          actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci);
>          if (actual) {
>              const char *actual_drvname;
> @@ -830,6 +865,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>  
> +        /* We can avoid looking up the actual device here, because performing
> +         * a PCI reset on a device doesn't require any information other than
> +         * the address, which 'pci' already contains */
>          VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
>          if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
>                                mgr->inactivePCIHostdevs) < 0) {
>