Re: [PATCH v2] qemu-config: add error propagation to qemu_config_parse

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This enables some simplification of vl.c via error_fatal, and improves
> error messages.  Before:
>
> 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 .: error reading file: Is a directory
>   $ ./qemu-system-x86_64 -readconfig foo
>   qemu-system-x86_64: -readconfig foo: Cannot read config file foo: No such 
> file or directory
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Lovely :)

> ---
>  block/blkdebug.c           |  3 +--
>  include/qemu/config-file.h |  5 +++--
>  softmmu/vl.c               | 29 +++++++++++------------------
>  util/qemu-config.c         | 23 ++++++++++++-----------
>  4 files changed, 27 insertions(+), 33 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 29226107bd..8d3e53ae4d 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -11,9 +11,10 @@ void qemu_add_drive_opts(QemuOptsList *list);
>  int qemu_global_option(const char *str);
>  
>  void qemu_config_write(FILE *fp);
> -int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
> +int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname,
> +                      Error **errp);
>  
> -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 e67f91dd37..e80859c40d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2062,17 +2062,19 @@ 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)
>  {
> +    ERRP_GUARD();
>      int ret;
>      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, errp);
> +    if (ret < 0) {
> +        if (ret == -ENOENT) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
>      }
> -
> -    return 0;
>  }

This change to return void goes against advice in error.h:

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

Tolerable.  The only caller wouldn't profit from a return value (it
passes &error_fatal), and more callers seem quite unlikely.

>  
>  static int qemu_set_option(const char *str)
> @@ -2640,9 +2642,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 */
> @@ -3330,15 +3330,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;
>              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 e2a700b284..af89df109d 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -350,7 +350,7 @@ void qemu_config_write(FILE *fp)
>  }
>  
>  /* 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;
>              }

Please avoid error_propagate() where possible:

               list = find_list(lists, group, errp);
               if (!list) {
                   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;
>              }

Likewise.

>              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, "error reading file");
> +        return res;
>      }
>      res = count;
>  out:
> @@ -420,16 +420,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;
>  }

Not avoiding error_propagate() isn't wrong, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

But please consider avoiding it anyway.