Re: [PULL 12/23] qemu-config: add error propagation to qemu_config_parse

[Peter Maydell]

On Sat, 6 Mar 2021 at 10:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> This enables some simplification of vl.c via error_fatal, and improves
> error messages.  Before:
>
>   $ ./qemu-system-x86_64 -readconfig .
>   qemu-system-x86_64: error reading file
>   qemu-system-x86_64: -readconfig .: read config .: Invalid argument
>   $ /usr/libexec/qemu-kvm -readconfig foo
>   qemu-kvm: -readconfig foo: read config foo: No such file or directory
>
> After:
>
>   $ ./qemu-system-x86_64 -readconfig .
>   qemu-system-x86_64: -readconfig .: Cannot read config file: Is a directory
>   $ ./qemu-system-x86_64 -readconfig foo
>   qemu-system-x86_64: -readconfig foo: Could not open 'foo': No such file or 
> directory
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <20210226170816.231173-1-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Hi; Coverity complains (CID 1457456) about a resource leak here:


>  /* Returns number of config groups on success, -errno on error */
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, 
> Error **errp)
>  {
>      char line[1024], group[64], id[64], arg[64], value[1024];
>      Location loc;
> @@ -375,7 +375,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, 
> const char *fname)
>              /* group with id */
>              list = find_list(lists, group, &local_err);
>              if (local_err) {
> -                error_report_err(local_err);
> +                error_propagate(errp, local_err);
>                  goto out;
>              }
>              opts = qemu_opts_create(list, id, 1, NULL);
> @@ -386,7 +386,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, 
> const char *fname)
>              /* group without id */
>              list = find_list(lists, group, &local_err);
>              if (local_err) {
> -                error_report_err(local_err);
> +                error_propagate(errp, local_err);
>                  goto out;
>              }
>              opts = qemu_opts_create(list, NULL, 0, &error_abort);
> @@ -398,21 +398,21 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, 
> const char *fname)
>              sscanf(line, " %63s = \"\"", arg) == 1) {
>              /* arg = value */
>              if (opts == NULL) {
> -                error_report("no group defined");
> +                error_setg(errp, "no group defined");
>                  goto out;
>              }
> -            if (!qemu_opt_set(opts, arg, value, &local_err)) {
> -                error_report_err(local_err);
> +            if (!qemu_opt_set(opts, arg, value, errp)) {
>                  goto out;
>              }
>              continue;
>          }
> -        error_report("parse error");
> +        error_setg(errp, "parse error");
>          goto out;
>      }
>      if (ferror(fp)) {
> -        error_report("error reading file");
> -        goto out;
> +        loc_pop(&loc);
> +        error_setg_errno(errp, errno, "Cannot read config file");

Here we no longer 'goto out' and therefore we don't qobject_unref(qdict)
in this error path.

> +        return res;
>      }
>      res = count;
>  out:

thanks
-- PMM