In virCHMontiorNew the monitor object is referenced an additional time
incorrectly preventing it from being disposed of. Because the disposal
wasn't being used, a bug in virCHMonitorClose that would incorrectly
unref the domain object wasn't being seen. This change fixes both.

Signed-off-by: William Douglas <william.douglas@intel.com>
---
 src/ch/ch_monitor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index a1430f0e65..800457af41 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -468,7 +468,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
     if (!vm->def) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("VM is not defined"));
-        return NULL;
+        goto cleanup;
     }
 
     /* prepare to launch Cloud-Hypervisor socket */
@@ -502,12 +502,14 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
     mon->handle = curl_easy_init();
 
     /* now has its own reference */
-    virObjectRef(mon);
     mon->vm = virObjectRef(vm);
 
     ret = mon;
+    mon = NULL;
 
  cleanup:
+    if (mon)
+        virCHMonitorClose(mon);
     virCommandFree(cmd);
     return ret;
 }
@@ -542,7 +544,6 @@ void virCHMonitorClose(virCHMonitor *mon)
         g_free(mon->socketpath);
     }
 
-    virObjectUnref(mon->vm);
     virObjectUnref(mon);
 }
 
-- 
2.33.0

On 10/1/21 2:12 PM, William Douglas wrote:
> In virCHMontiorNew the monitor object is referenced an additional time
> incorrectly preventing it from being disposed of. Because the disposal
> wasn't being used, a bug in virCHMonitorClose that would incorrectly
> unref the domain object wasn't being seen. This change fixes both.

Although each is very small, the fixes should be in separate patches 
since they are separate and unconnected problems. If it's okay with you, 
I'll just split this patch and adjust the log comments (while still 
attributing to you) before pushing. Or if you'd rather, you can resend 
two separate patches for it, with log messages of your choice. Let me 
know which you prefer.

Otherwise:

Reviewed-by: Laine Stump <laine@redhat.com>

> 
> Signed-off-by: William Douglas <william.douglas@intel.com>
> ---
>   src/ch/ch_monitor.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index a1430f0e65..800457af41 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -468,7 +468,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>       if (!vm->def) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("VM is not defined"));
> -        return NULL;
> +        goto cleanup;
>       }
>   
>       /* prepare to launch Cloud-Hypervisor socket */
> @@ -502,12 +502,14 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir)
>       mon->handle = curl_easy_init();
>   
>       /* now has its own reference */
> -    virObjectRef(mon);
>       mon->vm = virObjectRef(vm);
>   
>       ret = mon;
> +    mon = NULL;
>   
>    cleanup:
> +    if (mon)
> +        virCHMonitorClose(mon);
>       virCommandFree(cmd);
>       return ret;
>   }
> @@ -542,7 +544,6 @@ void virCHMonitorClose(virCHMonitor *mon)
>           g_free(mon->socketpath);
>       }
>   
> -    virObjectUnref(mon->vm);
>       virObjectUnref(mon);
>   }
>   
> 

On 10/3/21 3:43 PM, Laine Stump wrote:
>     cleanup:
> +    if (mon)
> +        virCHMonitorClose(mon);

Oops, I also meant to point out that the "if (mon)" is unnecessary here, 
because (as with all similar functions in libvirt) virCHMonitorClose() 
can be called with a null argument, and will just be a NOP in that case. 
If you want me to split and push, I'll fix that up before pushing too.

On Sun, 2021-10-03 at 15:52 -0400, Laine Stump wrote:
> On 10/3/21 3:43 PM, Laine Stump wrote:
> >     cleanup:
> > +    if (mon)
> > +        virCHMonitorClose(mon);
> 
> Oops, I also meant to point out that the "if (mon)" is unnecessary
> here, 
> because (as with all similar functions in libvirt)
> virCHMonitorClose() 
> can be called with a null argument, and will just be a NOP in that
> case. 
> If you want me to split and push, I'll fix that up before pushing
> too.
> 

Ah oops, thanks for pointing that out. I'll gladly take advantage of
your offer to split them up and push, thanks!

On 10/3/21 10:10 PM, Douglas, William wrote:
> On Sun, 2021-10-03 at 15:52 -0400, Laine Stump wrote:
>> On 10/3/21 3:43 PM, Laine Stump wrote:
>>>      cleanup:
>>> +    if (mon)
>>> +        virCHMonitorClose(mon);
>>
>> Oops, I also meant to point out that the "if (mon)" is unnecessary
>> here,
>> because (as with all similar functions in libvirt)
>> virCHMonitorClose()
>> can be called with a null argument, and will just be a NOP in that
>> case.
>> If you want me to split and push, I'll fix that up before pushing
>> too.
>>
> 
> Ah oops, thanks for pointing that out. I'll gladly take advantage of
> your offer to split them up and push, thanks!
> 

OKay, I've pushed everything now. Thanks for following up!