9:26 p.m.
These started with a couple small cleanups to functions I was going to
change the API for, but then I decided not to change the API, and just
didn't want to waste the effort spent cleaning them up.
Laine Stump (9):
util: simplify virHostdevPCISysfsPath()
util: simplify virPCIFile() and its callers
util: simplify virPCIDriverDir() and its callers
util: simplify virPCIProbeStubDriver()
util: avoid manual VIR_FREE of a g_autofree pointer in virPCIGetName()
util: use more g_autofree in virpci.c
util: remove unneeded cleanup:/ret in virpci.c
util: don't use virPCIGetSysfsFile()
util: remove unused function virPCIGetSysfsFile()
src/hypervisor/virhostdev.c | 10 +--
src/libvirt_private.syms | 1 -
src/util/virnetdev.c | 6 +-
src/util/virpci.c | 146 ++++++++++++------------------------
src/util/virpci.h | 4 -
5 files changed, 53 insertions(+), 114 deletions(-)
--
2.26.2
9:26 p.m.
New subject: [PATCH 1/9] util: simplify virHostdevPCISysfsPath()
Apparently at some point in the past, when there were multiple types
to represent PCI addresses, the function
virPCIDeviceAddressGetSysfsFile() used one of those types, while
virDomainHostDevDef used another. It's been quite awhile since we
reduced the number of different representations of PCI address, but
this function was still creating a temporary virPCIDeviceAddress, then
copying the individual elements into this temporary object from the
same type of object in the virDomainHostDevDef.
This patch just eliminates that pointless copy.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/hypervisor/virhostdev.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
index 69102b8ebf..120187b07a 100644
--- a/src/hypervisor/virhostdev.c
+++ b/src/hypervisor/virhostdev.c
@@ -285,18 +285,12 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int
nhostdevs)
return g_steal_pointer(&pcidevs);
}
+
static int
virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev,
char **sysfs_path)
{
- virPCIDeviceAddress config_address;
-
- config_address.domain = hostdev->source.subsys.u.pci.addr.domain;
- config_address.bus = hostdev->source.subsys.u.pci.addr.bus;
- config_address.slot = hostdev->source.subsys.u.pci.addr.slot;
- config_address.function = hostdev->source.subsys.u.pci.addr.function;
-
- return virPCIDeviceAddressGetSysfsFile(&config_address, sysfs_path);
+ return virPCIDeviceAddressGetSysfsFile(&hostdev->source.subsys.u.pci.addr,
sysfs_path);
}
--
2.26.2
9:26 p.m.
New subject: [PATCH 2/9] util: simplify virPCIFile() and its callers
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(a)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
9:26 p.m.
New subject: [PATCH 3/9] util: simplify virPCIDriverDir() and its callers
There is no need for a temporary variable in this function, and since
it can't return NULL, no need for callers to check for it.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 0786ddd478..ac37a60576 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -209,10 +209,7 @@ VIR_ONCE_GLOBAL_INIT(virPCI);
static char *
virPCIDriverDir(const char *driver)
{
- char *buffer;
-
- buffer = g_strdup_printf(PCI_SYSFS "drivers/%s", driver);
- return buffer;
+ return g_strdup_printf(PCI_SYSFS "drivers/%s", driver);
}
@@ -1002,7 +999,9 @@ virPCIProbeStubDriver(virPCIStubDriver driver)
}
recheck:
- if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath))
+ drvpath = virPCIDriverDir(drvname);
+
+ if (virFileExists(drvpath))
/* driver already loaded, return */
return 0;
@@ -1153,9 +1152,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev)
return -1;
}
- if (!(stubDriverPath = virPCIDriverDir(stubDriverName)))
- return -1;
-
+ stubDriverPath = virPCIDriverDir(stubDriverName);
driverLink = virPCIFile(dev->name, "driver");
if (virFileExists(driverLink)) {
--
2.26.2
9:26 p.m.
New subject: [PATCH 4/9] util: simplify virPCIProbeStubDriver()
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(a)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
3:41 a.m.
New subject: [PATCH 4/9] util: simplify virPCIProbeStubDriver()
On Tue, Oct 20, 2020 at 10:26:04PM -0400, Laine Stump wrote:
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(a)redhat.com>
Much better :)
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
9:26 p.m.
New subject: [PATCH 5/9] util: avoid manual VIR_FREE of a g_autofree pointer in virPCIGetName()
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(a)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
9:26 p.m.
New subject: [PATCH 6/9] util: use more g_autofree in virpci.c
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/util/virpci.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 3ca513702e..c7ee259e29 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1245,7 +1245,7 @@ static char *
virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name)
{
g_autofree char *path = NULL;
- char *id_str;
+ g_autofree char *id_str = NULL;
path = virPCIFile(dev->name, id_name);
@@ -1254,15 +1254,13 @@ virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name)
return NULL;
/* Check for 0x suffix */
- if (id_str[0] != '0' || id_str[1] != 'x') {
- VIR_FREE(id_str);
+ if (id_str[0] != '0' || id_str[1] != 'x')
return NULL;
- }
/* Chop off the newline; we know the string is 7 bytes */
id_str[6] = '\0';
- return id_str;
+ return g_steal_pointer(&id_str);
}
bool
@@ -1853,7 +1851,7 @@ virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr
newDevAddr, void *opaq
{
int ret = -1;
virPCIDeviceAddressListPtr addrList = opaque;
- virPCIDeviceAddressPtr copyAddr;
+ g_autofree virPCIDeviceAddressPtr copyAddr = NULL;
/* make a copy to insert onto the list */
copyAddr = g_new0(virPCIDeviceAddress, 1);
@@ -1866,7 +1864,6 @@ virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr
newDevAddr, void *opaq
ret = 0;
cleanup:
- VIR_FREE(copyAddr);
return ret;
}
@@ -2170,7 +2167,7 @@ virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr)
virPCIDeviceAddressPtr
virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
{
- virPCIDeviceAddressPtr bdf = NULL;
+ g_autofree virPCIDeviceAddressPtr bdf = NULL;
g_autofree char *config_address = NULL;
g_autofree char *device_path = NULL;
@@ -2194,11 +2191,10 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to parse PCI config address '%s'"),
config_address);
- VIR_FREE(bdf);
return NULL;
}
- return bdf;
+ return g_steal_pointer(&bdf);
}
/**
@@ -2251,7 +2247,7 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
size_t i;
g_autofree char *totalvfs_file = NULL;
g_autofree char *totalvfs_str = NULL;
- virPCIDeviceAddressPtr config_addr = NULL;
+ g_autofree virPCIDeviceAddressPtr config_addr = NULL;
*virtual_functions = NULL;
*num_virtual_functions = 0;
@@ -2296,7 +2292,6 @@ virPCIGetVirtualFunctions(const char *sysfs_path,
*num_virtual_functions, sysfs_path);
ret = 0;
cleanup:
- VIR_FREE(config_addr);
return ret;
error:
@@ -2333,7 +2328,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
size_t i;
size_t num_virt_fns = 0;
unsigned int max_virt_fns = 0;
- virPCIDeviceAddressPtr vf_bdf = NULL;
+ g_autofree virPCIDeviceAddressPtr vf_bdf = NULL;
virPCIDeviceAddressPtr *virt_fns = NULL;
if (!(vf_bdf = virPCIGetDeviceAddressFromSysfsLink(vf_sysfs_device_link)))
@@ -2362,8 +2357,6 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link,
VIR_FREE(virt_fns[i]);
VIR_FREE(virt_fns);
- VIR_FREE(vf_bdf);
-
return ret;
}
@@ -2492,7 +2485,7 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
char **pfname,
int *vf_index)
{
- virPCIDeviceAddressPtr pf_config_address = NULL;
+ g_autofree virPCIDeviceAddressPtr pf_config_address = NULL;
g_autofree char *pf_sysfs_device_path = NULL;
g_autofree char *vfname = NULL;
g_autofree char *vfPhysPortID = NULL;
@@ -2549,8 +2542,6 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
ret = 0;
cleanup:
- VIR_FREE(pf_config_address);
-
return ret;
}
--
2.26.2
9:26 p.m.
New subject: [PATCH 7/9] util: remove unneeded cleanup:/ret in virpci.c
These were nops once enough cleanup was g_auto'd.
Signed-off-by: Laine Stump <laine(a)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
3:36 a.m.
New subject: [PATCH 7/9] util: remove unneeded cleanup:/ret in virpci.c
On Tue, Oct 20, 2020 at 10:26:07PM -0400, Laine Stump wrote:
These were nops once enough cleanup was g_auto'd.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
Probably not as trivial as the removal of the ones below, but IMO it would
still be straightforward to also drop the cleanup label in
virPCIDeviceGetDriverPathAndName. All is needed is not to touch the caller
provided pointer until the very last moment you'd do a g_steal.
Perhaps a candidate for a standalone patch, your call.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
9:26 p.m.
New subject: [PATCH 8/9] util: don't use virPCIGetSysfsFile()
virPCIDeviceAddressGetSysfsFile() is simpler to call.
Signed-off-by: Laine Stump <laine(a)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
3:41 a.m.
New subject: [PATCH 8/9] util: don't use virPCIGetSysfsFile()
On Tue, Oct 20, 2020 at 10:26:08PM -0400, Laine Stump wrote:
virPCIDeviceAddressGetSysfsFile() is simpler to call.
Signed-off-by: Laine Stump <laine(a)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) {
new line ^here please
virReportSystemError(ENOSYS, "%s",
_("Failed to get PCI SYSFS file"));
goto cleanup;
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
9:26 p.m.
New subject: [PATCH 9/9] util: remove unused function virPCIGetSysfsFile()
Signed-off-by: Laine Stump <laine(a)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
3:42 a.m.
On Tue, Oct 20, 2020 at 10:26:00PM -0400, Laine Stump wrote:
These started with a couple small cleanups to functions I was going
to
change the API for, but then I decided not to change the API, and just
didn't want to waste the effort spent cleaning them up.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:47 a.m.
On Tue, Oct 20, 2020 at 22:26:00 -0400, Laine Stump wrote:
These started with a couple small cleanups to functions I was going
to
change the API for, but then I decided not to change the API, and just
didn't want to waste the effort spent cleaning them up.
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
11:39 a.m.
On a Tuesday in 2020, Laine Stump wrote:
These started with a couple small cleanups to functions I was going
to
change the API for, but then I decided not to change the API, and just
didn't want to waste the effort spent cleaning them up.
Thank you.
Laine Stump (9):
util: simplify virHostdevPCISysfsPath()
util: simplify virPCIFile() and its callers
util: simplify virPCIDriverDir() and its callers
util: simplify virPCIProbeStubDriver()
util: avoid manual VIR_FREE of a g_autofree pointer in virPCIGetName()
util: use more g_autofree in virpci.c
util: remove unneeded cleanup:/ret in virpci.c
util: don't use virPCIGetSysfsFile()
util: remove unused function virPCIGetSysfsFile()
src/hypervisor/virhostdev.c | 10 +--
src/libvirt_private.syms | 1 -
src/util/virnetdev.c | 6 +-
src/util/virpci.c | 146 ++++++++++++------------------------
src/util/virpci.h | 4 -
5 files changed, 53 insertions(+), 114 deletions(-)
Beautiful.
Jano
--
2.26.2
1488
days inactive
1493
days old
15 comments
4 participants
participants (4)
-
Erik Skultety -
Ján Tomko -
Laine Stump -
Peter Krempa