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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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) {
> +        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);

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().

> +
>   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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]