Re: [PATCH 2/3] vl: plumb keyval-based options into -readconfig

[Kevin Wolf <kwolf@redhat.com>]

Am 18.05.2021 um 17:40 hat Paolo Bonzini geschrieben:
> Let -readconfig support parsing configuration file groups into QDict in
> addition to the previous behavior of recording them into QemuOpts.
> This will be used to add back support for objects in -readconfig.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/block/qdict.h    |  2 -
>  include/qapi/qmp/qdict.h |  3 ++
>  softmmu/vl.c             | 82 ++++++++++++++++++++++++++++------------
>  3 files changed, 61 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/qdict.h b/include/block/qdict.h
> index d8cb502d7d..ced2acfb92 100644
> --- a/include/block/qdict.h
> +++ b/include/block/qdict.h
> @@ -20,8 +20,6 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  int qdict_array_entries(QDict *src, const char *subqdict);
> -QObject *qdict_crumple(const QDict *src, Error **errp);
> -void qdict_flatten(QDict *qdict);
>  
>  typedef struct QDictRenames {
>      const char *from;
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 9934539c1b..d5b5430e21 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -64,4 +64,7 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key);
>  
>  QDict *qdict_clone_shallow(const QDict *src);
>  
> +QObject *qdict_crumple(const QDict *src, Error **errp);
> +void qdict_flatten(QDict *qdict);
> +
>  #endif /* QDICT_H */
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 594de0864c..90e491cc0c 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -121,6 +121,7 @@
>  #include "qapi/qapi-commands-misc.h"
>  #include "qapi/qapi-visit-qom.h"
>  #include "qapi/qapi-commands-ui.h"
> +#include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/iothread.h"
>  #include "qemu/guest-random.h"
> @@ -2115,13 +2116,52 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> +/*
> + * Return whether configuration group @group is stored in QemuOpts, or
> + * recorded as one or more QDicts by qemu_record_config_group.
> + */
> +static bool is_qemuopts_group(const char *group)
> +{
> +    return true;
> +}
> +
> +static void qemu_record_config_group(const char *group, QDict *dict, bool from_json, Error **errp)

This is a bit more than 80 characters.

> +{
> +    abort();
> +}
> +
> +/*
> + * Parse non-QemuOpts config file groups, pass the rest to
> + * qemu_config_do_parse.
> + */
> +static void qemu_parse_config_group(const char *group, QDict *qdict,
> +                                    void *opaque, Error **errp)
> +{
> +    QObject *crumpled;
> +    if (is_qemuopts_group(group)) {
> +        qemu_config_do_parse(group, qdict, opaque, errp);
> +        return;
> +    }
> +
> +    crumpled = qdict_crumple(qdict, errp);
> +    if (!crumpled) {
> +        return;
> +    }
> +    if (qobject_type(crumpled) != QTYPE_QDICT) {
> +        assert(qobject_type(crumpled) == QTYPE_QLIST);
> +        error_setg(errp, "Lists cannot be at top level of a configuration section");

This line is too long, too.

> +        return;
> +    }
> +    qemu_record_config_group(group, qobject_to(QDict, crumpled), false, errp);
> +}
> +
>  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, qemu_config_do_parse, errp);
> +    ret = qemu_read_config_file(file, qemu_parse_config_group, errp);
>      if (ret < 0) {
>          if (ret == -ENOENT) {
>              error_free(*errp);
> @@ -2130,9 +2170,8 @@ static void qemu_read_default_config_file(Error **errp)
>      }
>  }
>  
> -static int qemu_set_option(const char *str)
> +static void qemu_set_option(const char *str, Error **errp)
>  {
> -    Error *local_err = NULL;
>      char group[64], id[64], arg[64];
>      QemuOptsList *list;
>      QemuOpts *opts;
> @@ -2140,27 +2179,23 @@ static int qemu_set_option(const char *str)
>  
>      rc = sscanf(str, "%63[^.].%63[^.].%63[^=]%n", group, id, arg, &offset);
>      if (rc < 3 || str[offset] != '=') {
> -        error_report("can't parse: \"%s\"", str);
> -        return -1;
> -    }
> -
> -    list = qemu_find_opts(group);
> -    if (list == NULL) {
> -        return -1;
> -    }
> -
> -    opts = qemu_opts_find(list, id);
> -    if (!opts) {
> -        error_report("there is no %s \"%s\" defined",
> -                     list->name, id);
> -        return -1;
> +        error_setg(errp, "can't parse: \"%s\"", str);
> +        return;
>      }
>  
> -    if (!qemu_opt_set(opts, arg, str + offset + 1, &local_err)) {
> -        error_report_err(local_err);
> -        return -1;
> +    if (!is_qemuopts_group(group)) {
> +        error_setg(errp, "-set is not supported with %s", group);
> +    } else {
> +        list = qemu_find_opts_err(group, errp);
> +        if (list) {

I think testing for error (like before) is more obvious than testing for
success.  Future patches extending the function could easily miss that
we didn't return in an error case and errp is already set.

With the long lines fixed, and with or without changing this one:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>