[libvirt] [PATCH v2 3/6] pci: Introduce virPCIStubDriver enumeration
Laine Stump
laine at laine.org
Fri Dec 18 16:12:30 UTC 2015
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list