Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- 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

Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- 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

Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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

On 9/22/21 4:55 PM, Ján Tomko wrote: Again, it's pre-existing, but it looks to me like "mon" is leaked when there is any error in this function. 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.