[v2,2/4] util: Code simplification

Message ID 20181117195113.12544-3-radoslaw.biernacki@linaro.org
State New
Headers show
Series
  • util: Fixing libvirt errors on cavium/thunder-nicvf
Related show

Commit Message

Radoslaw Biernacki Nov. 17, 2018, 7:51 p.m.
Removing redundant sections of the code

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

---
 src/util/virnetdev.c | 40 +++-----------------
 1 file changed, 5 insertions(+), 35 deletions(-)

-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Michal Prívozník Nov. 20, 2018, 10:17 a.m. | #1
On 11/17/18 8:51 PM, Radoslaw Biernacki wrote:
> Removing redundant sections of the code

> 

> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>

> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

> ---

>  src/util/virnetdev.c | 40 +++-----------------

>  1 file changed, 5 insertions(+), 35 deletions(-)

> 

> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c

> index f1c2ba8c17..9318de406c 100644

> --- a/src/util/virnetdev.c

> +++ b/src/util/virnetdev.c

> @@ -1441,34 +1441,16 @@ int

>  virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,

>                                  int *vf)

>  {

> -    char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;

> -    int ret = -1;

> -

>      *pfname = NULL;

>  

>      if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {

>          virReportError(VIR_ERR_INTERNAL_ERROR,

>                         _("Cannot get PF netdev name for VF %s"),

>                         vfname);


Sigh, this is overwriting an error reported by
virNetDevGetPhysicalFunction(). But it is pre-existing,

> -        return ret;

> +        return -1;

>      }

>  

> -    if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)

> -        goto cleanup;

> -

> -    if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)

> -        goto cleanup;

> -

> -    ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);

> -

> - cleanup:

> -    if (ret < 0)

> -        VIR_FREE(*pfname);

> -

> -    VIR_FREE(vf_sysfs_path);

> -    VIR_FREE(pf_sysfs_path);

> -

> -    return ret;

> +    return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);


Just realized that previously if there was an error then *pfname was
NULL upon returning from the function. Now it might be allocated. How about:

    ret = virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);

    if (ret < 0)
        VIR_FREE(*pfname);

    return ret;


>  }

>  

>  #else /* !__linux__ */

> @@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,

>           * it to PF + VFname

>           */

>  

> -        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)

> +        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))

>              goto cleanup;

> -

>          pfDevName = pfDevOrig;

> -

> -        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)

> -            goto cleanup;

>      }

>  

>      if (pfDevName) {

> @@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,

>           * it to PF + VFname

>           */

>  

> -        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)

> +        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))


No, please keep the proper < 0 comparison.

>              goto cleanup;

> -

>          pfDevName = pfDevOrig;

> -

> -        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)

> -            goto cleanup;

>      }

>  

>      /* if there is a PF, it's now in pfDevName, and linkdev is either

> @@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,

>           * it to PF + VFname

>           */

>  

> -        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)

> +        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))


Here too.

>              goto cleanup;

> -

>          pfDevName = pfDevOrig;

> -

> -        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)

> -            goto cleanup;

>      }

>  

>  

> 


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index f1c2ba8c17..9318de406c 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1441,34 +1441,16 @@  int
 virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
                                 int *vf)
 {
-    char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
-    int ret = -1;
-
     *pfname = NULL;
 
     if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Cannot get PF netdev name for VF %s"),
                        vfname);
-        return ret;
+        return -1;
     }
 
-    if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
-        goto cleanup;
-
-    if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
-        goto cleanup;
-
-    ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
-
- cleanup:
-    if (ret < 0)
-        VIR_FREE(*pfname);
-
-    VIR_FREE(vf_sysfs_path);
-    VIR_FREE(pf_sysfs_path);
-
-    return ret;
+    return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
 }
 
 #else /* !__linux__ */
@@ -1863,13 +1845,9 @@  virNetDevSaveNetConfig(const char *linkdev, int vf,
          * it to PF + VFname
          */
 
-        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
             goto cleanup;
-
         pfDevName = pfDevOrig;
-
-        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
-            goto cleanup;
     }
 
     if (pfDevName) {
@@ -2021,13 +1999,9 @@  virNetDevReadNetConfig(const char *linkdev, int vf,
          * it to PF + VFname
          */
 
-        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
             goto cleanup;
-
         pfDevName = pfDevOrig;
-
-        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
-            goto cleanup;
     }
 
     /* if there is a PF, it's now in pfDevName, and linkdev is either
@@ -2226,13 +2200,9 @@  virNetDevSetNetConfig(const char *linkdev, int vf,
          * it to PF + VFname
          */
 
-        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+        if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
             goto cleanup;
-
         pfDevName = pfDevOrig;
-
-        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
-            goto cleanup;
     }