10:12 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list