[libvirt PATCH] util: don't log error if SRIOV PF has no associated netdev

[Laine Stump]

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 at 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);
>