[libvirt] [PATCH v2] pci: Use virPCIDeviceAddress in virPCIDevice
Instead of replicating the information (domain, bus, slot, function) inside the virPCIDevice structure, use the already-existing virPCIDeviceAddress structure. Outside of the module, the only visible change is that the return value of virPCIDeviceGetAddress() must no longer be freed by the caller. --- Changes in v2: * use virPCIDeviceAddress instead of virPCIDeviceAddressPtr to avoid extra allocations src/util/virhostdev.c | 4 --- src/util/virpci.c | 95 +++++++++++++++++++++++---------------------------- 2 files changed, 43 insertions(+), 56 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index de029a0..173ac58 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -585,7 +585,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, goto cleanup; } - VIR_FREE(devAddr); if (!(devAddr = virPCIDeviceGetAddress(dev))) goto cleanup; @@ -728,7 +727,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr, virObjectUnlock(hostdev_mgr->activePCIHostdevs); virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnref(pcidevs); - VIR_FREE(devAddr); return ret; } @@ -1581,7 +1579,6 @@ virHostdevPCINodeDeviceDetach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } @@ -1613,7 +1610,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr hostdev_mgr, out: virObjectUnlock(hostdev_mgr->inactivePCIHostdevs); virObjectUnlock(hostdev_mgr->activePCIHostdevs); - VIR_FREE(devAddr); return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index bececb5..bb874ac 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -56,10 +56,7 @@ VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST, "", "2.5", "5", "8") struct _virPCIDevice { - unsigned int domain; - unsigned int bus; - unsigned int slot; - unsigned int function; + virPCIDeviceAddress address; char name[PCI_ADDR_LEN]; /* domain:bus:slot.function */ char id[PCI_ID_LEN]; /* product vendor */ @@ -655,10 +652,10 @@ virPCIDeviceSharesBusWithActive(virPCIDevicePtr dev, virPCIDevicePtr check, void virPCIDeviceList *inactiveDevs = data; /* Different domain, different bus, or simply identical device */ - if (dev->domain != check->domain || - dev->bus != check->bus || - (dev->slot == check->slot && - dev->function == check->function)) + if (dev->address.domain != check->address.domain || + dev->address.bus != check->address.bus || + (dev->address.slot == check->address.slot && + dev->address.function == check->address.function)) return 0; /* same bus, but inactive, i.e. about to be assigned to guest */ @@ -689,7 +686,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) int ret = 0; int fd; - if (dev->domain != check->domain) + if (dev->address.domain != check->address.domain) return 0; if ((fd = virPCIDeviceConfigOpen(check, false)) < 0) @@ -713,7 +710,7 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) /* if the secondary bus exactly equals the device's bus, then we found * the direct parent. No further work is necessary */ - if (dev->bus == secondary) { + if (dev->address.bus == secondary) { ret = 1; goto cleanup; } @@ -722,10 +719,12 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) * In this case, what we need to do is look for the "best" match; i.e. * the most restrictive match that still satisfies all of the conditions. */ - if (dev->bus > secondary && dev->bus <= subordinate) { + if (dev->address.bus > secondary && dev->address.bus <= subordinate) { if (*best == NULL) { - *best = virPCIDeviceNew(check->domain, check->bus, check->slot, - check->function); + *best = virPCIDeviceNew(check->address.domain, + check->address.bus, + check->address.slot, + check->address.function); if (*best == NULL) { ret = -1; goto cleanup; @@ -745,8 +744,10 @@ virPCIDeviceIsParent(virPCIDevicePtr dev, virPCIDevicePtr check, void *data) if (secondary > best_secondary) { virPCIDeviceFree(*best); - *best = virPCIDeviceNew(check->domain, check->bus, check->slot, - check->function); + *best = virPCIDeviceNew(check->address.domain, + check->address.bus, + check->address.slot, + check->address.function); if (*best == NULL) { ret = -1; goto cleanup; @@ -970,7 +971,7 @@ virPCIDeviceReset(virPCIDevicePtr dev, ret = virPCIDeviceTryPowerManagementReset(dev, fd); /* Bus reset is not an option with the root bus */ - if (ret < 0 && dev->bus != 0) + if (ret < 0 && dev->address.bus != 0) ret = virPCIDeviceTrySecondaryBusReset(dev, fd, inactiveDevs); if (ret < 0) { @@ -1483,8 +1484,8 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher) virStrToLong_ui(tmp + 1, &tmp, 16, &function) < 0 || *tmp != '\n') continue; - if (domain != dev->domain || bus != dev->bus || slot != dev->slot || - function != dev->function) + if (domain != dev->address.domain || bus != dev->address.bus || + slot != dev->address.slot || function != dev->address.function) continue; in_matching_device = true; match_depth = strspn(line, " "); @@ -1560,17 +1561,16 @@ virPCIDeviceNew(unsigned int domain, if (VIR_ALLOC(dev) < 0) return NULL; - dev->domain = domain; - dev->bus = bus; - dev->slot = slot; - dev->function = function; + dev->address.domain = domain; + dev->address.bus = bus; + dev->address.slot = slot; + dev->address.function = function; if (snprintf(dev->name, sizeof(dev->name), "%.4x:%.2x:%.2x.%.1x", - dev->domain, dev->bus, dev->slot, - dev->function) >= sizeof(dev->name)) { + domain, bus, slot, function) >= sizeof(dev->name)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), - dev->domain, dev->bus, dev->slot, dev->function); + domain, bus, slot, function); goto error; } if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", @@ -1661,25 +1661,14 @@ virPCIDeviceFree(virPCIDevicePtr dev) * @dev: device to get address from * * Take a PCI device on input and return its PCI address. The - * caller must free the returned value when no longer needed. + * returned object is owned by the device and must not be freed. * * Returns NULL on failure, the device address on success. */ virPCIDeviceAddressPtr virPCIDeviceGetAddress(virPCIDevicePtr dev) { - - virPCIDeviceAddressPtr pciAddrPtr; - - if (!dev || (VIR_ALLOC(pciAddrPtr) < 0)) - return NULL; - - pciAddrPtr->domain = dev->domain; - pciAddrPtr->bus = dev->bus; - pciAddrPtr->slot = dev->slot; - pciAddrPtr->function = dev->function; - - return pciAddrPtr; + return &(dev->address); } const char * @@ -1890,12 +1879,14 @@ virPCIDeviceListFindIndex(virPCIDeviceListPtr list, virPCIDevicePtr dev) { size_t i; - for (i = 0; i < list->count; i++) - if (list->devs[i]->domain == dev->domain && - list->devs[i]->bus == dev->bus && - list->devs[i]->slot == dev->slot && - list->devs[i]->function == dev->function) + 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) return i; + } return -1; } @@ -1910,10 +1901,11 @@ virPCIDeviceListFindByIDs(virPCIDeviceListPtr list, size_t i; for (i = 0; i < list->count; i++) { - if (list->devs[i]->domain == domain && - list->devs[i]->bus == bus && - list->devs[i]->slot == slot && - list->devs[i]->function == function) + virPCIDevicePtr other = list->devs[i]; + if (other->address.domain == domain && + other->address.bus == bus && + other->address.slot == slot && + other->address.function == function) return list->devs[i]; } return NULL; @@ -1944,7 +1936,8 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, int direrr; if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x", - dev->domain, dev->bus, dev->slot, dev->function) < 0) + dev->address.domain, dev->address.bus, + dev->address.slot, dev->address.function) < 0) goto cleanup; if (!(dir = opendir(pcidir))) { @@ -2074,13 +2067,11 @@ virPCIDeviceListPtr virPCIDeviceGetIOMMUGroupList(virPCIDevicePtr dev) { virPCIDeviceListPtr groupList = virPCIDeviceListNew(); - virPCIDeviceAddress devAddr = { dev->domain, dev->bus, - dev->slot, dev->function }; if (!groupList) goto error; - if (virPCIDeviceAddressIOMMUGroupIterate(&devAddr, + if (virPCIDeviceAddressIOMMUGroupIterate(&(dev->address), virPCIDeviceGetIOMMUGroupAddOne, groupList) < 0) goto error; @@ -2294,7 +2285,7 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) * into play since devices on the root bus can't P2P without going * through the root IOMMU. */ - if (dev->bus == 0) { + if (dev->address.bus == 0) { return 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.5.0
On Tue, 2015-12-15 at 10:05 +0100, Andrea Bolognani wrote:
@@ -1661,25 +1661,14 @@ virPCIDeviceFree(virPCIDevicePtr dev) * @dev: device to get address from * * Take a PCI device on input and return its PCI address. The - * caller must free the returned value when no longer needed. + * returned object is owned by the device and must not be freed. * * Returns NULL on failure, the device address on success. */
Not sending a v3 just for this, but if ACKed I'll also squash in - * Returns NULL on failure, the device address on success. + * Returns: a pointer to the address, which can never be NULL. to keep the documentation consistent with the actual behaviour. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 12/15/2015 05:48 AM, Andrea Bolognani wrote:
On Tue, 2015-12-15 at 10:05 +0100, Andrea Bolognani wrote:
@@ -1661,25 +1661,14 @@ virPCIDeviceFree(virPCIDevicePtr dev) * @dev: device to get address from * * Take a PCI device on input and return its PCI address. The - * caller must free the returned value when no longer needed. + * returned object is owned by the device and must not be freed. * * Returns NULL on failure, the device address on success. */ Not sending a v3 just for this, but if ACKed I'll also squash in
- * Returns NULL on failure, the device address on success. + * Returns: a pointer to the address, which can never be NULL.
to keep the documentation consistent with the actual behaviour.
Also, the places where this is called no longer need to check for NULL and goto cleanup. And beyond that, 2 of the three callers only use the returned value a single time, so the code could be shortened even further by just using "virPCIDeviceGetAddress(blah)" directly, instead of placing that return value into a local, and then calling using the local a single time later. ACK for the whole patch as long as you remove the check for NULL from all the callers of virPCIDeviceGetAddress() (and optionally eliminate the temporary variable in the two cases where it is used only once).
On Tue, 2015-12-15 at 12:27 -0500, Laine Stump wrote:
On 12/15/2015 05:48 AM, Andrea Bolognani wrote:
On Tue, 2015-12-15 at 10:05 +0100, Andrea Bolognani wrote:
@@ -1661,25 +1661,14 @@ virPCIDeviceFree(virPCIDevicePtr dev) * @dev: device to get address from * * Take a PCI device on input and return its PCI address. The - * caller must free the returned value when no longer needed. + * returned object is owned by the device and must not be freed. * * Returns NULL on failure, the device address on success. */ Not sending a v3 just for this, but if ACKed I'll also squash in
- * Returns NULL on failure, the device address on success. + * Returns: a pointer to the address, which can never be NULL.
to keep the documentation consistent with the actual behaviour.
Also, the places where this is called no longer need to check for NULL and goto cleanup. And beyond that, 2 of the three callers only use the returned value a single time, so the code could be shortened even further by just using "virPCIDeviceGetAddress(blah)" directly, instead of placing that return value into a local, and then calling using the local a single time later.
ACK for the whole patch as long as you remove the check for NULL from all the callers of virPCIDeviceGetAddress() (and optionally eliminate the temporary variable in the two cases where it is used only once).
v3 here: https://www.redhat.com/archives/libvir-list/2015-December/msg00601.html Hopefully I haven't made any silly mistakes this time :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Laine Stump