1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/5] viriscsi: Couple of fixes
  4. View patch

[libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment

Michal Privoznik posted 5 patches 7 years, 7 months ago
There is a newer version of this series
[libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment
Posted by Michal Privoznik 7 years, 7 months ago 
The return value of this function is overwritten more times than
I am comfortable with.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/viriscsi.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
index d4c745a1af..a308ee4bac 100644
--- a/src/util/viriscsi.c
+++ b/src/util/viriscsi.c
@@ -117,15 +117,16 @@ static int
 virStorageBackendIQNFound(const char *initiatoriqn,
                           char **ifacename)
 {
-    int ret = IQN_MISSING, fd = -1;
+    int ret = IQN_ERROR, fd = -1;
     char ebuf[64];
     FILE *fp = NULL;
     char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
     virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
                                              "--mode", "iface", NULL);
 
+    *ifacename = NULL;
+
     if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
-        ret = IQN_ERROR;
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Could not allocate memory for output of '%s'"),
                        ISCSIADM);
@@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn,
     memset(line, 0, LINE_SIZE);
 
     virCommandSetOutputFD(cmd, &fd);
-    if (virCommandRunAsync(cmd, NULL) < 0) {
-        ret = IQN_ERROR;
+    if (virCommandRunAsync(cmd, NULL) < 0)
         goto out;
-    }
 
     if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to open stream for file descriptor "
                          "when reading output from '%s': '%s'"),
                        ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
-        ret = IQN_ERROR;
         goto out;
     }
 
     while (fgets(line, LINE_SIZE, fp) != NULL) {
         newline = strrchr(line, '\n');
         if (newline == NULL) {
-            ret = IQN_ERROR;
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Unexpected line > %d characters "
                              "when parsing output of '%s'"),
@@ -169,27 +166,27 @@ virStorageBackendIQNFound(const char *initiatoriqn,
         if (STREQ(iqn, initiatoriqn)) {
             token = strchr(line, ' ');
             if (!token) {
-                ret = IQN_ERROR;
                 virReportError(VIR_ERR_INTERNAL_ERROR,
                                _("Missing space when parsing output "
                                  "of '%s'"), ISCSIADM);
                 goto out;
             }
-            if (VIR_STRNDUP(*ifacename, line, token - line) < 0) {
-                ret = IQN_ERROR;
+
+            if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
                 goto out;
-            }
+
             VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
-            ret = IQN_FOUND;
             break;
         }
     }
 
     if (virCommandWait(cmd, NULL) < 0)
-        ret = IQN_ERROR;
+        goto out;
+
+    ret = *ifacename ? IQN_FOUND : IQN_MISSING;
 
  out:
-    if (ret == IQN_MISSING)
+    if (ret != IQN_FOUND)
         VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
 
     VIR_FREE(line);
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] virStorageBackendIQNFound: Fix ret value assignment
Posted by John Ferlan 7 years, 7 months ago 

On 06/29/2018 11:01 AM, Michal Privoznik wrote:
> The return value of this function is overwritten more times than
> I am comfortable with.

True, but let's actually say what you're doing...

Perform some method clean-up to follow more accepted coding standards:

 * Initialize @ret to error value and prove otherwise.
 * Initialize *ifacename to NULL

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/viriscsi.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 

Touching Dave's 8+ year old code (commit id 6aabcb5), lucky you.  I
believe this "feature" was never documented too - now that your name is
on top you can own it :-P!

> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index d4c745a1af..a308ee4bac 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -117,15 +117,16 @@ static int
>  virStorageBackendIQNFound(const char *initiatoriqn,
>                            char **ifacename)
>  {
> -    int ret = IQN_MISSING, fd = -1;
> +    int ret = IQN_ERROR, fd = -1;
>      char ebuf[64];
>      FILE *fp = NULL;
>      char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>      virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>                                               "--mode", "iface", NULL);
>  
> +    *ifacename = NULL;
> +
>      if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
> -        ret = IQN_ERROR;
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Could not allocate memory for output of '%s'"),
>                         ISCSIADM);
> @@ -135,24 +136,20 @@ virStorageBackendIQNFound(const char *initiatoriqn,
>      memset(line, 0, LINE_SIZE);
>  
>      virCommandSetOutputFD(cmd, &fd);
> -    if (virCommandRunAsync(cmd, NULL) < 0) {
> -        ret = IQN_ERROR;
> +    if (virCommandRunAsync(cmd, NULL) < 0)
>          goto out;
> -    }
>  
>      if ((fp = VIR_FDOPEN(fd, "r")) == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to open stream for file descriptor "
>                           "when reading output from '%s': '%s'"),
>                         ISCSIADM, virStrerror(errno, ebuf, sizeof(ebuf)));
> -        ret = IQN_ERROR;
>          goto out;
>      }
>  
>      while (fgets(line, LINE_SIZE, fp) != NULL) {
>          newline = strrchr(line, '\n');
>          if (newline == NULL) {
> -            ret = IQN_ERROR;
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unexpected line > %d characters "
>                               "when parsing output of '%s'"),
> @@ -169,27 +166,27 @@ virStorageBackendIQNFound(const char *initiatoriqn,
>          if (STREQ(iqn, initiatoriqn)) {
>              token = strchr(line, ' ');
>              if (!token) {
> -                ret = IQN_ERROR;
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Missing space when parsing output "
>                                   "of '%s'"), ISCSIADM);
>                  goto out;
>              }
> -            if (VIR_STRNDUP(*ifacename, line, token - line) < 0) {
> -                ret = IQN_ERROR;
> +
> +            if (VIR_STRNDUP(*ifacename, line, token - line) < 0)
>                  goto out;
> -            }
> +
>              VIR_DEBUG("Found interface '%s' with IQN '%s'", *ifacename, iqn);
> -            ret = IQN_FOUND;
>              break;
>          }
>      }
>  
>      if (virCommandWait(cmd, NULL) < 0)
> -        ret = IQN_ERROR;
> +        goto out;
> +
> +    ret = *ifacename ? IQN_FOUND : IQN_MISSING;
>  
>   out:

While you're at it s/out/cleanup

> -    if (ret == IQN_MISSING)
> +    if (ret != IQN_FOUND)

IDC since it's VIR_DEBUG, but you'll get this message on IQN_ERROR too
as well as the virReportError... I would think that's fairly obvious on
the error paths

With some minor cleanups...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

>          VIR_DEBUG("Could not find interface with IQN '%s'", iqn);
>  
>      VIR_FREE(line);
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.