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

[Shivaprasad bhat]

Thanks for the comments Andrea.

On Fri, Oct 30, 2015 at 8:27 PM, Andrea Bolognani <abologna at redhat.com> wrote:
> 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.
>

Sure.. Will do that.

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

Its used in P3 as well in virHostdevPCINodeDeviceReAttach().
I want to keep that function as simple as it is now.
 And as you pointed out, i am using it in P7 too.Hope its okay now.

> 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