Re: [libvirt PATCH 2/9] ch: use g_auto in virCHMonitorBuildKernelRelatedJson

[Laine Stump <laine@xxxxxxxxxx>]

On 9/22/21 4:55 PM, Ján Tomko wrote:
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
>   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