Re: [PATCH v4] admin: use g_autofree

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2020, Daniel Henrique Barboza wrote:
> On 3/4/20 3:06 PM, Gaurav Agrawal wrote:
> > Signed-off-by: Gaurav Agrawal <agrawalgaurav@xxxxxxxxx>
> > ---
> >  src/admin/libvirt-admin.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
> > index 4099a54854..709e009467 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,13 @@ 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;
>
>
> virObjectUnref(conn) will end up turning conn to NULL via 'VIR_FREE()', given
> that 'conn' is guaranteed to have only one reference at this point (it was
> just created via virAdmConnectNew() in this function).

This is only partially correct.

The virAdmConnect structure at the address 'conn' does have only one
reference and the virObjectUnref call will call free() on that address
after it sets the reference count to zero.

Our 'VIR_FREE' wrapper over the 'free' function does set the pointer it
frees to NULL. But it does so by taking a pointer to the pointer to the
freed data as a parameter.

virObjectUnref only takes a pointer to the data. So the VIR_FREE call
can only set the 'obj' pointer inside the virObjectUnref to NULL. The
original 'conn' variable here stays unchanged.

> This 'conn = NULL' assignment was probably being used here to make it clearer
> that the 'goto cleanup' jump would return NULL in this situation. Since you're
> removing the need for the jump and the label, this 'conn = NULL' assignment is
> now unneeded.

All that being said, the assignment is not needed anymore so I've
removed it before pushing the patch.

> The rest of the code LGTM. If the maintainer does not mind removing the 'conn = NULL'
> when pushing the patch, here's my r-b:
>
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
and pushed now.

Jano

Attachment: signature.asc
Description: PGP signature