Re: [PATCH v2 3/6] pci: Introduce virPCIStubDriver enumeration

[Laine Stump <laine@xxxxxxxxx>]

On 12/18/2015 07:05 AM, Michal Privoznik wrote:
> On 17.12.2015 18:59, Andrea Bolognani wrote:
> > This replaces the virPCIKnownStubs string array that was used
> > internally for stub driver validation.
> >
> > Advantages:
> >
> >    * possible values are well-defined
> >    * typos in driver names will be detected at compile time
> >    * avoids having several copies of the same string around
> >    * no error checking required when setting / getting value
> >
> > The names used mirror those in the
> > virDomainHostdevSubsysPCIBackendType enumeration.
> > ---
> > Changes in v2:
> >
> >    * add VIR_PCI_STUB_DRIVER_NONE so we can detect when no driver has
> >      been configured for a specific device
> >    * simplify code in testVirPCIDeviceDetachFail() by not reading the
> >      driver back from the device, since we set it a few lines above
> >
> > testVirPCIDeviceDetachFail
> >   src/libvirt_private.syms |  2 ++
> >   src/libxl/libxl_driver.c |  3 +-
> >   src/qemu/qemu_driver.c   |  6 ++--
> >   src/util/virhostdev.c    | 45 +++++++++----------------
> >   src/util/virpci.c        | 86 +++++++++++++++++++++++++++---------------------
> >   src/util/virpci.h        | 17 +++++++---
> >   src/xen/xen_driver.c     |  3 +-
> >   tests/virhostdevtest.c   |  5 +--
> >   tests/virpcitest.c       | 23 ++++++-------
> >   9 files changed, 99 insertions(+), 91 deletions(-)
>
> > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> > index 4065535..f9072a4 100644
> > --- a/src/util/virhostdev.c
> > +++ b/src/util/virhostdev.c
> > @@ -237,22 +237,13 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs)
> >           }
> >   
> >           virPCIDeviceSetManaged(dev, hostdev->managed);
> > -        if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> > -            if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0) {
> > -                virObjectUnref(list);
> > -                return NULL;
> > -            }
> > -        } else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) {
> > -            if (virPCIDeviceSetStubDriver(dev, "pciback") < 0) {
> > -                virObjectUnref(list);
> > -                return NULL;
> > -            }
> > -        } else {
> > -            if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0) {
> > -                virObjectUnref(list);
> > -                return NULL;
> > -            }
> > -        }
> > +
> > +        if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO);
> > +        else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN)
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN);
> > +        else
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM);
> >       }
> >   
> >       return list;
> > @@ -574,7 +565,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
> >       for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> >           virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> >           bool strict_acs_check = !!(flags & VIR_HOSTDEV_STRICT_ACS_CHECK);
> > -        bool usesVfio = STREQ(virPCIDeviceGetStubDriver(dev), "vfio-pci");
> > +        bool usesVfio = (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_VFIO);
> I believe these braces are redundant. But do not hurt either.
>
> >           struct virHostdevIsPCINodeDeviceUsedData data = {hostdev_mgr, dom_name,
> >                                                            usesVfio};
> >   
> > @@ -745,7 +736,7 @@ virHostdevReattachPCIDevice(virPCIDevicePtr dev, virHostdevManagerPtr mgr)
> >       }
> >   
> >       /* Wait for device cleanup if it is qemu/kvm */
> > -    if (STREQ(virPCIDeviceGetStubDriver(dev), "pci-stub")) {
> > +    if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
> >           int retries = 100;
> >           while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
> >                  && retries) {
> > @@ -913,19 +904,15 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr,
> >               goto cleanup;
> >   
> >           virPCIDeviceSetManaged(dev, hostdev->managed);
> > -        if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> > -            if (virPCIDeviceSetStubDriver(dev, "vfio-pci") < 0)
> > -                goto cleanup;
> > -        } else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN) {
> > -            if (virPCIDeviceSetStubDriver(dev, "pciback") < 0)
> > -                goto cleanup;
> > -        } else {
> > -            if (virPCIDeviceSetStubDriver(dev, "pci-stub") < 0)
> > -                goto cleanup;
> > -
> > -        }
> >           virPCIDeviceSetUsedBy(dev, drv_name, dom_name);
> >   
> > +        if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_VFIO);
> > +        else if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN)
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_XEN);
> > +        else
> > +            virPCIDeviceSetStubDriver(dev, VIR_PCI_STUB_DRIVER_KVM);
> > +
> >           /* Setup the original states for the PCI device */
> >           virPCIDeviceSetUnbindFromStub(dev, hostdev->origstates.states.pci.unbind_from_stub);
> >           virPCIDeviceSetRemoveSlot(dev, hostdev->origstates.states.pci.remove_slot);
> > diff --git a/src/util/virpci.c b/src/util/virpci.c
> > index 21eacf5..aec7b69 100644
> > --- a/src/util/virpci.c
> > +++ b/src/util/virpci.c
> > @@ -55,6 +55,13 @@ VIR_LOG_INIT("util.pci");
> >   VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST,
> >                 "", "2.5", "5", "8")
> >   
> > +VIR_ENUM_IMPL(virPCIStubDriver, VIR_PCI_STUB_DRIVER_LAST,
> > +              "none",
> > +              "pciback", /* XEN */
> > +              "pci-stub", /* KVM */
> > +              "vfio-pci", /* VFIO */
> > +);
> > +
> >   struct _virPCIDevice {
> >       virPCIDeviceAddress address;
> >   
> > @@ -71,7 +78,8 @@ struct _virPCIDevice {
> >       bool          has_flr;
> >       bool          has_pm_reset;
> >       bool          managed;
> > -    char          *stubDriver;
> > +
> > +    virPCIStubDriver stubDriver;
> >   
> >       /* used by reattach function */
> >       bool          unbind_from_stub;
> > @@ -941,7 +949,7 @@ virPCIDeviceReset(virPCIDevicePtr dev,
> >       if (virPCIDeviceGetDriverPathAndName(dev, &drvPath, &drvName) < 0)
> >           goto cleanup;
> >   
> > -    if (STREQ_NULLABLE(drvName, "vfio-pci")) {
> > +    if (virPCIStubDriverTypeFromString(drvName) == VIR_PCI_STUB_DRIVER_VFIO) {
> >           VIR_DEBUG("Device %s is bound to vfio-pci - skip reset",
> >                     dev->name);
> >           ret = 0;
> > @@ -992,13 +1000,22 @@ virPCIDeviceReset(virPCIDevicePtr dev,
> >   
> >   
> >   static int
> > -virPCIProbeStubDriver(const char *driver)
> > +virPCIProbeStubDriver(virPCIStubDriver driver)
> >   {
> > +    const char *drvname = NULL;
> >       char *drvpath = NULL;
> >       bool probed = false;
> >   
> > +    if (!(drvname = virPCIStubDriverTypeToString(driver)) ||
> > +        driver == VIR_PCI_STUB_DRIVER_NONE) {

You could save a small bit of time by comparing to 
VIR_PCI_STUB_DRIVER_NONE first, and then calling ...TypeToString()...


> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       "%s",
> > +                       _("Attempting to use unknown stub driver"));
> > +        return -1;
> > +    }
> Hm. Interesting, I thought that checking for TypeToString(driver) would
> be useless, since we are passing an enum into the function. But
> apparently I was wrong since the following code does not throw any error
> whatsoever:
>
> virPCIProbeStubDriver(20);

You mean no error at compile time I guess (I was a bit confused at 
first, trying to see how virPCIStubDriverTypeToString could possibly not 
return NULL for an out of range enum value :-)

> So I guess it's a good idea to have it there. Also, this brings up an
> interesting question - should we check for other TypeToString() return
> values too? e.g. like we do in virCPUDefFormatBuf().

There is inconsistency in this throughout the code. I think older 
*Format functions tend to not check the return, as they are assuming 
that the data being formatted was previously parsed (and the parse would 
only have put a proper enum in there), but newer code is more likely to 
be paranoid and check the return value. I admit to having done both, 
depending on just how paranoid I'm feeling at the time - not checking 
the return makes the code cleaner and there is a *very* low probability 
that the data will have come from somewhere other than the output of the 
parser; on the other hand, this is not always the case, and adding in 
the checks for NULL assures that we catch a bug in the code sooner 
rather than later. Definitely if you're calling it with data that didn't 
come directly out of a parser, then you really really should be checking 
the return for NULL.



> > +
> >    recheck:
> > -    if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) {
> > +    if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) {
> >           /* driver already loaded, return */
> >           VIR_FREE(drvpath);
> >           return 0;
>
> ACK
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list