Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
---
 src/admin/libvirt-admin.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index 4099a54854..e272d7a422 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -111,7 +111,7 @@ getSocketPath(virURIPtr uri)
         virURIParamPtr param = &uri->params[i];
 
         if (STREQ(param->name, "socket")) {
-            VIR_FREE(sock_path);
+            g_free(sock_path);
             sock_path = g_strdup(param->value);
         } else {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -203,11 +203,11 @@ virAdmGetDefaultURI(virConfPtr conf, char **uristr)
 virAdmConnectPtr
 virAdmConnectOpen(const char *name, unsigned int flags)
 {
-    char *sock_path = NULL;
+    g_autofree char *sock_path = NULL;
     char *alias = NULL;
     virAdmConnectPtr conn = NULL;
     g_autoptr(virConf) conf = NULL;
-    char *uristr = NULL;
+    g_autofree char *uristr = NULL;
 
     if (virAdmInitialize() < 0)
         goto error;
@@ -233,7 +233,7 @@ virAdmConnectOpen(const char *name, unsigned int flags)
         goto error;
 
     if (alias) {
-        VIR_FREE(uristr);
+        g_free(uristr);
         uristr = alias;
     }
 
@@ -251,16 +251,11 @@ virAdmConnectOpen(const char *name, unsigned int flags)
     if (remoteAdminConnectOpen(conn, flags) < 0)
         goto error;
 
- cleanup:
-    VIR_FREE(sock_path);
-    VIR_FREE(uristr);
-    return conn;
 
  error:
     virDispatchError(NULL);
     virObjectUnref(conn);
-    conn = NULL;
-    goto cleanup;
+    return NULL;
 }
 
 /**
-- 
2.24.1

On Mon, Mar 02, 2020 at 07:22:03 +0530, Gaurav Agrawal wrote:
> Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
> ---
>  src/admin/libvirt-admin.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

[...]

> @@ -251,16 +251,11 @@ virAdmConnectOpen(const char *name, unsigned int flags)
>      if (remoteAdminConnectOpen(conn, flags) < 0)
>          goto error;
>  
> - cleanup:
> -    VIR_FREE(sock_path);
> -    VIR_FREE(uristr);
> -    return conn;

Removing this 'return' will make this function ALWAYS return NULL when
the control flow now continues through the error label.

>  
>   error:
>      virDispatchError(NULL);
>      virObjectUnref(conn);
> -    conn = NULL;
> -    goto cleanup;
> +    return NULL;
>  }

On review by Jan Tomko, for my earlier Patchv2, which had the return NULL
replace by the return conn ,
here is the review:
```
>+cleanup:
>     return conn;
>
>  error:
>     virDispatchError(NULL);

>-    virObjectUnref(conn);

'conn' is still not marked as g_auto, so this unref needs to stay.

>     conn = NULL;
>     goto cleanup;

These two lines can be replaced by return NULL, and the cleanup: label
above removed.

```

I went ahead with what this review meant, and initially my PATCHv1 also did
the same thing return NULL, because if there is an error the function is
supposed to return NULL anyways.



On Mon, Mar 2, 2020 at 2:08 PM Peter Krempa <pkrempa@redhat.com> wrote:

> On Mon, Mar 02, 2020 at 07:22:03 +0530, Gaurav Agrawal wrote:
> > Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
> > ---
> >  src/admin/libvirt-admin.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
>
> [...]
>
> > @@ -251,16 +251,11 @@ virAdmConnectOpen(const char *name, unsigned int
> flags)
> >      if (remoteAdminConnectOpen(conn, flags) < 0)
> >          goto error;
> >
> > - cleanup:
> > -    VIR_FREE(sock_path);
> > -    VIR_FREE(uristr);
> > -    return conn;
>
> Removing this 'return' will make this function ALWAYS return NULL when
> the control flow now continues through the error label.
>
> >
> >   error:
> >      virDispatchError(NULL);
> >      virObjectUnref(conn);
> > -    conn = NULL;
> > -    goto cleanup;
> > +    return NULL;
> >  }
>
>

On Mon, Mar 02, 2020 at 14:13:28 +0530, Gaurav Agrawal wrote:
> On review by Jan Tomko, for my earlier Patchv2, which had the return NULL

Please don't top-post on technical mailing lists.

> replace by the return conn ,

He told you the correct thing to do:

> here is the review:
> ```
> >+cleanup:
> >     return conn;

return conn MUST stay here. The function is returning a referenced 'conn
object on success.

> >
> >  error:
> >     virDispatchError(NULL);
> 
> >-    virObjectUnref(conn);
> 
> 'conn' is still not marked as g_auto, so this unref needs to stay.

On error 'conn' MUST be freed.

> 
> >     conn = NULL;
> >     goto cleanup;

And NULL must be returned on error. Since cleanup does nothing now other
than returning 'conn' which is NULL you can avoid the two lines by
returning NULL directly rather than jumping to 'cleanup'.

> 
> These two lines can be replaced by return NULL, and the cleanup: label
> above removed.

That's what's said here. There was no mention of changing what's
returned.

> 
> ```
> 
> I went ahead with what this review meant, and initially my PATCHv1 also did
> the same thing return NULL, because if there is an error the function is
> supposed to return NULL anyways.
> 
> 
> 
> On Mon, Mar 2, 2020 at 2:08 PM Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2020 at 07:22:03 +0530, Gaurav Agrawal wrote:
> > > Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
> > > ---
> > >  src/admin/libvirt-admin.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > [...]
> >
> > > @@ -251,16 +251,11 @@ virAdmConnectOpen(const char *name, unsigned int
> > flags)
> > >      if (remoteAdminConnectOpen(conn, flags) < 0)
> > >          goto error;
> > >
> > > - cleanup:
> > > -    VIR_FREE(sock_path);
> > > -    VIR_FREE(uristr);
> > > -    return conn;
> >
> > Removing this 'return' will make this function ALWAYS return NULL when
> > the control flow now continues through the error label.
> >
> > >
> > >   error:
> > >      virDispatchError(NULL);
> > >      virObjectUnref(conn);
> > > -    conn = NULL;
> > > -    goto cleanup;
> > > +    return NULL;
> > >  }
> >
> >