Right now, there is a lot of exit points from the function.
Depending on their position they need to copy the same free
calls. This goes against our style where we usually have just one
exit point from the function which also does the necessary free.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 daemon/libvirtd.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index d17a694c9..db239f0d4 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -262,50 +262,47 @@ daemonUnixSocketPaths(struct daemonConfig *config,
                       char **rosockfile,
                       char **admsockfile)
 {
+    int ret = -1;
+    char *rundir = NULL;
+
     if (config->unix_sock_dir) {
         if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir) < 0)
-            goto error;
+            goto cleanup;
 
         if (privileged) {
-            if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0)
-                goto error;
-            if (virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0)
-                goto error;
+            if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0 ||
+                virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0)
+                goto cleanup;
         }
     } else {
         if (privileged) {
             if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock") < 0 ||
                 VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro") < 0 ||
                 VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/libvirt-admin-sock") < 0)
-                goto error;
+                goto cleanup;
         } else {
-            char *rundir = NULL;
             mode_t old_umask;
 
             if (!(rundir = virGetUserRuntimeDirectory()))
-                goto error;
+                goto cleanup;
 
             old_umask = umask(077);
             if (virFileMakePath(rundir) < 0) {
                 umask(old_umask);
-                VIR_FREE(rundir);
-                goto error;
+                goto cleanup;
             }
             umask(old_umask);
 
             if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
-                virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0) {
-                VIR_FREE(rundir);
-                goto error;
-            }
-
-            VIR_FREE(rundir);
+                virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0)
+                goto cleanup;
         }
     }
-    return 0;
 
- error:
-    return -1;
+    ret = 0;
+ cleanup:
+    VIR_FREE(rundir);
+    return ret;
 }
 
 
-- 
2.13.0

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

On Tue, Jun 13, 2017 at 03:09:47PM +0200, Michal Privoznik wrote:
> Right now, there is a lot of exit points from the function.
> Depending on their position they need to copy the same free
> calls. This goes against our style where we usually have just one
> exit point from the function which also does the necessary free.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  daemon/libvirtd.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index d17a694c9..db239f0d4 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -262,50 +262,47 @@ daemonUnixSocketPaths(struct daemonConfig *config,
>                        char **rosockfile,
>                        char **admsockfile)
>  {
> +    int ret = -1;
> +    char *rundir = NULL;
> +
>      if (config->unix_sock_dir) {
>          if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir) < 0)
> -            goto error;
> +            goto cleanup;
>
>          if (privileged) {
> -            if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0)
> -                goto error;
> -            if (virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0)
> -                goto error;
> +            if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0 ||
> +                virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0)
> +                goto cleanup;

So, we tend to wrap the lines at 80 chars, but we don't enforce it anywhere, we
just recommend it. Now, in this particular case, although over, I must admit
that it's easier to read, it's much more self-contained so to speak.

>          }
>      } else {
>          if (privileged) {
>              if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock") < 0 ||
>                  VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro") < 0 ||
>                  VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/libvirt-admin-sock") < 0)

Same here.

> -                goto error;
> +                goto cleanup;
>          } else {
> -            char *rundir = NULL;
>              mode_t old_umask;
>
>              if (!(rundir = virGetUserRuntimeDirectory()))
> -                goto error;
> +                goto cleanup;
>
>              old_umask = umask(077);
>              if (virFileMakePath(rundir) < 0) {
>                  umask(old_umask);
> -                VIR_FREE(rundir);
> -                goto error;
> +                goto cleanup;
>              }
>              umask(old_umask);
>
>              if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
> -                virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0) {
> -                VIR_FREE(rundir);
> -                goto error;
> -            }
> -
> -            VIR_FREE(rundir);
> +                virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0)
> +                goto cleanup;
>          }
>      }
> -    return 0;
>
> - error:
> -    return -1;
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(rundir);
> +    return ret;
>  }
>

So ACK, even with the longer lines.

Erik

>
> --
> 2.13.0
>

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