There is no need for a temporary variable in this function, and ever since we switched to glib for memory allocation, there is no possibility it can return NULL, so callers don't need to check for it. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virpci.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 6fa8acd246..0786ddd478 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -219,10 +219,7 @@ virPCIDriverDir(const char *driver) static char * virPCIFile(const char *device, const char *file) { - char *buffer; - - buffer = g_strdup_printf(PCI_SYSFS "devices/%s/%s", device, file); - return buffer; + return g_strdup_printf(PCI_SYSFS "devices/%s/%s", device, file); } @@ -240,9 +237,9 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) g_autofree char *drvlink = NULL; *path = *name = NULL; + /* drvlink = "/sys/bus/pci/dddd:bb:ss.ff/driver" */ - if (!(drvlink = virPCIFile(dev->name, "driver"))) - goto cleanup; + drvlink = virPCIFile(dev->name, "driver"); if (!virFileExists(drvlink)) { ret = 0; @@ -376,8 +373,7 @@ virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class) g_autofree char *id_str = NULL; unsigned int value; - if (!(path = virPCIFile(dev->name, "class"))) - return -1; + path = virPCIFile(dev->name, "class"); /* class string is '0xNNNNNN\n' ... i.e. 9 bytes */ if (virFileReadAll(path, 9, &id_str) < 0) @@ -1054,8 +1050,7 @@ virPCIDeviceUnbind(virPCIDevicePtr dev) /* The device is not bound to any driver */ return 0; - if (!(path = virPCIFile(dev->name, "driver/unbind"))) - return -1; + path = virPCIFile(dev->name, "driver/unbind"); if (virFileExists(path)) { if (virFileWriteStr(path, dev->name, 0) < 0) { @@ -1111,8 +1106,7 @@ virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, { g_autofree char *path = NULL; - if (!(path = virPCIFile(dev->name, "driver_override"))) - return -1; + path = virPCIFile(dev->name, "driver_override"); if (virFileWriteStr(path, driverName, 0) < 0) { virReportSystemError(errno, @@ -1159,10 +1153,11 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return -1; } - if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || - !(driverLink = virPCIFile(dev->name, "driver"))) + if (!(stubDriverPath = virPCIDriverDir(stubDriverName))) return -1; + driverLink = virPCIFile(dev->name, "driver"); + if (virFileExists(driverLink)) { if (virFileLinkPointsTo(driverLink, stubDriverPath)) { /* The device is already bound to the correct driver */ @@ -1259,8 +1254,7 @@ virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name) g_autofree char *path = NULL; char *id_str; - if (!(path = virPCIFile(dev->name, id_name))) - return NULL; + path = virPCIFile(dev->name, id_name); /* ID string is '0xNNNN\n' ... i.e. 7 bytes */ if (virFileReadAll(path, 7, &id_str) < 0) @@ -1924,8 +1918,8 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) devName = g_strdup_printf(VIR_PCI_DEVICE_ADDRESS_FMT, addr->domain, addr->bus, addr->slot, addr->function); - if (!(devPath = virPCIFile(devName, "iommu_group"))) - return -1; + devPath = virPCIFile(devName, "iommu_group"); + if (virFileIsLink(devPath) != 1) return -2; if (virFileResolveLink(devPath, &groupPath) < 0) { @@ -1973,8 +1967,8 @@ virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev) g_autofree char *groupPath = NULL; g_autofree char *groupFile = NULL; - if (!(devPath = virPCIFile(dev->name, "iommu_group"))) - return NULL; + devPath = virPCIFile(dev->name, "iommu_group"); + if (virFileIsLink(devPath) != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid device %s iommu_group file %s is not a symlink"), -- 2.26.2

This function had a loop that was only executed twice; it was artificially constructed with a label, a goto, and a boolean to tell that it had already been executed once. Aside from that, the body of the loop contained only two lines that needed to be repeated (the second time through, everything beyond those two lines would be skipped). One side effect of this strange loop was that a g_autofree string was manually freed and re-initialized; I've been told that manually freeing a g_auto_free object is highly discouraged. This patch refactors the function to simply repeat the 2 lines that might possibly be executed twice, thus eliminating the ugly use of goto to construct a loop, and also takes advantage of the fact that virPCIDriverDir() was previously returning *exactly* the same string both times it was called to eliminate the manual VIR_FREE of drvpath. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virpci.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index ac37a60576..045121c453 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -988,7 +988,7 @@ virPCIProbeStubDriver(virPCIStubDriver driver) { const char *drvname = NULL; g_autofree char *drvpath = NULL; - bool probed = false; + g_autofree char *errbuf = NULL; if (driver == VIR_PCI_STUB_DRIVER_NONE || !(drvname = virPCIStubDriverTypeToString(driver))) { @@ -998,25 +998,21 @@ virPCIProbeStubDriver(virPCIStubDriver driver) return -1; } - recheck: drvpath = virPCIDriverDir(drvname); + /* driver previously loaded, return */ if (virFileExists(drvpath)) - /* driver already loaded, return */ return 0; - if (!probed) { - g_autofree char *errbuf = NULL; - probed = true; - if ((errbuf = virKModLoad(drvname))) { - VIR_WARN("failed to load driver %s: %s", drvname, errbuf); - goto cleanup; - } - - VIR_FREE(drvpath); - goto recheck; + if ((errbuf = virKModLoad(drvname))) { + VIR_WARN("failed to load driver %s: %s", drvname, errbuf); + goto cleanup; } + /* driver loaded after probing */ + if (virFileExists(drvpath)) + return 0; + cleanup: /* If we know failure was because of admin config, let's report that; * otherwise, report a more generic failure message -- 2.26.2

thisPhysPortID is only used inside a conditional, so reduce its scope to just the body of that conditional, which will eliminate the need for the undesirable manual VIR_FREE(). Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virpci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 045121c453..3ca513702e 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2408,7 +2408,6 @@ virPCIGetNetName(const char *device_link_sysfs_path, { g_autofree char *pcidev_sysfs_net_path = NULL; g_autofree char *firstEntryName = NULL; - g_autofree char *thisPhysPortID = NULL; int ret = -1; DIR *dir = NULL; struct dirent *entry = NULL; @@ -2433,12 +2432,13 @@ virPCIGetNetName(const char *device_link_sysfs_path, * physportID of this netdev. If not, look for entry[idx]. */ if (physPortID) { + g_autofree char *thisPhysPortID = NULL; + if (virNetDevGetPhysPortID(entry->d_name, &thisPhysPortID) < 0) goto cleanup; /* if this one doesn't match, keep looking */ if (STRNEQ_NULLABLE(physPortID, thisPhysPortID)) { - VIR_FREE(thisPhysPortID); /* save the first entry we find to use as a failsafe * in case we don't match the phys_port_id. This is * needed because some NIC drivers (e.g. i40e) -- 2.26.2

These were nops once enough cleanup was g_auto'd. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virpci.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index c7ee259e29..855872031e 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1849,7 +1849,6 @@ typedef virPCIDeviceAddressList *virPCIDeviceAddressListPtr; static int virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) { - int ret = -1; virPCIDeviceAddressListPtr addrList = opaque; g_autofree virPCIDeviceAddressPtr copyAddr = NULL; @@ -1860,11 +1859,9 @@ virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaq if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices, *addrList->nIommuGroupDevices, copyAddr) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -2243,7 +2240,6 @@ virPCIGetVirtualFunctions(const char *sysfs_path, size_t *num_virtual_functions, unsigned int *max_virtual_functions) { - int ret = -1; size_t i; g_autofree char *totalvfs_file = NULL; g_autofree char *totalvfs_str = NULL; @@ -2290,16 +2286,14 @@ virPCIGetVirtualFunctions(const char *sysfs_path, VIR_DEBUG("Found %zu virtual functions for %s", *num_virtual_functions, sysfs_path); - ret = 0; - cleanup: - return ret; + return 0; error: for (i = 0; i < *num_virtual_functions; i++) VIR_FREE((*virtual_functions)[i]); VIR_FREE(*virtual_functions); *num_virtual_functions = 0; - goto cleanup; + return -1; } @@ -2489,22 +2483,21 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, g_autofree char *pf_sysfs_device_path = NULL; g_autofree char *vfname = NULL; g_autofree char *vfPhysPortID = NULL; - int ret = -1; if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) - goto cleanup; + return -1; if (!pf_config_address) - goto cleanup; + return -1; if (virPCIDeviceAddressGetSysfsFile(pf_config_address, &pf_sysfs_device_path) < 0) { - goto cleanup; + return -1; } if (virPCIGetVirtualFunctionIndex(pf_sysfs_device_path, vf_sysfs_device_path, vf_index) < 0) { - goto cleanup; + return -1; } /* If the caller hasn't asked for a specific pfNetDevIdx, and VF @@ -2516,18 +2509,18 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, */ if (pfNetDevIdx == -1) { if (virPCIGetNetName(vf_sysfs_device_path, 0, NULL, &vfname) < 0) - goto cleanup; + return -1; if (vfname) { if (virNetDevGetPhysPortID(vfname, &vfPhysPortID) < 0) - goto cleanup; + return -1; } pfNetDevIdx = 0; } if (virPCIGetNetName(pf_sysfs_device_path, pfNetDevIdx, vfPhysPortID, pfname) < 0) { - goto cleanup; + return -1; } if (!*pfname) { @@ -2537,12 +2530,10 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, virReportError(VIR_ERR_INTERNAL_ERROR, _("The PF device for VF %s has no network device name"), vf_sysfs_device_path); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.26.2

virPCIDeviceAddressGetSysfsFile() is simpler to call. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdev.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 088f35621d..e284d62233 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1212,13 +1212,9 @@ virNetDevGetVirtualFunctions(const char *pfname, *vfname = g_new0(char *, *n_vfname); for (i = 0; i < *n_vfname; i++) { - g_autofree char *pciConfigAddr = NULL; g_autofree char *pci_sysfs_device_link = NULL; - if (!(pciConfigAddr = virPCIDeviceAddressAsString((*virt_fns)[i]))) - goto cleanup; - - if (virPCIGetSysfsFile(pciConfigAddr, &pci_sysfs_device_link) < 0) { + if (virPCIDeviceAddressGetSysfsFile((*virt_fns)[i], &pci_sysfs_device_link) < 0) { virReportSystemError(ENOSYS, "%s", _("Failed to get PCI SYSFS file")); goto cleanup; -- 2.26.2

Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virpci.c | 15 --------------- src/util/virpci.h | 4 ---- 3 files changed, 20 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae543589f1..b2d786409b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2849,7 +2849,6 @@ virPCIGetHeaderType; virPCIGetMdevTypes; virPCIGetNetName; virPCIGetPhysicalFunction; -virPCIGetSysfsFile; virPCIGetVirtualFunctionIndex; virPCIGetVirtualFunctionInfo; virPCIGetVirtualFunctions; diff --git a/src/util/virpci.c b/src/util/virpci.c index 855872031e..1f679a7b45 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2358,14 +2358,6 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, * Returns a path to the PCI sysfs file given the BDF of the PCI function */ -int -virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link) -{ - *pci_sysfs_device_link = g_strdup_printf(PCI_SYSFS "devices/%s", - virPCIDeviceName); - return 0; -} - int virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr, char **pci_sysfs_device_link) @@ -2633,13 +2625,6 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link G_GNUC_UNUSED, } -int -virPCIGetSysfsFile(char *virPCIDeviceName G_GNUC_UNUSED, - char **pci_sysfs_device_link G_GNUC_UNUSED) -{ - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); - return -1; -} int virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr dev G_GNUC_UNUSED, diff --git a/src/util/virpci.h b/src/util/virpci.h index b3322ba61b..1f896ca481 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -234,10 +234,6 @@ int virPCIGetNetName(const char *device_link_sysfs_path, char *physPortID, char **netname); -int virPCIGetSysfsFile(char *virPCIDeviceName, - char **pci_sysfs_device_link) - G_GNUC_WARN_UNUSED_RESULT; - bool virPCIDeviceAddressIsValid(virPCIDeviceAddressPtr addr, bool report); bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); -- 2.26.2