Re: [Qemu-devel] [PATCH V13 2/6] avoid duplication of default value in QemuOpts

[Markus Armbruster]

Dong Xu Wang <address@hidden> writes:

> According Markus's comments, his patch will move the default value entirely
> to QemuOptDesc.
>
> When getting the value of an option that hasn't been set, and
> QemuOptDesc has a default value, return that.  Else, behave as
> before.
>
> Example: qemu_opt_get_number(opts, "foo", 42)
>
>    If "foo" has been set in opts, return its value.
>
>    Else, if opt's QemuOptDesc has a default value for "foo", return
>    that.
>
>    Else, return 42.
>
>    Note that the last argument is useless when QemuOptDesc has a
>    default value.  Ugly.  If it bothers us, assert that the argument
>    equals the default from QemuOptDesc.

Last sentence is not 100% clear.  Either change it to "If it bothers us,
we could assert", or implement the assert.  Considering we're at v12, I
recommend the former.

>
> Example: qemu_opt_get(opts, "bar")
>
>    If "bar" has been set in opts, return its value.
>
>    Else, if opt's QemuOptDesc has a default value for "bar", return
>    that.
>
>    Else, return NULL.
>
> Signed-off-by: Dong Xu Wang <address@hidden>
> ---
>  util/qemu-option.c | 58 
> +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 57cdd57..4f94000 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -525,10 +525,28 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const 
> char *name)
>      return NULL;
>  }
>  
> +static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> +                                            const char *name)
> +{
> +    int i;
> +
> +    for (i = 0; desc[i].name != NULL; i++) {
> +        if (strcmp(desc[i].name, name) == 0) {
> +            return &desc[i];
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> -    return opt ? opt->str : NULL;
> +    const QemuOptDesc *desc;
> +    desc = find_desc_by_name(opts->list->desc, name);
> +
> +    return opt ? opt->str :
> +        (desc && desc->def_value_str ? desc->def_value_str : NULL);
>  }
>  

I'd make this function work exactly like the ones that follow:

    if (!opt) {
        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;

Matter of taste; no need to respin just for that.

>  bool qemu_opt_has_help_opt(QemuOpts *opts)
> @@ -546,9 +564,15 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    const QemuOptDesc *desc;
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_bool(name, desc->def_value_str, &defval, NULL);

Ignores errors.  Should only happen when somebody sets a bad
desc->def_value_str.  Because those are fixed at compile time, that's a
programming error.  Recommend to assert like this:

            parse_option_bool(name, desc->def_value_str, &defval, &local_err);
            assert(!local_err);

> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
>      return opt->value.boolean;
>  }
> @@ -556,9 +580,15 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, 
> bool defval)
>  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t 
> defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    const QemuOptDesc *desc;
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_number(name, desc->def_value_str, &defval, NULL);

Likewise.

> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
>      return opt->value.uint;
>  }
> @@ -566,9 +596,15 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char 
> *name, uint64_t defval)
>  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    const QemuOptDesc *desc;
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_size(name, desc->def_value_str, &defval, NULL);

Likewise.

> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
>      return opt->value.uint;
>  }
> @@ -609,20 +645,6 @@ static bool opts_accepts_any(const QemuOpts *opts)
>      return opts->list->desc[0].name == NULL;
>  }
>  
> -static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> -                                            const char *name)
> -{
> -    int i;
> -
> -    for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> -            return &desc[i];
> -        }
> -    }
> -
> -    return NULL;
> -}
> -
>  static void opt_set(QemuOpts *opts, const char *name, const char *value,
>                      bool prepend, Error **errp)
>  {