[libvirt PATCH 0/9] ch: use g_auto where possible
While cleaning up the tests, I noticed that the 'ch' driver is not using g_auto up to its full potential. Use automatic cleanup where possible and remove redundant labels. Ján Tomko (9): ch: use g_auto in virCHMonitorBuildCPUJson ch: use g_auto in virCHMonitorBuildKernelRelatedJson ch: use g_auto in virCHMonitorBuildMemoryJson ch: use g_auto in virCHMonitorBuildDiskJson ch: use g_auto in virCHMonitorBuildDisksJson ch: use g_auto in virCHMonitorBuildNetJson ch: use g_auto in virCHMonitorBuildNetsJson ch: use g_auto in virCHMonitorBuildVMJson ch: use g_auto in virCHMonitorNew src/ch/ch_monitor.c | 167 +++++++++++++++++--------------------------- 1 file changed, 63 insertions(+), 104 deletions(-) -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d277466262..876a553f74 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -58,7 +58,7 @@ int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint); static int virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *cpus; + g_autoptr(virJSONValue) cpus = NULL; unsigned int maxvcpus = 0; unsigned int nvcpus = 0; virDomainVcpuDef *vcpu; @@ -75,18 +75,14 @@ virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef) if (maxvcpus != 0 || nvcpus != 0) { cpus = virJSONValueNewObject(); if (virJSONValueObjectAppendNumberInt(cpus, "boot_vcpus", nvcpus) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppendNumberInt(cpus, "max_vcpus", vmdef->maxvcpus) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "cpus", &cpus) < 0) - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(cpus); - return -1; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.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 876a553f74..d241d30b10 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -112,45 +112,38 @@ 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 { kernel = virJSONValueNewObject(); 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) { initramfs = virJSONValueNewObject(); 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.31.1
On 9/22/21 4:55 PM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.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 876a553f74..d241d30b10 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -112,45 +112,38 @@ 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 { kernel = virJSONValueNewObject();
pre-existing, but this ^^^^ is leaking a virJSONValue, isn't it?
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) { initramfs = virJSONValueNewObject();
same here ^^^^
if (virJSONValueObjectAppendString(initramfs, "path", vmdef->os.initrd) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppend(content, "initramfs", &initramfs) < 0) - goto cleanup; + return -1; }
return 0;
Being unfamiliar with virJSONValueObjectAppend(), I looked to make sure it steals the object is appending, and it does.
- - cleanup: - virJSONValueFree(kernel); - virJSONValueFree(cmdline); - virJSONValueFree(initramfs); - - return -1; }
static int
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d241d30b10..9c562fdd0f 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -149,22 +149,18 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *memory; + g_autoptr(virJSONValue) memory = NULL; unsigned long long total_memory = virDomainDefGetMemoryInitial(vmdef) * 1024; if (total_memory != 0) { 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.31.1
On 9/22/21 4:55 PM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d241d30b10..9c562fdd0f 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -149,22 +149,18 @@ virCHMonitorBuildKernelRelatedJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *memory; + g_autoptr(virJSONValue) memory = NULL; unsigned long long total_memory = virDomainDefGetMemoryInitial(vmdef) * 1024;
if (total_memory != 0) { memory = virJSONValueNewObject();
You could have moved this initialization into the definition line for memory to eliminate one more line (that is your goal, right? :-)
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
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 9c562fdd0f..174ea47c51 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -166,31 +166,31 @@ virCHMonitorBuildMemoryJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) { - virJSONValue *disk = virJSONValueNewObject(); + g_autoptr(virJSONValue) disk = virJSONValueNewObject(); if (!diskdef->src) - goto cleanup; + return -1; switch (diskdef->src->type) { case VIR_STORAGE_TYPE_FILE: if (!diskdef->src->path) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Missing disk file path in domain")); - goto cleanup; + return -1; } if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_INVALID_ARG, _("Only virtio bus types are supported for '%s'"), diskdef->src->path); - goto cleanup; + return -1; } if (virJSONValueObjectAppendString(disk, "path", diskdef->src->path) < 0) - goto cleanup; + return -1; if (diskdef->src->readonly) { if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0) - goto cleanup; + return -1; } if (virJSONValueArrayAppend(disks, &disk) < 0) - goto cleanup; + return -1; break; case VIR_STORAGE_TYPE_NONE: @@ -202,14 +202,10 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) case VIR_STORAGE_TYPE_VHOST_USER: default: virReportEnumRangeError(virStorageType, diskdef->src->type); - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(disk); - return -1; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 174ea47c51..d268dc3c90 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -211,7 +211,7 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) static int virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *disks; + g_autoptr(virJSONValue) disks = NULL; size_t i; if (vmdef->ndisks > 0) { @@ -219,17 +219,13 @@ virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef) for (i = 0; i < vmdef->ndisks; i++) { if (virCHMonitorBuildDiskJson(disks, vmdef->disks[i]) < 0) - goto cleanup; + return -1; } if (virJSONValueObjectAppend(content, "disks", &disks) < 0) - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(disks); - return -1; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index d268dc3c90..21f8cb6782 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -233,7 +233,7 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) { virDomainNetType netType = virDomainNetGetActualType(netdef); char macaddr[VIR_MAC_STRING_BUFLEN]; - virJSONValue *net; + g_autoptr(virJSONValue) net = NULL; // check net type at first net = virJSONValueNewObject(); @@ -246,20 +246,20 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) virSocketAddr netmask; g_autofree char *netmaskStr = NULL; if (!(addr = virSocketAddrFormat(&ip->address))) - goto cleanup; + return -1; if (virJSONValueObjectAppendString(net, "ip", addr) < 0) - goto cleanup; + return -1; if (virSocketAddrPrefixToNetmask(ip->prefix, &netmask, AF_INET) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to translate net prefix %d to netmask"), ip->prefix); - goto cleanup; + return -1; } if (!(netmaskStr = virSocketAddrFormat(&netmask))) - goto cleanup; + return -1; if (virJSONValueObjectAppendString(net, "mask", netmaskStr) < 0) - goto cleanup; + return -1; } else if (netdef->guestIP.nips > 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("ethernet type supports a single guest ip")); @@ -269,12 +269,12 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) if ((virDomainChrType)netdef->data.vhostuser->type != VIR_DOMAIN_CHR_TYPE_UNIX) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("vhost_user type support UNIX socket in this CH")); - goto cleanup; + return -1; } else { if (virJSONValueObjectAppendString(net, "vhost_socket", netdef->data.vhostuser->data.nix.path) < 0) - goto cleanup; + return -1; if (virJSONValueObjectAppendBoolean(net, "vhost_user", true) < 0) - goto cleanup; + return -1; } break; case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -291,26 +291,26 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) case VIR_DOMAIN_NET_TYPE_LAST: default: virReportEnumRangeError(virDomainNetType, netType); - goto cleanup; + return -1; } if (netdef->ifname != NULL) { if (virJSONValueObjectAppendString(net, "tap", netdef->ifname) < 0) - goto cleanup; + return -1; } if (virJSONValueObjectAppendString(net, "mac", virMacAddrFormat(&netdef->mac, macaddr)) < 0) - goto cleanup; + return -1; if (netdef->virtio != NULL) { if (netdef->virtio->iommu == VIR_TRISTATE_SWITCH_ON) { if (virJSONValueObjectAppendBoolean(net, "iommu", true) < 0) - goto cleanup; + return -1; } } if (netdef->driver.virtio.queues) { if (virJSONValueObjectAppendNumberInt(net, "num_queues", netdef->driver.virtio.queues) < 0) - goto cleanup; + return -1; } if (netdef->driver.virtio.rx_queue_size || netdef->driver.virtio.tx_queue_size) { @@ -319,20 +319,16 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) _("virtio rx_queue_size option %d is not same with tx_queue_size %d"), netdef->driver.virtio.rx_queue_size, netdef->driver.virtio.tx_queue_size); - goto cleanup; + return -1; } if (virJSONValueObjectAppendNumberInt(net, "queue_size", netdef->driver.virtio.rx_queue_size) < 0) - goto cleanup; + return -1; } if (virJSONValueArrayAppend(nets, &net) < 0) - goto cleanup; + return -1; return 0; - - cleanup: - virJSONValueFree(net); - return -1; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 21f8cb6782..aa27eea304 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -334,7 +334,7 @@ virCHMonitorBuildNetJson(virJSONValue *nets, virDomainNetDef *netdef) static int virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef) { - virJSONValue *nets; + g_autoptr(virJSONValue) nets = NULL; size_t i; if (vmdef->nnets > 0) { @@ -342,17 +342,13 @@ virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef) for (i = 0; i < vmdef->nnets; i++) { if (virCHMonitorBuildNetJson(nets, vmdef->nets[i]) < 0) - goto cleanup; + return -1; } if (virJSONValueObjectAppend(content, "net", &nets) < 0) - goto cleanup; + return -1; } return 0; - - cleanup: - virJSONValueFree(nets); - return -1; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/ch/ch_monitor.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index aa27eea304..3504c21f9d 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -354,41 +354,36 @@ virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef) static int virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr) { - virJSONValue *content = virJSONValueNewObject(); - int ret = -1; + g_autoptr(virJSONValue) content = virJSONValueNewObject(); if (vmdef == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("VM is not defined")); - goto cleanup; + return -1; } if (virCHMonitorBuildPTYJson(content, vmdef) < 0) - goto cleanup; + return -1; if (virCHMonitorBuildCPUJson(content, vmdef) < 0) - goto cleanup; + return -1; if (virCHMonitorBuildMemoryJson(content, vmdef) < 0) - goto cleanup; + return -1; if (virCHMonitorBuildKernelRelatedJson(content, vmdef) < 0) - goto cleanup; + return -1; if (virCHMonitorBuildDisksJson(content, vmdef) < 0) - goto cleanup; + return -1; if (virCHMonitorBuildNetsJson(content, vmdef) < 0) - goto cleanup; + return -1; if (!(*jsonstr = virJSONValueToString(content, false))) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virJSONValueFree(content); - return ret; + return 0; } static int -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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; } 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); mon->vm = virObjectRef(vm); - ret = mon; - - cleanup: - virCommandFree(cmd); - return ret; + return mon; } static void virCHMonitorDispose(void *opaque) -- 2.31.1
On 9/22/21 4:55 PM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- 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)
On Wed, 2021-09-22 at 22:55 +0200, Ján Tomko wrote:
While cleaning up the tests, I noticed that the 'ch' driver is not using g_auto up to its full potential.
Use automatic cleanup where possible and remove redundant labels.
Ján Tomko (9): ch: use g_auto in virCHMonitorBuildCPUJson ch: use g_auto in virCHMonitorBuildKernelRelatedJson ch: use g_auto in virCHMonitorBuildMemoryJson ch: use g_auto in virCHMonitorBuildDiskJson ch: use g_auto in virCHMonitorBuildDisksJson ch: use g_auto in virCHMonitorBuildNetJson ch: use g_auto in virCHMonitorBuildNetsJson ch: use g_auto in virCHMonitorBuildVMJson ch: use g_auto in virCHMonitorNew
src/ch/ch_monitor.c | 167 +++++++++++++++++------------------------- -- 1 file changed, 63 insertions(+), 104 deletions(-)
Nice cleanup +1 for the patchset from me, thanks!
On 9/22/21 4:55 PM, Ján Tomko wrote:
While cleaning up the tests, I noticed that the 'ch' driver is not using g_auto up to its full potential.
Use automatic cleanup where possible and remove redundant labels.
Ján Tomko (9): ch: use g_auto in virCHMonitorBuildCPUJson ch: use g_auto in virCHMonitorBuildKernelRelatedJson ch: use g_auto in virCHMonitorBuildMemoryJson ch: use g_auto in virCHMonitorBuildDiskJson ch: use g_auto in virCHMonitorBuildDisksJson ch: use g_auto in virCHMonitorBuildNetJson ch: use g_auto in virCHMonitorBuildNetsJson ch: use g_auto in virCHMonitorBuildVMJson ch: use g_auto in virCHMonitorNew
src/ch/ch_monitor.c | 167 +++++++++++++++++--------------------------- 1 file changed, 63 insertions(+), 104 deletions(-)
I pointed out pre-existing object leaks in patch 2 & 9, which you could just fix with simple prerequisite patches. Aside from that Reviewed-by: Laine Stump <laine@redhat.com> for the series.
participants (3)
-
Douglas, William -
Ján Tomko -
Laine Stump