[PATCH 0/5] ch: Fixup object leaks and extend g_auto usage
Based off of feedback from Laine to Ján's series of patches increasing g_auto usage for the ch_driver. This fixes leaks in virCHMonitorBuildKernelRelatedJson and virCHMonitorNew while tidying up and increasing the automatic cleanup for those functions and virCHMonitorBuildMemoryJson. William Douglas (5): ch_monitor: Stop leaking json value objects ch_monitor: Correctly close and ref the virCHMonitor ch: use g_auto in virCHMonitorBuildMemoryJson ch: use g_auto in virCHMonitorBuildKernelRelatedJson ch: use g_auto in virCHMonitorNew src/ch/ch_monitor.c | 59 +++++++++++++++------------------------------ src/ch/ch_monitor.h | 1 + 2 files changed, 20 insertions(+), 40 deletions(-) -- 2.33.0
In virCHMonitorBuildKernelRelatedJson there are two cases of json value objects being lost after the pointer being redefined. This change removes the needless redefinition. Signed-off-by: William Douglas <william.douglas@intel.com> --- src/ch/ch_monitor.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 5ed26a574f..a1430f0e65 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -121,7 +121,6 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) _("Kernel image path in this domain is not defined")); goto cleanup; } else { - kernel = virJSONValueNewObject(); if (virJSONValueObjectAppendString(kernel, "path", vmdef->os.kernel) < 0) goto cleanup; if (virJSONValueObjectAppend(content, "kernel", &kernel) < 0) @@ -136,7 +135,6 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) } if (vmdef->os.initrd != NULL) { - initramfs = virJSONValueNewObject(); if (virJSONValueObjectAppendString(initramfs, "path", vmdef->os.initrd) < 0) goto cleanup; if (virJSONValueObjectAppend(content, "initramfs", &initramfs) < 0) -- 2.33.0
On 10/1/21 2:12 PM, William Douglas wrote:
In virCHMonitorBuildKernelRelatedJson there are two cases of json value objects being lost after the pointer being redefined. This change removes the needless redefinition.
Signed-off-by: William Douglas <william.douglas@intel.com>
Reviewed-by: Laine Stump <laine@redhat.com>
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!
Signed-off-by: William Douglas <william.douglas@intel.com> --- src/ch/ch_monitor.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 800457af41..7326ac645b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -154,22 +154,17 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *memory; unsigned long long total_memory = virDomainDefGetMemoryInitial(vmdef) * 1024; if (total_memory != 0) { - memory = virJSONValueNewObject(); + g_autoptr(virJSONValue) memory = virJSONValueNewObject(); if (virJSONValueObjectAppendNumberUlong(memory, "size", total_memory) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "memory", &memory) < 0) - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(memory); - return -1; } static int -- 2.33.0
On 10/1/21 2:12 PM, William Douglas wrote:
Signed-off-by: William Douglas <william.douglas@intel.com> --- src/ch/ch_monitor.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 800457af41..7326ac645b 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -154,22 +154,17 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *memory; unsigned long long total_memory = virDomainDefGetMemoryInitial(vmdef) * 1024;
if (total_memory != 0) { - memory = virJSONValueNewObject(); + g_autoptr(virJSONValue) memory = virJSONValueNewObject();
there should be an extra empy line here between variable definition and the start of instructions.
if (virJSONValueObjectAppendNumberUlong(memory, "size", total_memory) < 0) - goto cleanup; + return -1;
also an empty line here makes the code more readable.
if (virJSONValueObjectAppend(content, "memory", &memory) < 0) - goto cleanup; + return -1; }
return 0; - - cleanup: - virJSONValueFree(memory); - return -1; }
static int
Reviewed-by: Laine Stump <laine@redhat.com> I'll add the extra empty lines before pushing.
Signed-off-by: William Douglas <william.douglas@intel.com> --- src/ch/ch_monitor.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 7326ac645b..bb49c70069 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -112,43 +112,36 @@ virCHMonitorBuildPTYJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *kernel = virJSONValueNewObject(); - virJSONValue *cmdline = virJSONValueNewObject(); - virJSONValue *initramfs = virJSONValueNewObject(); + g_autoptr(virJSONValue) kernel = virJSONValueNewObject(); + g_autoptr(virJSONValue) cmdline = virJSONValueNewObject(); + g_autoptr(virJSONValue) initramfs = virJSONValueNewObject(); if (vmdef->os.kernel == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel image path in this domain is not defined")); - goto cleanup; + return -1; } else { if (virJSONValueObjectAppendString(kernel, "path", vmdef->os.kernel) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "kernel", &kernel) < 0) - goto cleanup; + return -1; } if (vmdef->os.cmdline) { if (virJSONValueObjectAppendString(cmdline, "args", vmdef->os.cmdline) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "cmdline", &cmdline) < 0) - goto cleanup; + return -1; } if (vmdef->os.initrd != NULL) { if (virJSONValueObjectAppendString(initramfs, "path", vmdef->os.initrd) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "initramfs", &initramfs) < 0) - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(kernel); - virJSONValueFree(cmdline); - virJSONValueFree(initramfs); - - return -1; } static int -- 2.33.0
Also introduces a G_DEFINE_AUTOPTR_CLEANUP_FUNC for virCHMonitor. Signed-off-by: William Douglas <william.douglas@intel.com> --- src/ch/ch_monitor.c | 22 +++++++--------------- src/ch/ch_monitor.h | 1 + 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index bb49c70069..376404033e 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -442,9 +442,8 @@ chMonitorCreateSocket(const char *socket_path) virCHMonitor * virCHMonitorNew(virDomainObj *vm, const char *socketdir) { - virCHMonitor *ret = NULL; - virCHMonitor *mon = NULL; - virCommand *cmd = NULL; + g_autoptr(virCHMonitor) mon = NULL; + g_autoptr(virCommand) cmd = NULL; int socket_fd = 0; if (virCHMonitorInitialize() < 0) @@ -456,7 +455,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) if (!vm->def) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("VM is not defined")); - goto cleanup; + return NULL; } /* prepare to launch Cloud-Hypervisor socket */ @@ -465,7 +464,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) virReportSystemError(errno, _("Cannot create socket directory '%s'"), socketdir); - goto cleanup; + return NULL; } cmd = virCommandNew(vm->def->emulator); @@ -475,7 +474,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) virReportSystemError(errno, _("Cannot create socket '%s'"), mon->socketpath); - goto cleanup; + return NULL; } virCommandAddArg(cmd, "--api-socket"); @@ -484,7 +483,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(); @@ -492,14 +491,7 @@ virCHMonitorNew(virDomainObj *vm, const char *socketdir) /* now has its own reference */ mon->vm = virObjectRef(vm); - ret = mon; - mon = NULL; - - cleanup: - if (mon) - virCHMonitorClose(mon); - virCommandFree(cmd); - return ret; + return g_steal_pointer(&mon); } static void virCHMonitorDispose(void *opaque) diff --git a/src/ch/ch_monitor.h b/src/ch/ch_monitor.h index e39b4eb8b2..0f684ca583 100644 --- a/src/ch/ch_monitor.h +++ b/src/ch/ch_monitor.h @@ -53,6 +53,7 @@ struct _virCHMonitor { virCHMonitor *virCHMonitorNew(virDomainObj *vm, const char *socketdir); void virCHMonitorClose(virCHMonitor *mon); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCHMonitor, virCHMonitorClose); int virCHMonitorCreateVM(virCHMonitor *mon); int virCHMonitorBootVM(virCHMonitor *mon); -- 2.33.0
participants (3)
-
Douglas, William -
Laine Stump -
William Douglas