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

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(a)linux.vnet.ibm.com&gt; > --- > 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