Re: [libvirt PATCH 9/9] ch: use g_auto in virCHMonitorNew

[Laine Stump <laine@xxxxxxxxxx>]

On 9/22/21 4:55 PM, Ján Tomko wrote:
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
>   src/ch/ch_monitor.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 3504c21f9d..804704e66d 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -445,9 +445,8 @@ chMonitorCreateSocket(const char *socket_path)
>   virCHMonitor *
>   virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>   {
> -    virCHMonitor *ret = NULL;
>       virCHMonitor *mon = NULL;
> -    virCommand *cmd = NULL;
> +    g_autoptr(virCommand) cmd = NULL;
>       int socket_fd = 0;
>   
>       if (virCHMonitorInitialize() < 0)
> @@ -468,7 +467,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>           virReportSystemError(errno,
>                                _("Cannot create socket directory '%s'"),
>                                socketdir);
> -        goto cleanup;
> +        return NULL;

Again, it's pre-existing, but it looks to me like "mon" is leaked when 
there is any error in this function.

>       }
>   
>       cmd = virCommandNew(vm->def->emulator);
> @@ -478,7 +477,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>           virReportSystemError(errno,
>                                _("Cannot create socket '%s'"),
>                                mon->socketpath);
> -        goto cleanup;
> +        return NULL;
>       }
>   
>       virCommandAddArg(cmd, "--api-socket");
> @@ -487,7 +486,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>   
>       /* launch Cloud-Hypervisor socket */
>       if (virCommandRunAsync(cmd, &mon->pid) < 0)
> -        goto cleanup;
> +        return NULL;
>   
>       /* get a curl handle */
>       mon->handle = curl_easy_init();
> @@ -496,11 +495,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>       virObjectRef(mon);

Hmm, actually it's *always* leaked:

Also pre-existing, but the virObjectRef(mon) here seems to be 
superfluous and will cause the monitor object to never be disposed. I 
guess, based on the comment that immediately precedes it, that this 
virObjectRef() is intended to be the ref for the object pointer that 
will now be returned from virCHMonitorNew(), but the object already has 
1 ref just by virtue of being created, and that ref isn't being removed 
during cleanup, so the object will have 2 refs on return.

I think instead there should be a g_auto cleanup defined for virCHMonitor:

 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCHMonitor, virObjectUnref);

then mon should be declared as

    g_autoptr(virCHMonitor) mon = NULL;

and finally, instead of having the extra virObjectRef(mon) once success 
is assured, the return should be done with:

    return g_steal_pointer(&mon);

But this is all fixing an existing bug, so maybe it should be done as a 
separate prerequisite patch. It's up to you.


>       mon->vm = virObjectRef(vm);
>   
> -    ret = mon;
> -
> - cleanup:
> -    virCommandFree(cmd);
> -    return ret;
> +    return mon;
>   }
>   
>   static void virCHMonitorDispose(void *opaque)