Re: [PATCH 12/25] qemu-config: add error propagation to qemu_config_parse

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This enables some simplification of vl.c via error_fatal.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/blkdebug.c           |  3 +--
>  include/qemu/config-file.h |  4 ++--
>  softmmu/vl.c               | 30 ++++++++++++------------------
>  util/qemu-config.c         | 20 ++++++++++----------
>  4 files changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 5fe6172da9..7eaa8a28bf 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -279,9 +279,8 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
>              return -errno;
>          }
>  
> -        ret = qemu_config_parse(f, config_groups, filename);
> +        ret = qemu_config_parse(f, config_groups, filename, errp);
>          if (ret < 0) {
> -            error_setg(errp, "Could not parse blkdebug config file");
>              goto fail;
>          }
>      }
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index 7d26fe3816..da6f4690b7 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -10,9 +10,9 @@ void qemu_add_opts(QemuOptsList *list);
>  void qemu_add_drive_opts(QemuOptsList *list);
>  int qemu_global_option(const char *str);
>  
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error **errp);

Long line.

>  
> -int qemu_read_config_file(const char *filename);
> +int qemu_read_config_file(const char *filename, Error **errp);
>  
>  /* Parse QDict options as a replacement for a config file (allowing multiple
>     enumerated (0..(n-1)) configuration "sections") */
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index d34307bf11..d991919155 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2056,17 +2056,20 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> -static int qemu_read_default_config_file(void)
> +static void qemu_read_default_config_file(Error **errp)
>  {
>      int ret;
> +    Error *local_err = NULL;
>      g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR "/qemu.conf");
>  
> -    ret = qemu_read_config_file(file);
> -    if (ret < 0 && ret != -ENOENT) {
> -        return ret;
> +    ret = qemu_read_config_file(file, &local_err);
> +    if (ret < 0) {
> +        if (ret == -ENOENT) {
> +            error_free(local_err);
> +        } else {
> +            error_propagate(errp, local_err);
> +        }
>      }
> -
> -    return 0;
>  }

Please use ERRP_GUARD() in new code:

   static void qemu_read_default_config_file(Error **errp)
   {
       ERRP_GUARD();
       int ret;
       g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR
                                                  "/qemu.conf");

       ret = qemu_read_config_file(file, errp);
       if (ret == -ENOENT) {
           error_free(*errp);
           *errp = NULL;
       }
   }

>  
>  static int qemu_set_option(const char *str)
> @@ -2622,9 +2625,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      }
>  
>      if (userconfig) {
> -        if (qemu_read_default_config_file() < 0) {
> -            exit(1);
> -        }
> +        qemu_read_default_config_file(&error_fatal);
>      }
>  
>      /* second pass of option parsing */
> @@ -3312,15 +3313,8 @@ void qemu_init(int argc, char **argv, char **envp)
>                  qemu_plugin_opt_parse(optarg, &plugin_list);
>                  break;
>              case QEMU_OPTION_readconfig:
> -                {
> -                    int ret = qemu_read_config_file(optarg);
> -                    if (ret < 0) {
> -                        error_report("read config %s: %s", optarg,
> -                                     strerror(-ret));
> -                        exit(1);
> -                    }
> -                    break;
> -                }
> +                qemu_read_config_file(optarg, &error_fatal);
> +                break;

More than just code simplifcation: you're deleting an extra error
message.  Test case:

    $ qemu-system-x86_64 -readconfig .
    qemu: error reading file
    qemu: -readconfig .: read config .: Invalid argument

Pretty bad.  With your patch applied:

    qemu-system-x86_64: error reading file

Worse :)

I actually expected this to come out as

    qemu-system-x86_64: -readconfig .: error reading file

That would be an improvement.

The reason for the bad location information is here:

    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;
        QemuOptsList *list = NULL;
        Error *local_err = NULL;
        QemuOpts *opts = NULL;
        int res = -EINVAL, lno = 0;
        int count = 0;

        loc_push_none(&loc);
        while (fgets(line, sizeof(line), fp) != NULL) {

If the very first fgets() fails setting @fp's error indicator, ...

            loc_set_file(fname, ++lno);
            [...]
        }
        if (ferror(fp)) {

... we get here with error location "none", and ...

            error_setg(errp, "error reading file");

... use it to report the error (remember, @errp is &error_fatal).

Independently, error_setg_errno() would be nice.  Assuming we're willing
to rely on errno being useful after fgets().

            goto out;
        }
        res = count;
    out:
        loc_pop(&loc);
        return res;
    }

Always, always, always test the error messages.

>              case QEMU_OPTION_spice:
>                  olist = qemu_find_opts_err("spice", NULL);
>                  if (!olist) {
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index a4a1324c68..d0629f4960 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -308,7 +308,7 @@ void qemu_add_opts(QemuOptsList *list)
>  }
>  
>  /* 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;
> @@ -333,7 +333,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;
>              }

Please avoid error_propagate() where possible:

               list = find_list(lists, group, errp);
               if (!list) {
                   goto out;
               }


>              opts = qemu_opts_create(list, id, 1, NULL);
> @@ -344,7 +344,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;
>              }

Likewise.

>              opts = qemu_opts_create(list, NULL, 0, &error_abort);
> @@ -356,20 +356,19 @@ 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");

*Ächz*  Not your patch's fault.

>          goto out;
>      }
>      if (ferror(fp)) {
> -        error_report("error reading file");
> +        error_setg(errp, "error reading file");
>          goto out;
>      }
>      res = count;
> @@ -378,16 +377,17 @@ out:
>      return res;
>  }
>  
> -int qemu_read_config_file(const char *filename)
> +int qemu_read_config_file(const char *filename, Error **errp)
>  {
>      FILE *f = fopen(filename, "r");
>      int ret;
>  
>      if (f == NULL) {
> +        error_setg_errno(errp, errno, "Cannot read config file %s", filename);
>          return -errno;
>      }
>  
> -    ret = qemu_config_parse(f, vm_config_groups, filename);
> +    ret = qemu_config_parse(f, vm_config_groups, filename, errp);
>      fclose(f);
>      return ret;
>  }