On Mon, 2015-11-02 at 10:06 +0530, Shivaprasad bhat wrote:

...

...

+{ + int ret = 0; + virPCIDevicePtr pci = NULL; + + if (!(pci = virPCIDeviceNew(devAddr->domain, devAddr->bus, + devAddr->slot, devAddr->function))) + goto cleanup; + + if (STREQ_NULLABLE(pci->stubDriver, "vfio-pci")) + ret = -1;

As mentioned in the comment for Patch 1, I think this is fairly obscure: if I had not read throught the whole series, I'd assume this checks whether the device is configured to use vfio-pci as stub driver, not whether it is currently bound to it, and it would not be immediately clear to me how this check fits in a function called virPCIDeviceBoundToVFIODriver().

I think it would be much cleaner to query the driver explicitly using virPCIDeviceGetDriverPathAndName() and remove that call from virPCIDeviceNew().

The PCIDeviceNew does exactly the same of going through the sysfs and populating it. Its only code which is as though the stubDriver scan is happening inside than explicitly outside.

The Iterator today takes virPCIDeviceAddressPtr and virPCIDeviceGetDriverPathAndName for which we anyway need to call virPCIDevNew. I am just avoiding one extra function call by moving it inside virPCIDevNew

The problem I have with this approach is that, as far as I understand, before your patch dev->stubDriver is a configuration item: you set it to whatever's appropriate, and the value is later inspected in virPCIDeviceBindToStub()[1] to decide what stub driver the device should be bound to. After your patch, depending on the context, it could be either a configuration item, as before, or state, eg. the current stub driver the device is bound to, with no clear way to tell one situation from the other. I think using a single attribute to store related but different information should be avoided, as it has the potential to make the code much harder to understand, especially later on for someone who's not been involved in the implementation. Moreover you only need that value for comparison purposes in a couple of places, and by moving the code to virPCIDeviceNew() you make it so the value is loaded (by performing filesystem operations) every single time a virDevice is created, whether the value is actually used or not. Cheers. [1] Rather virPCIDeviceDetach() in reality - I have a pending patch that moves it to virPCIDeviceBindToStub() where it IMHO belongs -- Andrea Bolognani Software Engineer - Virtualization Team