Commit 795e9e05c3 (libvirt-7.7.0) refactored the code in virpci.c and virnetdev.c that gathered lists of the Virtual Functions (VF) of an SRIOV Physical Function (PF) to simplify the code. Unfortunately the simplification made the assumption that a VF's netdev interface name should only be retrieved if the PF had a valid phys_port_id. That is an incorrect assumption - only a small handful of (now previous-generation) Mellanox SRIOV cards actually use phys_port_id (this is for an odd design where there are multiple physical network ports on a single PCI address); all other SRIOV cards (including new Mellanox cards) have a file in sysfs called phys_port_id, but it can't be read, and so the pfPhysPortID string is NULL. The result of this logic error is that virtual networks that are a pool of VFs to be used for macvtap connections will be unable to start, giving an errror like this: VF 0 of SRIOV PF enp130s0f0 couldn't be added to the interface pool because it isn't bound to a network driver - possibly in use elsewhere The simple/quick solution to this is to not imply that "pfPhysPortID == NULL" is the same as "don't fill in the VF's netdev ifname". Instead, add a bool getIfName to the args of virNetDevGetVirtualFunctionsFull() so that we can still get the ifname filled in when pfPhysPortID is NULL. Resolves: https://bugzilla.redhat.com/2025432 Fixes: 795e9e05c3b6b9ef3abe6f6078a6373a136ec23b Signed-off-by: Laine Stump <laine@xxxxxxxxxx> --- On one hand this is a regression with an apparently simple fix (that has been tested to solve the problem), and it's always good to fix regressions before a release rather than after. On the other hand it has been broken since August, and nobody complained until last week (and that was a QE tester, not an actual end-user), so it seems the bug is in functionality that isn't used much in the field (or at least no downstream with a used of the functionality has made a release since then that was installed by said user). I've grown increasingly wary of pushing anything just before a release, especially when it modifies the args of a function that has multiple definitions for different platforms (although CI is supposed to be thorough enough to catch those types of problems these days). So I'm all for pushing this right after the release, rather than before. But if anyone sees a reason for doing otherwise, we can talk about it in about 10 hours when I sit down at the keyboard again :-). P.S. I'm planning to make a followup that eliminates the pfPhysPortID arg completely, but wanted the bugfix to be as stripped down as possible. src/util/virnetdev.c | 2 +- src/util/virpci.c | 20 ++++++++++++-------- src/util/virpci.h | 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 58f7360a0f..300d7e4f45 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1231,7 +1231,7 @@ virNetDevGetVirtualFunctions(const char *pfname, if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) return -1; - if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, vfs, pfPhysPortID) < 0) + if (virPCIGetVirtualFunctionsFull(pf_sysfs_device_link, true, vfs, pfPhysPortID) < 0) return -1; return 0; diff --git a/src/util/virpci.c b/src/util/virpci.c index 2d12e28004..b7b987dd63 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2263,7 +2263,7 @@ int virPCIGetVirtualFunctions(const char *sysfs_path, virPCIVirtualFunctionList **vfs) { - return virPCIGetVirtualFunctionsFull(sysfs_path, vfs, NULL); + return virPCIGetVirtualFunctionsFull(sysfs_path, false, vfs, NULL); } @@ -2339,15 +2339,18 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, /** * virPCIGetVirtualFunctionsFull: * @sysfs_path: path to physical function sysfs entry + * @getIfName: true if the ifname of the VF should also be filled in, + * false to only fill in the PCI address. * @vfs: filled with the virtual function data - * @pfPhysPortID: Optional physical port id. If provided the network interface - * name of the VFs is queried too. + * @pfPhysPortID: Optional physical port id. if non-null it will be used + * when determining the ifname. * * * Returns virtual functions of a physical function. */ int virPCIGetVirtualFunctionsFull(const char *sysfs_path, + bool getIfName, virPCIVirtualFunctionList **vfs, const char *pfPhysPortID) { @@ -2390,11 +2393,10 @@ virPCIGetVirtualFunctionsFull(const char *sysfs_path, return -1; } - if (pfPhysPortID) { - if (virPCIGetNetName(device_link, 0, pfPhysPortID, &fnc.ifname) < 0) { - g_free(fnc.addr); - return -1; - } + if (getIfName && + virPCIGetNetName(device_link, 0, pfPhysPortID, &fnc.ifname) < 0) { + g_free(fnc.addr); + return -1; } VIR_APPEND_ELEMENT(list->functions, list->nfunctions, fnc); @@ -2711,8 +2713,10 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path G_GNUC_UNUSED, int virPCIGetVirtualFunctionsFull(const char *sysfs_path G_GNUC_UNUSED, + bool getIfName G_GNUC_UNUSED, virPCIVirtualFunctionList **vfs G_GNUC_UNUSED, const char *pfPhysPortID G_GNUC_UNUSED) + { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); return -1; diff --git a/src/util/virpci.h b/src/util/virpci.h index 3346321ec9..2f64c447cc 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -229,6 +229,7 @@ void virPCIVirtualFunctionListFree(virPCIVirtualFunctionList *list); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVirtualFunctionList, virPCIVirtualFunctionListFree); int virPCIGetVirtualFunctionsFull(const char *sysfs_path, + bool getIfName, virPCIVirtualFunctionList **vfs, const char *pfPhysPortID); int virPCIGetVirtualFunctions(const char *sysfs_path, -- 2.33.1
