[PATCH v2 1/2] util: add virNetDevGetPhysPortName

[Laine Stump]

On 1/21/21 7:15 AM, Moshe Levi wrote:
> This commit add virNetDevGetPhysPortName to read netdevice
> phys_port_name from sysfs. It also refactor the code so
> virNetDevGetPhysPortName and virNetDevGetPhysPortID will use
> same method to read the netdevice sysfs.
> ---
>   src/util/virnetdev.c | 74 +++++++++++++++++++++++++++++++++++---------
>   src/util/virnetdev.h |  4 +++
>   2 files changed, 63 insertions(+), 15 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index a73e5f72f1..d41b967d6a 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1147,6 +1147,29 @@ virNetDevGetPCIDevice(const char *devName)
>   # endif
>   
>   
> +/* A wrapper to get content of file from ifname SYSFS_NET_DIR
> + */
> +static int
> +virNetDevGetSysfsFileValue(const char *ifname,
> +                           const char *fileName,
> +                           char **sysfsFileData)
> +{
> +    g_autofree char *sysfsFile = NULL;
> +
> +    *sysfsFileData = NULL;
> +
> +    if (virNetDevSysfsFile(&sysfsFile, ifname, fileName) < 0)
> +        return -1;
> +
> +    /* a failure to read just means the driver doesn't support
> +     * <fileName>, so set success now and ignore the return from
> +     * virFileReadAllQuiet().
> +     */
> +
> +    ignore_value(virFileReadAllQuiet(sysfsFile, 1024, sysfsFileData));
> +    return 0;
> +}
> +
>   /**
>    * virNetDevGetPhysPortID:
>    *
> @@ -1165,20 +1188,29 @@ int
>   virNetDevGetPhysPortID(const char *ifname,
>                          char **physPortID)
>   {
> -    g_autofree char *physPortIDFile = NULL;
> -
> -    *physPortID = NULL;
> -
> -    if (virNetDevSysfsFile(&physPortIDFile, ifname, "phys_port_id") < 0)
> -        return -1;
> +    return virNetDevGetSysfsFileValue(ifname, "phys_port_id", physPortID);
> +}
>   
> -    /* a failure to read just means the driver doesn't support
> -     * phys_port_id, so set success now and ignore the return from
> -     * virFileReadAllQuiet().
> -     */
>   
> -    ignore_value(virFileReadAllQuiet(physPortIDFile, 1024, physPortID));
> -    return 0;
> +/**
> + * virNetDevGetPhysPortName:
> + *
> + * @ifname: name of a netdev
> + *
> + * @physPortName: pointer to char* that will receive @ifname's
> + *                phys_port_name from sysfs (null terminated
> + *                string). Could be NULL if @ifname's net driver doesn't
> + *                support phys_port_name (most netdev drivers
> + *                don't). Caller is responsible for freeing the string
> + *                when finished.
> + *
> + * Returns 0 on success or -1 on failure.
> + */
> +int
> +virNetDevGetPhysPortName(const char *ifname,
> +                         char **physPortName)
> +{
> +    return virNetDevGetSysfsFileValue(ifname, "phys_port_name", physPortName);
>   }
>   
>   
> @@ -1231,7 +1263,7 @@ virNetDevGetVirtualFunctions(const char *pfname,
>           }
>   
>           if (virPCIGetNetName(pci_sysfs_device_link, 0,
> -                             pfPhysPortID, &((*vfname)[i])) < 0) {
> +                             pfPhysPortID, NULL, &((*vfname)[i])) < 0) {


This (and all the other changes to the calling sequence of 
virPCIGetNetName()) shouldn't be in this patch - not only are they not 
relevant to the patch's purpose, but you've eliminated the regex 
argument in this spin of the patches, so you ended up just changing 
these lines back in the next patch.


I'll remove these changes before I push.


Reviewed-by: Laine Stump <laine at redhat.com>


>               goto cleanup;
>           }
>   
> @@ -1326,7 +1358,8 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
>           return -1;
>   
>       if (virPCIGetNetName(physfn_sysfs_path, 0,
> -                         vfPhysPortID, pfname) < 0) {
> +                         vfPhysPortID,
> +                         VIR_PF_PHYS_PORT_NAME_REGEX, pfname) < 0) {
>           return -1;
>       }
>   
> @@ -1389,7 +1422,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
>        * isn't bound to a netdev driver, it won't have a netdev name,
>        * and vfname will be NULL).
>        */
> -    return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
> +    return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, NULL, vfname);
>   }
>   
>   
> @@ -1433,6 +1466,17 @@ virNetDevGetPhysPortID(const char *ifname G_GNUC_UNUSED,
>       return 0;
>   }
>   
> +int
> +virNetDevGetPhysPortName(const char *ifname G_GNUC_UNUSED,
> +                       char **physPortName)
> +{
> +    /* this actually should never be called, and is just here to
> +     * satisfy the linker.
> +     */
> +    *physPortName = NULL;
> +    return 0;
> +}
> +
>   int
>   virNetDevGetVirtualFunctions(const char *pfname G_GNUC_UNUSED,
>                                char ***vfname G_GNUC_UNUSED,
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index f016012718..e9349e7f59 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -250,6 +250,10 @@ int virNetDevGetPhysPortID(const char *ifname,
>                              char **physPortID)
>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
>       G_GNUC_WARN_UNUSED_RESULT;
> +int virNetDevGetPhysPortName(const char *ifname,
> +                           char **physPortName)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> +    G_GNUC_WARN_UNUSED_RESULT;
>   
>   int virNetDevGetVirtualFunctions(const char *pfname,
>                                    char ***vfname,