Re: [Qemu-devel] [PATCH v24 02/31] QemuOpts: add def_value_str to QemuOptDesc

[Chunyan Liu]

2014-04-08 9:31 GMT+08:00 Leandro Dorileo <address@hidden>:

On Thu, Apr 03, 2014 at 05:54:20PM +0800, Chunyan Liu wrote:
> Add def_value_str (default value) to QemuOptDesc, to replace function of the
> default value in QEMUOptionParameter.
>
> Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise,
> if desc->def_value_str is set, return desc->def_value_str; otherwise, return
> input defval.
>
> Improve qemu_opts_print: if option is set, print opt->str; otherwise, if
> desc->def_value_str is set, also print it.
>
> Reviewed-by: Eric Blake <address@hidden>
> Signed-off-by: Dong Xu Wang <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  include/qemu/option.h |  3 ++-
>  util/qemu-option.c    | 56 ++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 8c0ac34..c3b0a91 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -99,6 +99,7 @@ typedef struct QemuOptDesc {
>      const char *name;
>      enum QemuOptType type;
>      const char *help;
> +    const char *def_value_str;
>  } QemuOptDesc;


I still think you don't need to define the default value as a string to later
parse it, see what I mean https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04273.html

But I think we can live with it. See bellow, if you fix the issue you can add my Reviewed-by.


>
>  struct QemuOptsList {
> @@ -156,7 +157,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
>  void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>
>  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
> -int qemu_opts_print(QemuOpts *opts, void *dummy);
> +void qemu_opts_print(QemuOpts *opts);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure);
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index e6d10bc..d5e80da 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -570,6 +570,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +
> +    if (!opt) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            return desc->def_value_str;
> +        }
> +    }
>      return opt ? opt->str : NULL;
>  }
>
> @@ -589,8 +596,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_bool(name, desc->def_value_str, &defval, &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
>      return opt->value.boolean;
>  }
> @@ -599,8 +611,14 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_number(name, desc->def_value_str, &defval,
> +                                &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
>      return opt->value.uint;
>  }
> @@ -609,8 +627,13 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_size(name, desc->def_value_str, &defval, &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
>      return opt->value.uint;
>  }
> @@ -895,17 +918,32 @@ void qemu_opts_del(QemuOpts *opts)
>      g_free(opts);
>  }
>
> -int qemu_opts_print(QemuOpts *opts, void *dummy)
> +void qemu_opts_print(QemuOpts *opts)
>  {
>      QemuOpt *opt;
> +    QemuOptDesc *desc = opts->list->desc;
>
> -    fprintf(stderr, "%s: %s:", opts->list->name,
> -            opts->id ? opts->id : "<noid>");
> -    QTAILQ_FOREACH(opt, &opts->head, next) {
> -        fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
> +    if (desc[0].name == NULL) {
> +        QTAILQ_FOREACH(opt, &opts->head, next) {
> +            fprintf(stderr, "%s=\"%s\" ", opt->name, opt->str);
> +        }
> +        return;
> +    }
> +    for (; desc && desc->name; desc++) {
> +        const char *value;
> +        QemuOpt *opt = qemu_opt_find(opts, desc->name);
> +
> +        value = opt ? opt->str : desc->def_value_str;
> +        if (!value) {
> +            continue;
> +        }
> +        if (desc->type == QEMU_OPT_STRING) {
> +            fprintf(stderr, "%s='%s' ", desc->name, value);
> +        } else {
> +            fprintf(stderr, "%s=%s ", desc->name, value);
> +        }
>      }
>      fprintf(stderr, "\n");



I see you removed this fprintf() in the commit afe65f5c92694d551aaebc128233b623e8665d5e
Please do this change in this commit and avoid breaking the block tests in
further commits.

This function won't be used until 0012-change-block-layer-to-support-both-QemuOpts-and-QEMU.patch.

It won't break block tests. The commit afe65f5c92694d551aaebc128233b623e8665d5e
could be squashed with this commit directly. I split it into two steps just to make
each patch easy to review, since in previous version, switching fprintf to printf
in this patch was complained. But sure I can merge these two patches  if you
still think it's better.

Chunyan

 

--
Leandro Dorileo

> -    return 0;
>  }
>
>  static int opts_do_parse(QemuOpts *opts, const char *params,
> --
> 1.7.12.4
>