Thanks for the comments Andrea. On Fri, Oct 30, 2015 at 8:27 PM, Andrea Bolognani <abologna@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@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