Thanks for the comments Andrea.
On Fri, Oct 30, 2015 at 8:27 PM, Andrea Bolognani <abologna@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list