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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list