[libvirt] [PATCH 1/2] hostdev: Detect untracked inactive devices

[John Ferlan]

On 03/17/2016 01:24 PM, Andrea Bolognani wrote:
> Unmanaged devices are attached to guests in two steps: first,
> the device is detached from the host and marked as inactive;
> subsequently, it is marked as active and attached to the guest.
> 
> If the daemon is restarted between these two operations, we lose
> track of the inactive device.
> 
> Steps 5 and 6 of virHostdevPreparePCIDevices() already subtly
> take care of this situation, but some planned changes will make
> it so that's no longer the case. Plus, explicit is always better
> than implicit.
> ---
>  src/util/virhostdev.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index e0d6465..7204bd7 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -560,7 +560,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>          }
>      }
>  
> -    /* Step 2: detach managed devices (i.e. bind to appropriate stub driver) */
> +    /* Step 2: detach managed devices and make sure unmanaged devices
> +     *         have already been taken care of */
>      for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>          virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>  
> @@ -577,8 +578,48 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>                                     mgr->inactivePCIHostdevs) < 0)
>                  goto reattachdevs;
>          } else {
> -            VIR_DEBUG("Not detaching unmanaged PCI device %s",
> -                      virPCIDeviceGetName(pci));
> +            char *driverPath;
> +            char *driverName;
> +            int stub;

stub = -1;

> +
> +            /* Unmanaged devices should already have been marked as
> +             * inactive: if that's the case, we can simply move on */
> +            if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) {
> +                VIR_DEBUG("Not detaching unmanaged PCI device %s",
> +                          virPCIDeviceGetName(pci));
> +                continue;
> +            }
> +
> +            /* If that's not the case, though, it might be because the
> +             * daemon has been restarted, causing us to lose track of the
> +             * device. Try and recover by marking the device as inactive
> +             * if it happens to be bound to a known stub driver.
> +             *

Just to make it clearer (for me) - this is because using the node device
API virHostdevPCINodeDeviceDetach it's possible that somewhere between
virPCIDeviceBindToStub and the virPCIDeviceListAddCopy we had a daemon
restart causing us to lose track of the device, so this code tries to
detect that the BindToStub was done and complete the task.

Of course it's also possible that BindToStub was "in process" when we
failed, but I'm not sure *how* to detect that...  I suppose though all
that matters is we had a completed BindToStub.

> +             * FIXME Get rid of this once a proper way to keep track of
> +             *       information about active / inactive device across
> +             *       daemon restarts has been implemented */
> +
> +            if (virPCIDeviceGetDriverPathAndName(pci,
> +                                                 &driverPath, &driverName) < 0)
> +                goto reattachdevs;

I note that virPCIDeviceUnbindFromStub can return 0, but driverName ==
NULL. If driverName == NULL, then it declares it's not bound to a known
stub.  However, for our purposes, it more than likely means the
BindToStub may not have completed successfully. But still we only *care*
that it did.

> +
> +            stub = virPCIStubDriverTypeFromString(driverName);

So, I think this becomes,

    if (driverName)
        stub = virPCIStubDriverTypeFromString(driverName);

> +
> +            VIR_FREE(driverPath);
> +            VIR_FREE(driverName);
> +
> +            if (stub >= 0 &&
> +                stub != VIR_PCI_STUB_DRIVER_NONE) {

Strange check since VIR_PCI_STUB_DRIVER_NONE = 0

   if (stub >= 0 && stub != 0)

a change to seems to be what you're after

   if (stub > VIR_PCI_STUB_DRIVER_NONE &&
       stub < VIR_PCI_STUB_DRIVER_LAST)

> +
> +                /* The device is bound to a known stub driver: store this
> +                 * information and add a copy to the inactive list */
> +                virPCIDeviceSetStubDriver(pci, stub);
> +
> +                VIR_DEBUG("Adding PCI device %s to inactive list",
> +                          virPCIDeviceGetName(pci));
> +                if (virPCIDeviceListAddCopy(mgr->inactivePCIHostdevs, pci) < 0)
> +                    goto reattachdevs;
> +            }

I think perhaps patch 2 should be merged into here - I was kind of
wondering what the "else" condition would be...  Until I looked at patch 2.

ACK with the adjustments - including the merge; otherwise, we fall into
Step 3 still.

John

>          }
>      }
>  
>