There is no need to open code the PCI address string format when we have a function that does exactly that. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_audit.c | 6 +----- src/util/virpci.c | 6 ++---- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 8bc6633af4..5fa65a8078 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -361,11 +361,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS: switch ((virDomainHostdevSubsysType) hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: - address = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, - pcisrc->addr.domain, - pcisrc->addr.bus, - pcisrc->addr.slot, - pcisrc->addr.function); + address = virPCIDeviceAddressAsString(&pcisrc->addr); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: address = g_strdup_printf("%.3d.%.3d", usbsrc->bus, usbsrc->device); diff --git a/src/util/virpci.c b/src/util/virpci.c index 50fd5ef7ea..5f50f92abb 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1471,8 +1471,7 @@ virPCIDeviceNew(unsigned int domain, dev->address.slot = slot; dev->address.function = function; - dev->name = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, domain, bus, slot, - function); + dev->name = virPCIDeviceAddressAsString(&dev->address); dev->path = g_strdup_printf(PCI_SYSFS "devices/%s/config", dev->name); @@ -1998,8 +1997,7 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) g_autofree char *groupNumStr = NULL; unsigned int groupNum; - devName = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, addr->domain, addr->bus, - addr->slot, addr->function); + devName = virPCIDeviceAddressAsString(addr); devPath = virPCIFile(devName, "iommu_group"); -- 2.26.2

libxlNodeDeviceGetPCIInfo() and qemuNodeDeviceGetPCIInfo() are equal. Let's move the logic to a new virDomainDriverNodeDeviceGetPCIInfo() info to be used by libxl_driver.c and qemu_driver.c. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/domain_driver.c | 33 +++++++++++++++++++++++++++++ src/hypervisor/domain_driver.h | 7 +++++++ src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 38 ++++------------------------------ src/qemu/qemu_driver.c | 37 +++------------------------------ 5 files changed, 48 insertions(+), 68 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 8dc5870a61..68dbf10ac6 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -21,6 +21,7 @@ #include <config.h> #include "domain_driver.h" +#include "node_device_conf.h" #include "viralloc.h" #include "virstring.h" #include "vircrypto.h" @@ -336,3 +337,35 @@ virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, return ret; } + + +int +virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, + unsigned *domain, + unsigned *bus, + unsigned *slot, + unsigned *function) +{ + virNodeDevCapsDefPtr cap; + + cap = def->caps; + while (cap) { + if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { + *domain = cap->data.pci_dev.domain; + *bus = cap->data.pci_dev.bus; + *slot = cap->data.pci_dev.slot; + *function = cap->data.pci_dev.function; + break; + } + + cap = cap->next; + } + + if (!cap) { + virReportError(VIR_ERR_INVALID_ARG, + _("device %s is not a PCI device"), def->name); + return -1; + } + + return 0; +} diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index b66ae2d421..2bb053d559 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -21,6 +21,7 @@ #pragma once #include "domain_conf.h" +#include "node_device_conf.h" char * virDomainDriverGenerateRootHash(const char *drivername, @@ -45,3 +46,9 @@ int virDomainDriverParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, virTypedParameterPtr params, int nparams); + +int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, + unsigned *domain, + unsigned *bus, + unsigned *slot, + unsigned *function); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c325040b60..55284577b0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1445,6 +1445,7 @@ virDomainCgroupSetupMemtune; virDomainDriverGenerateMachineName; virDomainDriverGenerateRootHash; virDomainDriverMergeBlkioDevice; +virDomainDriverNodeDeviceGetPCIInfo; virDomainDriverParseBlkioDeviceStr; virDomainDriverSetupPersistentDefBlkioParams; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5bd3614e21..0821d39c9b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -56,6 +56,7 @@ #include "cpu/cpu.h" #include "virutil.h" #include "domain_validate.h" +#include "domain_driver.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -5773,37 +5774,6 @@ libxlConnectSupportsFeature(virConnectPtr conn, int feature) } } -static int -libxlNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) -{ - virNodeDevCapsDefPtr cap; - - cap = def->caps; - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; - break; - } - - cap = cap->next; - } - - if (!cap) { - virReportError(VIR_ERR_INVALID_ARG, - _("device %s is not a PCI device"), def->name); - return -1; - } - - return 0; -} - static int libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, const char *driverName, @@ -5845,7 +5815,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup; - if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); @@ -5916,7 +5886,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup; - if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); @@ -5974,7 +5944,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup; - if (libxlNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 027617deef..0a732a241d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11955,37 +11955,6 @@ qemuDomainMigrateConfirm3Params(virDomainPtr domain, } -static int -qemuNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) -{ - virNodeDevCapsDefPtr cap; - - cap = def->caps; - while (cap) { - if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; - break; - } - - cap = cap->next; - } - - if (!cap) { - virReportError(VIR_ERR_INVALID_ARG, - _("device %s is not a PCI device"), def->name); - return -1; - } - - return 0; -} - static int qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, const char *driverName, @@ -12028,7 +11997,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup; - if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); @@ -12109,7 +12078,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup; - if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); @@ -12163,7 +12132,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup; - if (qemuNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) goto cleanup; pci = virPCIDeviceNew(domain, bus, slot, function); -- 2.26.2

Instead of receiving 4 uints in order and write domain/bus/slot/function, receive a virPCIDeviceAddressPtr instead and write into it. This change will allow us to simplify the API for virPCIDeviceNew() in the next patch. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/domain_driver.c | 13 +++++-------- src/hypervisor/domain_driver.h | 5 +---- src/libxl/libxl_driver.c | 18 +++++++++--------- src/qemu/qemu_driver.c | 18 +++++++++--------- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c index 68dbf10ac6..cdb53bcf3e 100644 --- a/src/hypervisor/domain_driver.c +++ b/src/hypervisor/domain_driver.c @@ -341,20 +341,17 @@ virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function) + virPCIDeviceAddressPtr devAddr) { virNodeDevCapsDefPtr cap; cap = def->caps; while (cap) { if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - *domain = cap->data.pci_dev.domain; - *bus = cap->data.pci_dev.bus; - *slot = cap->data.pci_dev.slot; - *function = cap->data.pci_dev.function; + devAddr->domain = cap->data.pci_dev.domain; + devAddr->bus = cap->data.pci_dev.bus; + devAddr->slot = cap->data.pci_dev.slot; + devAddr->function = cap->data.pci_dev.function; break; } diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h index 2bb053d559..86b92d0284 100644 --- a/src/hypervisor/domain_driver.h +++ b/src/hypervisor/domain_driver.h @@ -48,7 +48,4 @@ int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef, int nparams); int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def, - unsigned *domain, - unsigned *bus, - unsigned *slot, - unsigned *function); + virPCIDeviceAddressPtr devAddr); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0821d39c9b..360d553a22 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5780,7 +5780,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, unsigned int flags) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5815,10 +5815,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; @@ -5853,7 +5853,7 @@ static int libxlNodeDeviceReAttach(virNodeDevicePtr dev) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5886,10 +5886,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; @@ -5911,7 +5911,7 @@ static int libxlNodeDeviceReset(virNodeDevicePtr dev) { virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; char *xml = NULL; @@ -5944,10 +5944,10 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a732a241d..28781cc34b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11962,7 +11962,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -11997,10 +11997,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; @@ -12046,7 +12046,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci = NULL; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -12078,10 +12078,10 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; @@ -12100,7 +12100,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) { virQEMUDriverPtr driver = dev->conn->privateData; virPCIDevicePtr pci; - unsigned domain = 0, bus = 0, slot = 0, function = 0; + virPCIDeviceAddress devAddr; int ret = -1; virNodeDeviceDefPtr def = NULL; g_autofree char *xml = NULL; @@ -12132,10 +12132,10 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0) goto cleanup; - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0) + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(domain, bus, slot, function); + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); if (!pci) goto cleanup; -- 2.26.2

The current virPCIDeviceNew() signature, receiving 4 uints in sequence (domain, bus, slot, function), is not neat. We already have a way to represent a PCI address in virPCIDeviceAddress that is used in the code. Aside from the test files, most of virPCIDeviceNew() callers have access to a virPCIDeviceAddress reference, but then we need to retrieve the 4 required uints (addr.domain, addr.bus, addr.slot, addr.function) to satisfy virPCIDeviceNew(). The result is that we have extra verbosity/boilerplate to retrieve an information that is already available in virPCIDeviceAddress. A better way is presented by virNVMEDeviceNew(), where the caller just supplies a virPCIDeviceAddress pointer and the function handles the details internally. This patch changes virPCIDeviceNew() to receive a virPCIDeviceAddress pointer instead of 4 uints. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 3 +-- src/libxl/libxl_driver.c | 6 ++--- src/node_device/node_device_udev.c | 11 +++++--- src/qemu/qemu_domain_address.c | 5 +--- src/qemu/qemu_driver.c | 6 ++--- src/security/security_apparmor.c | 3 +-- src/security/security_dac.c | 6 ++--- src/security/security_selinux.c | 6 ++--- src/security/virt-aa-helper.c | 6 +---- src/util/virnetdev.c | 3 +-- src/util/virnvme.c | 5 +--- src/util/virpci.c | 40 +++++++++--------------------- src/util/virpci.h | 5 +--- tests/virhostdevtest.c | 3 ++- tests/virpcitest.c | 35 +++++++++++++++++++------- 15 files changed, 64 insertions(+), 79 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index be32a26164..bd35397f2c 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -235,8 +235,7 @@ virHostdevGetPCIHostDevice(const virDomainHostdevDef *hostdev, hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) return 0; - actual = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + actual = virPCIDeviceNew(&pcisrc->addr); if (!actual) return -1; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 360d553a22..3eaf106006 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5818,7 +5818,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; @@ -5889,7 +5889,7 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; @@ -5947,7 +5947,7 @@ libxlNodeDeviceReset(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..fceb135aa5 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -367,6 +367,7 @@ udevProcessPCI(struct udev_device *device, virNodeDevCapPCIDevPtr pci_dev = &def->caps->data.pci_dev; virPCIEDeviceInfoPtr pci_express = NULL; virPCIDevicePtr pciDev = NULL; + virPCIDeviceAddress devAddr; int ret = -1; char *p; bool privileged; @@ -416,10 +417,12 @@ udevProcessPCI(struct udev_device *device, if (virNodeDeviceGetPCIDynamicCaps(def->sysfs_path, pci_dev) < 0) goto cleanup; - if (!(pciDev = virPCIDeviceNew(pci_dev->domain, - pci_dev->bus, - pci_dev->slot, - pci_dev->function))) + devAddr.domain = pci_dev->domain; + devAddr.bus = pci_dev->bus; + devAddr.slot = pci_dev->slot; + devAddr.function = pci_dev->function; + + if (!(pciDev = virPCIDeviceNew(&devAddr))) goto cleanup; /* We need to be root to read PCI device configs */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index f0ba318cc8..ae4ad6677c 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -857,10 +857,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, return 0; } - if (!(pciDev = virPCIDeviceNew(hostAddr->domain, - hostAddr->bus, - hostAddr->slot, - hostAddr->function))) { + if (!(pciDev = virPCIDeviceNew(hostAddr))) { /* libvirt should be able to perform all the * operations in virPCIDeviceNew() even if it's * running unprivileged, so if this fails, the device diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 28781cc34b..7581e3c8cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12000,7 +12000,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev, if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; @@ -12081,7 +12081,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; @@ -12135,7 +12135,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev) if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) goto cleanup; - pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function); + pci = virPCIDeviceNew(&devAddr); if (!pci) goto cleanup; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1b035cce2f..b564866678 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -876,8 +876,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr); if (!pci) goto done; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4f4a0a069e..b5e56feaa8 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1268,8 +1268,7 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr); if (!pci) return -1; @@ -1437,8 +1436,7 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr); if (!pci) return -1; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2fc6ef2616..76edaed027 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2103,8 +2103,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr); if (!pci) return -1; @@ -2343,8 +2342,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { virPCIDevicePtr pci = - virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, - pcisrc->addr.slot, pcisrc->addr.function); + virPCIDeviceNew(&pcisrc->addr); if (!pci) return -1; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5a6f4a5f7d..5efdb61e59 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1104,11 +1104,7 @@ get_files(vahControl * ctl) } case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevicePtr pci = virPCIDeviceNew( - dev->source.subsys.u.pci.addr.domain, - dev->source.subsys.u.pci.addr.bus, - dev->source.subsys.u.pci.addr.slot, - dev->source.subsys.u.pci.addr.function); + virPCIDevicePtr pci = virPCIDeviceNew(&dev->source.subsys.u.pci.addr); virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index a73e5f72f1..acb3ec960c 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1141,8 +1141,7 @@ virNetDevGetPCIDevice(const char *devName) if (!vfPCIAddr) return NULL; - return virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus, - vfPCIAddr->slot, vfPCIAddr->function); + return virPCIDeviceNew(vfPCIAddr); } # endif diff --git a/src/util/virnvme.c b/src/util/virnvme.c index b8179aa431..66b73cd1d1 100644 --- a/src/util/virnvme.c +++ b/src/util/virnvme.c @@ -290,10 +290,7 @@ virNVMeDeviceCreatePCIDevice(const virNVMeDevice *nvme) { g_autoptr(virPCIDevice) pci = NULL; - if (!(pci = virPCIDeviceNew(nvme->address.domain, - nvme->address.bus, - nvme->address.slot, - nvme->address.function))) + if (!(pci = virPCIDeviceNew(&nvme->address))) return NULL; /* NVMe devices must be bound to vfio */ diff --git a/src/util/virpci.c b/src/util/virpci.c index 5f50f92abb..5a91553b5f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -475,24 +475,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, return -1; while ((ret = virDirRead(dir, &entry, PCI_SYSFS "devices")) > 0) { - unsigned int domain, bus, slot, function; g_autoptr(virPCIDevice) check = NULL; + virPCIDeviceAddress devAddr; char *tmp; /* expected format: <domain>:<bus>:<slot>.<function> */ if (/* domain */ - virStrToLong_ui(entry->d_name, &tmp, 16, &domain) < 0 || *tmp != ':' || + virStrToLong_ui(entry->d_name, &tmp, 16, &devAddr.domain) < 0 || *tmp != ':' || /* bus */ - virStrToLong_ui(tmp + 1, &tmp, 16, &bus) < 0 || *tmp != ':' || + virStrToLong_ui(tmp + 1, &tmp, 16, &devAddr.bus) < 0 || *tmp != ':' || /* slot */ - virStrToLong_ui(tmp + 1, &tmp, 16, &slot) < 0 || *tmp != '.' || + virStrToLong_ui(tmp + 1, &tmp, 16, &devAddr.slot) < 0 || *tmp != '.' || /* function */ - virStrToLong_ui(tmp + 1, NULL, 16, &function) < 0) { + virStrToLong_ui(tmp + 1, NULL, 16, &devAddr.function) < 0) { VIR_WARN("Unusual entry in " PCI_SYSFS "devices: %s", entry->d_name); continue; } - check = virPCIDeviceNew(domain, bus, slot, function); + check = virPCIDeviceNew(&devAddr); if (!check) { ret = -1; break; @@ -767,10 +767,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) */ if (dev->address.bus > secondary && dev->address.bus <= subordinate) { if (*best == NULL) { - *best = virPCIDeviceNew(check->address.domain, - check->address.bus, - check->address.slot, - check->address.function); + *best = virPCIDeviceNew(&check->address); if (*best == NULL) { ret = -1; goto cleanup; @@ -790,10 +787,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) if (secondary > best_secondary) { virPCIDeviceFree(*best); - *best = virPCIDeviceNew(check->address.domain, - check->address.bus, - check->address.slot, - check->address.function); + *best = virPCIDeviceNew(&check->address); if (*best == NULL) { ret = -1; goto cleanup; @@ -1455,10 +1449,7 @@ virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) } virPCIDevicePtr -virPCIDeviceNew(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function) +virPCIDeviceNew(const virPCIDeviceAddress *address) { g_autoptr(virPCIDevice) dev = NULL; g_autofree char *vendor = NULL; @@ -1466,10 +1457,7 @@ virPCIDeviceNew(unsigned int domain, dev = g_new0(virPCIDevice, 1); - dev->address.domain = domain; - dev->address.bus = bus; - dev->address.slot = slot; - dev->address.function = function; + virPCIDeviceAddressCopy(&dev->address, address); dev->name = virPCIDeviceAddressAsString(&dev->address); @@ -1896,8 +1884,7 @@ virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) virPCIDeviceListPtr groupList = opaque; g_autoptr(virPCIDevice) newDev = NULL; - if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus, - newDevAddr->slot, newDevAddr->function))) + if (!(newDev = virPCIDeviceNew(newDevAddr))) return -1; if (virPCIDeviceListAdd(groupList, newDev) < 0) @@ -2028,10 +2015,7 @@ virPCIDeviceAddressGetIOMMUGroupDev(const virPCIDeviceAddress *devAddr) { g_autoptr(virPCIDevice) pci = NULL; - if (!(pci = virPCIDeviceNew(devAddr->domain, - devAddr->bus, - devAddr->slot, - devAddr->function))) + if (!(pci = virPCIDeviceNew(devAddr))) return NULL; return virPCIDeviceGetIOMMUGroupDev(pci); diff --git a/src/util/virpci.h b/src/util/virpci.h index 43828b0a8a..d4451848c1 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -113,10 +113,7 @@ struct _virPCIEDeviceInfo { virPCIELink *link_sta; /* Actually negotiated capabilities */ }; -virPCIDevicePtr virPCIDeviceNew(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function); +virPCIDevicePtr virPCIDeviceNew(const virPCIDeviceAddress *address); virPCIDevicePtr virPCIDeviceCopy(virPCIDevicePtr dev); void virPCIDeviceFree(virPCIDevicePtr dev); const char *virPCIDeviceGetName(virPCIDevicePtr dev); diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c index 385db0849a..40c14a5281 100644 --- a/tests/virhostdevtest.c +++ b/tests/virhostdevtest.c @@ -138,7 +138,8 @@ myInit(void) } for (i = 0; i < nhostdevs; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virDomainHostdevSubsys subsys = hostdevs[i]->source.subsys; + if (!(dev[i] = virPCIDeviceNew(&subsys.u.pci.addr))) goto cleanup; virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); diff --git a/tests/virpcitest.c b/tests/virpcitest.c index 6f064a3f85..6a4bd5518d 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -60,8 +60,9 @@ testVirPCIDeviceNew(const void *opaque G_GNUC_UNUSED) int ret = -1; virPCIDevicePtr dev; const char *devName; + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, .slot = 0, .function = 0}; - if (!(dev = virPCIDeviceNew(0, 0, 0, 0))) + if (!(dev = virPCIDeviceNew(&devAddr))) goto cleanup; devName = virPCIDeviceGetName(dev); @@ -103,7 +104,9 @@ testVirPCIDeviceDetach(const void *opaque G_GNUC_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0); for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup; virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); @@ -144,7 +147,9 @@ testVirPCIDeviceReset(const void *opaque G_GNUC_UNUSED) CHECK_LIST_COUNT(inactiveDevs, 0); for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup; virPCIDeviceSetStubDriver(dev[i], VIR_PCI_STUB_DRIVER_VFIO); @@ -176,7 +181,9 @@ testVirPCIDeviceReattach(const void *opaque G_GNUC_UNUSED) goto cleanup; for (i = 0; i < nDev; i++) { - if (!(dev[i] = virPCIDeviceNew(0, 0, i + 1, 0))) + virPCIDeviceAddress devAddr = {.domain = 0, .bus = 0, + .slot = i + 1, .function = 0}; + if (!(dev[i] = virPCIDeviceNew(&devAddr))) goto cleanup; if (virPCIDeviceListAdd(inactiveDevs, dev[i]) < 0) { @@ -222,8 +229,10 @@ testVirPCIDeviceIsAssignable(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; - if (!(dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function))) + if (!(dev = virPCIDeviceNew(&devAddr))) return -1; if (virPCIDeviceIsAssignable(dev, true)) @@ -239,8 +248,10 @@ testVirPCIDeviceDetachSingle(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup; @@ -261,8 +272,10 @@ testVirPCIDeviceReattachSingle(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup; @@ -285,8 +298,10 @@ testVirPCIDeviceCheckDriverTest(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup; @@ -305,8 +320,10 @@ testVirPCIDeviceUnbind(const void *opaque) const struct testPCIDevData *data = opaque; int ret = -1; virPCIDevicePtr dev; + virPCIDeviceAddress devAddr = {.domain = data->domain, .bus = data->bus, + .slot = data->slot, .function = data->function}; - dev = virPCIDeviceNew(data->domain, data->bus, data->slot, data->function); + dev = virPCIDeviceNew(&devAddr); if (!dev) goto cleanup; -- 2.26.2

On 1/20/21 10:17 PM, Laine Stump wrote: Convert nodedev_dev to use PCIDeviceAddr instead of 4 uints is not that hard (just checked with a quick search). I'm not sure if this would have an implication in the public API though. Thanks, DHB

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: This same string is used at the top of virPCIDeviceNew(). Might be good to have a short (static) utility function virPCIDeviceConfigFileName() function. But, as with most of my comments, that's for later. Reviewed-by: Laine Stump <laine(a)redhat.com&gt;

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: Resolution: "Don't do that!!" :-) Seriously, though, there are any number of things that someone with root access to the host could do that would completely confuse libvirt and break everything. For example, assigning a PF to a guest after having assigned one or more of that PF's VFs to guests. But I guess there's no harm in trying to mitigate the damage. As long as it doesn't complicate the code to the extent that it becomes more confusing, difficult to maintain, and liable to other regressions. If qemu knows enough to unplug the device from the guest, then I'm presuming that it must also be sending a DEVICE_DELETED event up to libvirt. Of course libvirt isn't expecting this, and so probably throws it away, right? What happens if the device disappeared and then came back before you got here? (i.e. by setting sriov_numvfs = 0, and then sriov_numvfs = 128 (or whatever)? It's too late in the day for my to process this too much, so I'm going to come back to it tomorrow. I will continue on to see if there is other "simple" stuff in subsequent patches though - even if we can't get all of this pushed right away, there's a significant amount that can be pushed.

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: You've created a special return code to mean "This is a PCI device, but it isn't present on the host"... ...But you haven't actually used it. Will it become necessary later to have this special value? right now a missing device is treated exactly the same as if it isn't a PCI device. I guess I can understand the conceptual desire to return an error of some type in this case though, and there are other places where something similar is done (-2 indicates some type of "odd" error), so I'll let it pass :-) Reviewed-by: Laine Stump <laine(a)redhat.com&gt;

Saying "modernize" in the title line is kind of odd - when I read that I thought it must be changing something related to selinux, not just switching to g_autoptr. How about you just say that in the first line (use g_auto* in security_selinux.c, or something like that). Aside from that: Reviewed-by: Laine Stump <laine(a)redhat.com&gt; On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

Again, don't use "modernize". Just "convert to use g_auto*" or something like that. On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: This is fine as far as it goes, but what about AppArmorSetSecurityHostdevLabel() (and also whatever it is that's going on in the get_files() function in virt-aa-helper.c). You likely don't have a setup to test it, but it seems pretty straightforward to extrapolate that if you should skip setting the selinux and DAC labels when a device doesn't exist, you can probably do the same thing for AppArmor. Reviewed-by: Laine Stump <laine(a)redhat.com&gt; but add another patch that fixes the problem for AppArmor too. (Also, all the code repetition here makes me think that there must be a better way to do this and reduce all the boilerplate, but I think it would be better to just make these changes and then look at eliminating the boilerplate afterwards, rather than re-structuring the code (which could create regressions) while adding this new concept on top of it.

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: NAK - you can't change a public function.

On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote: This patch is obsoleted by your own commit 23cdab6a3de0f6336505adcb446f77a6e0628e6b

Use g_auto* wherever we can and remove the 'cleanup' label. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/libxl/libxl_driver.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3eaf106006..fbb67e9ed6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5852,18 +5852,17 @@ libxlNodeDeviceDettach(virNodeDevicePtr dev) static int libxlNodeDeviceReAttach(virNodeDevicePtr dev) { - virPCIDevicePtr pci = NULL; + g_autoptr(virPCIDevice) pci = NULL; virPCIDeviceAddress devAddr; - int ret = -1; - virNodeDeviceDefPtr def = NULL; - char *xml = NULL; + g_autoptr(virNodeDeviceDef) def = NULL; + g_autofree char *xml = NULL; libxlDriverPrivatePtr driver = dev->conn->privateData; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - virConnectPtr nodeconn = NULL; - virNodeDevicePtr nodedev = NULL; + g_autoptr(virConnect) nodeconn = NULL; + g_autoptr(virNodeDevice) nodedev = NULL; if (!(nodeconn = virGetConnectNodeDev())) - goto cleanup; + return -1; /* 'dev' is associated with the QEMU virConnectPtr, * so for split daemons, we need to get a copy that @@ -5871,40 +5870,32 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev) */ if (!(nodedev = virNodeDeviceLookupByName( nodeconn, virNodeDeviceGetName(dev)))) - goto cleanup; + return -1; xml = virNodeDeviceGetXMLDesc(nodedev, 0); if (!xml) - goto cleanup; + return -1; def = virNodeDeviceDefParseString(xml, EXISTING_DEVICE, NULL); if (!def) - goto cleanup; + return -1; /* ACL check must happen against original 'dev', * not the new 'nodedev' we acquired */ if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0) - goto cleanup; + return -1; if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0) - goto cleanup; + return -1; pci = virPCIDeviceNew(&devAddr); if (!pci) - goto cleanup; + return -1; if (virHostdevPCINodeDeviceReAttach(hostdev_mgr, pci) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virPCIDeviceFree(pci); - virNodeDeviceDefFree(def); - virObjectUnref(nodedev); - virObjectUnref(nodeconn); - VIR_FREE(xml); - return ret; + return 0; } static int -- 2.26.2

Use g_auto* pointers to avoid the need of a cleanup label. The type of the pointer 'virDomainPtr dom' was changed to its alias 'virshDomainPtr' to allow the use of g_autoptr(). Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- tools/virsh-domain.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2bb136333f..b32529f073 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11953,11 +11953,10 @@ static const vshCmdOptDef opts_detach_device[] = { static bool cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) { - virDomainPtr dom = NULL; + g_autoptr(virshDomain) dom = NULL; const char *from = NULL; - char *buffer = NULL; + g_autofree char *buffer = NULL; int ret; - bool funcRet = false; bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); @@ -11982,11 +11981,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_AFFECT_LIVE; if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) - goto cleanup; + return false; if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { vshReportError(ctl); - goto cleanup; + return false; } if (flags != 0 || current) @@ -11996,16 +11995,11 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); - goto cleanup; + return false; } vshPrintExtra(ctl, "%s", _("Device detached successfully\n")); - funcRet = true; - - cleanup: - VIR_FREE(buffer); - virshDomainFree(dom); - return funcRet; + return true; } -- 2.26.2

Add a helper to quickly determine if a hostdev is a PCI device, instead of doing a tedius 'if' check with hostdev mode and subsys type. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 12 +++++++++--- src/hypervisor/virhostdev.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index dbba36193b..402d7be42d 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -347,12 +347,18 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, } +bool +virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev) +{ + return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; +} + + static bool virHostdevIsPCINetDevice(const virDomainHostdevDef *hostdev) { - return hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && - hostdev->parentnet != NULL; + return virHostdevIsPCIDevice(hostdev) && hostdev->parentnet != NULL; } diff --git a/src/hypervisor/virhostdev.h b/src/hypervisor/virhostdev.h index 811bda40ed..b9407cd837 100644 --- a/src/hypervisor/virhostdev.h +++ b/src/hypervisor/virhostdev.h @@ -235,3 +235,5 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr hostdev_mgr, const char *dom_name, virDomainDiskDefPtr *disks, size_t ndisks); + +bool virHostdevIsPCIDevice(const virDomainHostdevDef *hostdev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 36635e6de7..01fc50b687 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1461,6 +1461,7 @@ virCloseCallbacksUnset; # hypervisor/virhostdev.h virHostdevFindUSBDevice; +virHostdevIsPCIDevice; virHostdevManagerGetDefault; virHostdevPCINodeDeviceDetach; virHostdevPCINodeDeviceReAttach; -- 2.26.2

There is no need to bother with cgroup tearing down for absent PCI devices, given that their entries in the sysfs are already gone. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/qemu/qemu_cgroup.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f7146a71c9..050df21d87 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -467,6 +467,16 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) return 0; + /* Skip tearing down Cgroup for hostdevs that represents absent + * PCI devices, e.g. SR-IOV virtual functions that were removed from + * the host while the domain was still running. */ + if (virHostdevIsPCIDevice(dev)) { + const virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci; + + if (!virPCIDeviceExists(&pcisrc->addr)) + return 0; + } + if (qemuDomainGetHostdevPath(dev, &path, NULL) < 0) return -1; -- 2.26.2

We're going to need a way to remove a PCI Device from a list without having a valid virPCIDevicePtr, because the device is missing from the host. This means that virPCIDevicesListDel() must operate with a PCI Device address instead. Turns out that virPCIDevicesListDel() and its related functions only use the virPCIDeviceAddressPtr of the virPCIDevicePtr, so this change is simple to do and will not cause hassle in all other callers. Let's start adapting virPCIDeviceListFindIndex() and crawl our way up to virPCIDevicesListDel(). Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/util/virpci.c | 15 ++++++++------- src/util/virpci.h | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 7143380348..1554acffb6 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1745,7 +1745,7 @@ virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, dev)); + return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, &dev->address)); } void @@ -1756,16 +1756,17 @@ virPCIDeviceListDel(virPCIDeviceListPtr list, } int -virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev) +virPCIDeviceListFindIndex(virPCIDeviceListPtr list, + virPCIDeviceAddressPtr devAddr) { size_t i; for (i = 0; i < list->count; i++) { virPCIDevicePtr other = list->devs[i]; - if (other->address.domain == dev->address.domain && - other->address.bus == dev->address.bus && - other->address.slot == dev->address.slot && - other->address.function == dev->address.function) + if (other->address.domain == devAddr->domain && + other->address.bus == devAddr->bus && + other->address.slot == devAddr->slot && + other->address.function == devAddr->function) return i; } return -1; @@ -1798,7 +1799,7 @@ virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDevicePtr dev) { int idx; - if ((idx = virPCIDeviceListFindIndex(list, dev)) >= 0) + if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0) return list->devs[idx]; else return NULL; diff --git a/src/util/virpci.h b/src/util/virpci.h index a9c597a428..8c6776da21 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -175,7 +175,7 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, unsigned int slot, unsigned int function); int virPCIDeviceListFindIndex(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr); /* * Callback that will be invoked once for each file -- 2.26.2

Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 12 ++++++++---- src/util/virpci.c | 16 ++++++++-------- src/util/virpci.h | 2 +- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index 402d7be42d..3bfb04c674 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -657,7 +657,8 @@ virHostdevReattachAllPCIDevices(virHostdevManagerPtr mgr, /* We need to look up the actual device because that's what * virPCIDeviceReattach() expects as its argument */ - if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci))) + if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, + virPCIDeviceGetAddress(pci)))) continue; if (virPCIDeviceGetManaged(actual)) { @@ -777,7 +778,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, /* Unmanaged devices should already have been marked as * inactive: if that's the case, we can simply move on */ - if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)) { + if (virPCIDeviceListFind(mgr->inactivePCIHostdevs, + virPCIDeviceGetAddress(pci))) { VIR_DEBUG("Not detaching unmanaged PCI device %s", virPCIDeviceGetName(pci)); continue; @@ -860,7 +862,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, * there because 'pci' only contain address information and will * be released at the end of the function */ pci = virPCIDeviceListGet(pcidevs, i); - actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci)); VIR_DEBUG("Setting driver and domain information for PCI device %s", virPCIDeviceGetName(pci)); @@ -992,7 +995,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, * information such as by which domain and driver it is used. As a * side effect, by looking it up we can also tell whether it was * really active in the first place */ - actual = virPCIDeviceListFind(mgr->activePCIHostdevs, pci); + actual = virPCIDeviceListFind(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci)); if (actual) { const char *actual_drvname; const char *actual_domname; diff --git a/src/util/virpci.c b/src/util/virpci.c index 1554acffb6..9544275c31 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -705,7 +705,7 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void return 0; /* same bus, but inactive, i.e. about to be assigned to guest */ - if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, check)) + if (inactiveDevs && virPCIDeviceListFind(inactiveDevs, &check->address)) return 0; return 1; @@ -1022,7 +1022,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, return -1; } - if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not resetting active device %s"), dev->name); return -1; @@ -1294,7 +1294,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev, if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1; - if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not detaching active device %s"), dev->name); return -1; @@ -1306,7 +1306,7 @@ virPCIDeviceDetach(virPCIDevicePtr dev, /* Add *a copy of* the dev into list inactiveDevs, if * it's not already there. */ - if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, dev)) { + if (inactiveDevs && !virPCIDeviceListFind(inactiveDevs, &dev->address)) { VIR_DEBUG("Adding PCI device %s to inactive list", dev->name); if (virPCIDeviceListAddCopy(inactiveDevs, dev) < 0) return -1; @@ -1324,7 +1324,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev, virPCIDeviceListPtr activeDevs, virPCIDeviceListPtr inactiveDevs) { - if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { + if (activeDevs && virPCIDeviceListFind(activeDevs, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not reattaching active device %s"), dev->name); return -1; @@ -1684,7 +1684,7 @@ int virPCIDeviceListAdd(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - if (virPCIDeviceListFind(list, dev)) { + if (virPCIDeviceListFind(list, &dev->address)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s is already in use"), dev->name); return -1; @@ -1795,11 +1795,11 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, virPCIDevicePtr -virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDevicePtr dev) +virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDeviceAddressPtr devAddr) { int idx; - if ((idx = virPCIDeviceListFindIndex(list, &dev->address)) >= 0) + if ((idx = virPCIDeviceListFindIndex(list, devAddr)) >= 0) return list->devs[idx]; else return NULL; diff --git a/src/util/virpci.h b/src/util/virpci.h index 8c6776da21..628a293972 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -167,7 +167,7 @@ virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list, void virPCIDeviceListDel(virPCIDeviceListPtr list, virPCIDevicePtr dev); virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr); virPCIDevicePtr virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, unsigned int domain, -- 2.26.2

Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 8 +++++--- src/util/virpci.c | 6 +++--- src/util/virpci.h | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index 3bfb04c674..4042f874d6 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -846,7 +846,7 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, VIR_DEBUG("Removing PCI device %s from inactive list", virPCIDeviceGetName(pci)); - actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, pci); + actual = virPCIDeviceListSteal(mgr->inactivePCIHostdevs, virPCIDeviceGetAddress(pci)); VIR_DEBUG("Adding PCI device %s to active list", virPCIDeviceGetName(pci)); @@ -918,7 +918,8 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr, VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci))) + if (!(actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci)))) continue; VIR_DEBUG("Adding PCI device %s to inactive list", @@ -1022,7 +1023,8 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(pci)); - actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, pci); + actual = virPCIDeviceListSteal(mgr->activePCIHostdevs, + virPCIDeviceGetAddress(pci)); VIR_DEBUG("Adding PCI device %s to inactive list", virPCIDeviceGetName(pci)); diff --git a/src/util/virpci.c b/src/util/virpci.c index 9544275c31..332e681c43 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1743,16 +1743,16 @@ virPCIDeviceListStealIndex(virPCIDeviceListPtr list, virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list, - virPCIDevicePtr dev) + virPCIDeviceAddressPtr devAddr) { - return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, &dev->address)); + return virPCIDeviceListStealIndex(list, virPCIDeviceListFindIndex(list, devAddr)); } void virPCIDeviceListDel(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - virPCIDeviceFree(virPCIDeviceListSteal(list, dev)); + virPCIDeviceFree(virPCIDeviceListSteal(list, &dev->address)); } int diff --git a/src/util/virpci.h b/src/util/virpci.h index 628a293972..e3458a75fa 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -161,7 +161,7 @@ virPCIDevicePtr virPCIDeviceListGet(virPCIDeviceListPtr list, int idx); size_t virPCIDeviceListCount(virPCIDeviceListPtr list); virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr); virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list, int idx); void virPCIDeviceListDel(virPCIDeviceListPtr list, -- 2.26.2

This change will allow us to remove PCI devices from a list without the need of a PCI Device object, which will be need in the next patch. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 7 ++++--- src/util/virpci.c | 6 +++--- src/util/virpci.h | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index 4042f874d6..c708791eec 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -1005,11 +1005,11 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, if (STRNEQ_NULLABLE(drv_name, actual_drvname) || STRNEQ_NULLABLE(dom_name, actual_domname)) { - virPCIDeviceListDel(pcidevs, pci); + virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci)); continue; } } else { - virPCIDeviceListDel(pcidevs, pci); + virPCIDeviceListDel(pcidevs, virPCIDeviceGetAddress(pci)); continue; } @@ -2522,7 +2522,8 @@ virHostdevUpdateActiveNVMeDevices(virHostdevManagerPtr hostdev_mgr, while (lastGoodPCIIdx >= 0) { virPCIDevicePtr actual = virPCIDeviceListGet(pciDevices, i); - virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, actual); + virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, + virPCIDeviceGetAddress(actual)); lastGoodPCIIdx--; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 332e681c43..76dfd71802 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1336,7 +1336,7 @@ virPCIDeviceReattach(virPCIDevicePtr dev, /* Steal the dev from list inactiveDevs */ if (inactiveDevs) { VIR_DEBUG("Removing PCI device %s from inactive list", dev->name); - virPCIDeviceListDel(inactiveDevs, dev); + virPCIDeviceListDel(inactiveDevs, &dev->address); } return 0; @@ -1750,9 +1750,9 @@ virPCIDeviceListSteal(virPCIDeviceListPtr list, void virPCIDeviceListDel(virPCIDeviceListPtr list, - virPCIDevicePtr dev) + virPCIDeviceAddressPtr devAddr) { - virPCIDeviceFree(virPCIDeviceListSteal(list, &dev->address)); + virPCIDeviceFree(virPCIDeviceListSteal(list, devAddr)); } int diff --git a/src/util/virpci.h b/src/util/virpci.h index e3458a75fa..eb71eb4451 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -165,7 +165,7 @@ virPCIDevicePtr virPCIDeviceListSteal(virPCIDeviceListPtr list, virPCIDevicePtr virPCIDeviceListStealIndex(virPCIDeviceListPtr list, int idx); void virPCIDeviceListDel(virPCIDeviceListPtr list, - virPCIDevicePtr dev); + virPCIDeviceAddressPtr devAddr); virPCIDevicePtr virPCIDeviceListFind(virPCIDeviceListPtr list, virPCIDeviceAddressPtr devAddr); virPCIDevicePtr -- 2.26.2

virHostdevReAttachPCIDevices() is called when we want to re-attach a list of hostdevs back to the host, either on the shutdown path or via a 'virsh detach-device' call. This function always count on the existence of the device in the host to work, but this can lead to problems. For example, a SR-IOV device can be removed via an admin "echo 0 > /sys/bus/pci/devices/<addr>/sriov_numvfs", making the kernel fire up and eventfd_signal() to the process, asking for the process to release the device. The result might vary depending on the device driver and OS/arch, but two possible outcomes are: 1) the hypervisor driver will detach the device from the VM, issuing a delete event to Libvirt. This can be observed in QEMU; 2) the 'echo 0 > ...' will hang waiting for the device to be unplugged. This means that the VM process failed/refused to release the hostdev back to the host, and the hostdev will be detached during VM shutdown. Today we don't behave well for both cases. We'll fail to remove the PCI device reference from mgr->activePCIHostdevs and mgr->inactivePCIHostdevs because we rely on the existence of the PCI device conf file in the sysfs. Attempting to re-utilize the same device (assuming it is now present back in the host) can result in an error like this: $ ./run tools/virsh start vm1-sriov --console error: Failed to start domain vm1-sriov error: Requested operation is not valid: PCI device 0000:01:00.2 is in use by driver QEMU, domain vm1-sriov For (1), a VM destroy/start cycle is needed to re-use the VF in the guest. For (2), the effect is more nefarious, requiring a Libvirtd daemon restart to use the VF again in any guest. We can make it a bit better by checking, during virHostdevReAttachPCIDevices(), if there is any missing PCI device that will be left behind in activePCIHostdevs and inactivePCIHostdevs lists. Remove any missing device found from both lists, unconditionally, matching the current state of the host. This change affects the code path in (1) (processDeviceDeletedEvent into qemuDomainRemoveDevice, all the way back to qemuHostdevReAttachPCIDevices) and also in (b) (qemuProcessStop into qemuHostdevReAttachDomainDevices). NB: Although this patch enables the possibility of 'outside Libvirt' SR-IOV hotunplug of PCI devices, if the hypervisor and the PCI driver copes with it, our goal is to mitigate what it is still considered an user oopsie. For all supported purposes, the admin must remove the SR-IOV VFs from all running domains before removing the VFs from the host. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/72 Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/hypervisor/virhostdev.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index c708791eec..ed43733e71 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -1077,6 +1077,40 @@ virHostdevReAttachPCIDevicesImpl(virHostdevManagerPtr mgr, } +static void +virHostdevDeleteMissingPCIDevices(virHostdevManagerPtr mgr, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + size_t i; + + if (nhostdevs == 0) + return; + + virObjectLock(mgr->activePCIHostdevs); + virObjectLock(mgr->inactivePCIHostdevs); + + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDef *hostdev = hostdevs[i]; + virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci; + g_autoptr(virPCIDevice) pci = NULL; + + if (virHostdevGetPCIHostDevice(hostdev, &pci) != -2) + continue; + + /* The PCI device from 'hostdev' does not exist in the host + * anymore. Delete it from both active and inactive lists to + * reflect the current host state. + */ + virPCIDeviceListDel(mgr->activePCIHostdevs, &pcisrc->addr); + virPCIDeviceListDel(mgr->inactivePCIHostdevs, &pcisrc->addr); + } + + virObjectUnlock(mgr->activePCIHostdevs); + virObjectUnlock(mgr->inactivePCIHostdevs); +} + + /* @oldStateDir: * For upgrade purpose: see virHostdevRestoreNetConfig */ @@ -1102,6 +1136,10 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr, virHostdevReAttachPCIDevicesImpl(mgr, drv_name, dom_name, pcidevs, hostdevs, nhostdevs, oldStateDir); + + /* Handle the case where PCI devices from the host went missing + * during the domain lifetime */ + virHostdevDeleteMissingPCIDevices(mgr, hostdevs, nhostdevs); } -- 2.26.2

On 2/22/21 1:12 AM, Laine Stump wrote: Oh yeah thanks for pointing this out. I learned this 20+ years ago and forgot it along the road. Heavy metal lyrics and World of Warcraft public chat aren't the best sources for learning proper English grammar, so it seems. That was a huge oversight. I've fixed it to unlock in the reverse order. Thanks for the reviews Laine. I dropped the patch you NACKED and fixed everything else following your suggestions. We're in freeze ATM and this isn't critical or urgent, so I'll wait for the next release before pushing it. Thanks, DHB