[libvirt] [PATCH 1/8] Initialize the stubDriver of pci devices if bound to a valid one

[Andrea Bolognani]

On Fri, 2015-10-30 at 04:56 +0530, Shivaprasad G Bhat wrote:
> The stubDriver name can be used to make useful decisions if readily available.
> Set it if bound to a valid one during initialisation.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
> ---
>  src/util/virpci.c |   36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 35b1459..5acf486 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1080,6 +1080,22 @@ static const char *virPCIKnownStubs[] = {
>      NULL
>  };
>  
> +static bool virPCIIsAKnownStub(char *driver)

Return type on a separate line, please.

> +{
> +    const char **stubTest;
> +    bool ret = false;
> +
> +    for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
> +        if (STREQ_NULLABLE(driver, *stubTest)) {
> +            ret = true;
> +            VIR_DEBUG("Found stub driver %s", *stubTest);
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  static int
>  virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>  {
> @@ -1087,8 +1103,6 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>      char *drvdir = NULL;
>      char *path = NULL;
>      char *driver = NULL;
> -    const char **stubTest;
> -    bool isStub = false;
>  
>      /* If the device is currently bound to one of the "well known"
>       * stub drivers, then unbind it, otherwise ignore it.
> @@ -1105,14 +1119,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
>          goto remove_slot;
>  
>      /* If the device isn't bound to a known stub, skip the unbind. */
> -    for (stubTest = virPCIKnownStubs; *stubTest != NULL; stubTest++) {
> -        if (STREQ(driver, *stubTest)) {
> -            isStub = true;
> -            VIR_DEBUG("Found stub driver %s", *stubTest);
> -            break;
> -        }
> -    }
> -    if (!isStub)
> +    if (!virPCIIsAKnownStub(driver))
>          goto remove_slot;
>  
>      if (virPCIDeviceUnbind(dev, dev->reprobe) < 0)

I would split this patch in two: everything above is just moving
an existing check into a separate function, without introducing
any functional change.

The code below, on the other hand, is changing the behavior of
virPCIDeviceNew() and as such should go in its own patch. But
see below.

> @@ -1556,6 +1563,8 @@ virPCIDeviceNew(unsigned int domain,
>      virPCIDevicePtr dev;
>      char *vendor = NULL;
>      char *product = NULL;
> +    char *drvpath = NULL;
> +    char *driver = NULL;
>  
>      if (VIR_ALLOC(dev) < 0)
>          return NULL;
> @@ -1603,9 +1612,16 @@ virPCIDeviceNew(unsigned int domain,
>          goto error;
>      }
>  
> +    if (virPCIDeviceGetDriverPathAndName(dev, &drvpath, &driver) < 0)
> +        goto cleanup;
> +
> +    if (virPCIIsAKnownStub(driver))
> +        dev->stubDriver = driver;
> +
>      VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
>  
>   cleanup:
> +    VIR_FREE(drvpath);
>      VIR_FREE(product);
>      VIR_FREE(vendor);
>      return dev;

What are you doing this for? AFAICT you're using this so you
can, in Patch 7, do

  pci = virPCIDeviceNew(...);
  if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci"))
      ...

Is that so, or is there another reason I'm missing?

If the former, I'd rather not overload the meaning of
dev->stubDriver and call virPCIDeviceGetDriverPathAndName()
explicitly in Patch 7, so that the intentions behind the
code are abundantly clear.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team