Re: [Qemu-devel] [PATCH 22/31] vl: Clean up error reporting in parse_fw_cfg()

[Marc-André Lureau]

Hi


On Mon, Oct 8, 2018 at 9:57 PM Markus Armbruster <address@hidden> wrote:
>
> Calling error_report() in a function that takes an Error ** argument
> is suspicious.  parse_fw_cfg() does that, and then fails without
> setting an error.  Its caller main(), via qemu_opts_foreach(), is fine
> with it, but clean it up anyway.
>
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

> ---
>  vl.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 1009d708a0..a3a39ec06b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2174,7 +2174,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>      FWCfgState *fw_cfg = (FWCfgState *) opaque;
>
>      if (fw_cfg == NULL) {
> -        error_report("fw_cfg device not available");
> +        error_setg(errp, "fw_cfg device not available");
>          return -1;
>      }
>      name = qemu_opt_get(opts, "name");
> @@ -2183,15 +2183,16 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>
>      /* we need name and either a file or the content string */
>      if (!(nonempty_str(name) && (nonempty_str(file) || nonempty_str(str)))) {
> -        error_report("invalid argument(s)");
> +        error_setg(errp, "invalid argument(s)");
>          return -1;
>      }
>      if (nonempty_str(file) && nonempty_str(str)) {
> -        error_report("file and string are mutually exclusive");
> +        error_setg(errp, "file and string are mutually exclusive");
>          return -1;
>      }
>      if (strlen(name) > FW_CFG_MAX_FILE_PATH - 1) {
> -        error_report("name too long (max. %d char)", FW_CFG_MAX_FILE_PATH - 
> 1);
> +        error_setg(errp, "name too long (max. %d char)",
> +                   FW_CFG_MAX_FILE_PATH - 1);
>          return -1;
>      }
>      if (strncmp(name, "opt/", 4) != 0) {
> @@ -2203,7 +2204,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, 
> Error **errp)
>          buf = g_memdup(str, size);
>      } else {
>          if (!g_file_get_contents(file, &buf, &size, NULL)) {
> -            error_report("can't load %s", file);
> +            error_setg(errp, "can't load %s", file);
>              return -1;
>          }
>      }
> @@ -4429,10 +4430,8 @@ int main(int argc, char **argv, char **envp)
>          hax_sync_vcpus();
>      }
>
> -    if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> -                          parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
> -        exit(1);
> -    }
> +    qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> +                      parse_fw_cfg, fw_cfg_find(), &error_fatal);
>
>      /* init USB devices */
>      if (machine_usb(current_machine)) {
> --
> 2.17.1
>
>


-- 
Marc-André Lureau