Ahem.. Hello? Is this thing on?? :-)
On 3/3/21 3:34 PM, Laine Stump wrote:
ping
On 2/23/21 10:35 PM, Laine Stump wrote:
> Some SRIOV PFs don't have a netdev associated with them (the spec
> apparently doesn't require it). In most cases when libvirt is dealing
> with an SRIOV VF, that VF must have a PF, and the PF *must* have an
> associated netdev (the only way to set the MAC address of a VF is by
> sending a netlink message to the netdev of that VF's PF). But there
> are times when we don't need for the PF to have a netdev; in
> particular, when we're just getting the Switchdev Features for a VF,
> we don't need the PF netdev - the netdev of the VF (apparently) works
> just as well.
>
> Commit 6452e2f5 (libvirt 5.1.0) *kind of* made libvirt work around PFs
> with no netdevs in this case - if virNetDevGetPhysicalFunction
> returned an error when setting up to retrieve Switchdev feature info,
> it would ignore the error, and then check if the PF netdev name was
> NULL and, if so it would reset the error object and continue on rather
> than returning early with a failure. The problem is that by the time
> this special handling occured, the error message about missing netdev
> had already been logged, which was harmless to proper operation, but
> confused the user.
>
> Fortunately there are only 2 users of virNetDevGetPhysicalFunction, so
> it is easy to redefine it's API to state that a missing netdev name is
> *not* an error - in that case it will still return success, but the
> caller must be prepared for the PF netdev name to be NULL. After
> making this change, we can modify the two callers to behave properly
> with the new semantics (for one of the callers it *is* still an error,
> so the error message is moved there, but for the other it is okay to
> continue), and our spurious error messages are a thing of the past.
>
> Resolves:
https://bugzilla.redhat.com/1924616
> Fixes: 6452e2f5e1837bd753ee465e6607ed3c4f62b815
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/util/virnetdev.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 2b4c8b6280..7b766234ec 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1339,7 +1339,8 @@ virNetDevGetVirtualFunctionIndex(const char
> *pfname, const char *vfname,
> *
> * @ifname : name of the physical function interface name
> * @pfname : Contains sriov physical function for interface ifname
> - * upon successful return
> + * upon successful return (might be NULL if the PF has no
> + * associated netdev. This is *not* an error)
> *
> * Returns 0 on success, -1 on failure
> *
> @@ -1361,15 +1362,6 @@ virNetDevGetPhysicalFunction(const char
> *ifname, char **pfname)
> return -1;
> }
> - if (!*pfname) {
> - /* The SRIOV standard does not require VF netdevs to have
> - * the netdev assigned to a PF. */
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("The PF device for VF %s has no network
> device name"),
> - ifname);
> - return -1;
> - }
> -
> return 0;
> }
> @@ -1442,6 +1434,17 @@ virNetDevGetVirtualFunctionInfo(const char
> *vfname, char **pfname,
> if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
> return -1;
> + if (!*pfname) {
> + /* The SRIOV standard does not require VF netdevs to have the
> + * netdev assigned to a PF, but our method of retrieving
> + * VFINFO does.
> + */
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("The PF device for VF %s has no network
> device name, cannot get virtual function info"),
> + vfname);
> + return -1;
> + }
> +
> if (virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf) < 0)
> goto cleanup;
> @@ -3204,12 +3207,8 @@ virNetDevSwitchdevFeature(const char *ifname,
> if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
> return ret;
> - if (is_vf == 1) {
> - /* Ignore error if PF does not have netdev assigned.
> - * In that case pfname == NULL. */
> - if (virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
> - virResetLastError();
> - }
> + if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) <
0)
> + return ret;
> pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
> virNetDevGetPCIDevice(ifname);
>