1:12 p.m.
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
1:12 p.m.
New subject: [PATCH 1/5] ch_monitor: Stop leaking json value objects
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(a)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
2:26 p.m.
New subject: [PATCH 1/5] ch_monitor: Stop leaking json value objects
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(a)intel.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
1:12 p.m.
New subject: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
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(a)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
2:43 p.m.
New subject: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
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(a)redhat.com>
Signed-off-by: William Douglas <william.douglas(a)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:52 p.m.
New subject: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
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.
9:10 p.m.
New subject: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
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!
11:25 p.m.
New subject: [PATCH 2/5] ch_monitor: Correctly close and ref the virCHMonitor
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!
1:12 p.m.
New subject: [PATCH 3/5] ch: use g_auto in virCHMonitorBuildMemoryJson
Signed-off-by: William Douglas <william.douglas(a)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
2:46 p.m.
New subject: [PATCH 3/5] ch: use g_auto in virCHMonitorBuildMemoryJson
On 10/1/21 2:12 PM, William Douglas wrote:
Signed-off-by: William Douglas <william.douglas(a)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(a)redhat.com>
I'll add the extra empty lines before pushing.
1:12 p.m.
New subject: [PATCH 4/5] ch: use g_auto in virCHMonitorBuildKernelRelatedJson
Signed-off-by: William Douglas <william.douglas(a)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
1:12 p.m.
New subject: [PATCH 5/5] ch: use g_auto in virCHMonitorNew
Also introduces a G_DEFINE_AUTOPTR_CLEANUP_FUNC for virCHMonitor.
Signed-off-by: William Douglas <william.douglas(a)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
1180
days inactive
1184
days old
11 comments
3 participants
participants (3)
-
Douglas, William -
Laine Stump -
William Douglas