Re: [PATCH v2] qapi, qemu-options: make all parsing visitors parse boolean options the same

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> OptsVisitor, StringInputVisitor and the keyval visitor have
> three different ideas of how a human could write the value of
> a boolean option.  Pay homage to the backwards-compatibility
> gods and make the new common helper accept all four sets (on/off,
> true/false, y/n and yes/no), and case-insensitive at that.
>
> Since OptsVisitor is supposed to match qemu-options, adjust
> it as well.

For clarity: s/it/the latter/

> Supersedes: <20201103142344.402353-1-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qapi/util.h          |  2 ++
>  qapi/opts-visitor.c          | 14 +-------------
>  qapi/qapi-util.c             | 23 +++++++++++++++++++++++
>  qapi/qobject-input-visitor.c | 15 +++++----------
>  qapi/string-input-visitor.c  | 17 +----------------
>  util/qemu-option.c           | 20 ++------------------
>  6 files changed, 34 insertions(+), 57 deletions(-)
>
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index bc312e90aa..6178e98e97 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -19,6 +19,8 @@ typedef struct QEnumLookup {
>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
>  int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>                      int def, Error **errp);
> +bool qapi_bool_parse(const char *name, const char *value, bool *obj,
> +                     Error **errp);
>  
>  int parse_qapi_name(const char *name, bool complete);
>  
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 7781c23a42..587f31baf6 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -368,7 +368,6 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
> Error **errp)
>  }
>  
>  
> -/* mimics qemu-option.c::parse_option_bool() */
>  static bool
>  opts_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
>  {
> @@ -379,19 +378,8 @@ opts_type_bool(Visitor *v, const char *name, bool *obj, 
> Error **errp)
>      if (!opt) {
>          return false;
>      }
> -
>      if (opt->str) {
> -        if (strcmp(opt->str, "on") == 0 ||
> -            strcmp(opt->str, "yes") == 0 ||
> -            strcmp(opt->str, "y") == 0) {
> -            *obj = true;
> -        } else if (strcmp(opt->str, "off") == 0 ||
> -            strcmp(opt->str, "no") == 0 ||
> -            strcmp(opt->str, "n") == 0) {
> -            *obj = false;
> -        } else {
> -            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
> -                       "on|yes|y|off|no|n");
> +        if (!qapi_bool_parse(opt->name, opt->str, obj, errp)) {
>              return false;
>          }
>      } else {
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 29a6c98b53..4dd8f6c313 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/ctype.h"
> +#include "qapi/qmp/qerror.h"
>  
>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val)
>  {
> @@ -40,6 +41,28 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char 
> *buf,
>      return def;
>  }
>  
> +bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error 
> **errp)
> +{
> +    if (!strcasecmp(value, "on") ||
> +        !strcasecmp(value, "yes") ||
> +        !strcasecmp(value, "true") ||
> +        !strcasecmp(value, "y")) {
> +        *obj = true;
> +        return true;
> +    }
> +    if (!strcasecmp(value, "off") ||
> +        !strcasecmp(value, "no") ||
> +        !strcasecmp(value, "false") ||
> +        !strcasecmp(value, "n")) {
> +        *obj = false;
> +        return true;
> +    }
> +
> +    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +               "boolean (on/off, yes/no, true/false, y/n)");

Recommend to have the error message only mention the preferred form.  I
like the laconic "'on' or 'off'".  It's really all the user needs to
know.

> +    return false;
> +}
> +
>  /*
>   * Parse a valid QAPI name from @str.
>   * A valid name consists of letters, digits, hyphen and underscore.
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 7b184b50a7..f4541a4fdd 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -512,16 +512,11 @@ static bool qobject_input_type_bool_keyval(Visitor *v, 
> const char *name,
>          return false;
>      }
>  
> -    if (!strcmp(str, "on")) {
> -        *obj = true;
> -    } else if (!strcmp(str, "off")) {
> -        *obj = false;
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> -                   full_name(qiv, name), "'on' or 'off'");
> -        return false;
> -    }
> -    return true;
> +    /*
> +     * Calling full_name is a bit slow, but keyval (human/CLI) parsing
> +     * is not a hot path.
> +     */
> +    return qapi_bool_parse(full_name(qiv, name), str, obj, errp);

Avoiding the full_name() on success isn't hard:

       if (!qapi_bool_parse(name, str, obj, NULL)) {
           error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                      full_name(qiv, name), "'on' or 'off'");
           return false;
       }
       return true;

A callback to build the name only on error would be overkill.

By the way: related qapi_enum_parse() also takes an @errp parameter, and
several of its callers pass NULL and call error_setg() themselves.
Sometimes one doesn't fit all.  I've been tempted more than once to
provide two functions: one that doesn't take @errp, and the obvious
wrapper that does.

>  }
>  
>  static bool qobject_input_type_str(Visitor *v, const char *name, char **obj,
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 6e53396ea3..197139c1c0 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -332,22 +332,7 @@ static bool parse_type_bool(Visitor *v, const char 
> *name, bool *obj,
>      StringInputVisitor *siv = to_siv(v);
>  
>      assert(siv->lm == LM_NONE);
> -    if (!strcasecmp(siv->string, "on") ||
> -        !strcasecmp(siv->string, "yes") ||
> -        !strcasecmp(siv->string, "true")) {
> -        *obj = true;
> -        return true;
> -    }
> -    if (!strcasecmp(siv->string, "off") ||
> -        !strcasecmp(siv->string, "no") ||
> -        !strcasecmp(siv->string, "false")) {
> -        *obj = false;
> -        return true;
> -    }
> -
> -    error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> -               "boolean");
> -    return false;
> +    return qapi_bool_parse(name ? name : "null", siv->string, obj, errp);
>  }
>  
>  static bool parse_type_str(Visitor *v, const char *name, char **obj,
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index b9f93a7f8b..acefbc23fa 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -96,21 +96,6 @@ const char *get_opt_value(const char *p, char **value)
>      return offset;
>  }
>  
> -static bool parse_option_bool(const char *name, const char *value, bool *ret,
> -                              Error **errp)
> -{
> -    if (!strcmp(value, "on")) {
> -        *ret = 1;
> -    } else if (!strcmp(value, "off")) {
> -        *ret = 0;
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> -                   name, "'on' or 'off'");
> -        return false;
> -    }
> -    return true;
> -}
> -
>  static bool parse_option_number(const char *name, const char *value,
>                                  uint64_t *ret, Error **errp)
>  {
> @@ -363,7 +348,7 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, 
> const char *name,
>      if (opt == NULL) {
>          def_val = find_default_by_name(opts, name);
>          if (def_val) {
> -            parse_option_bool(name, def_val, &ret, &error_abort);
> +            qapi_bool_parse(name, def_val, &ret, &error_abort);
>          }
>          return ret;
>      }
> @@ -471,8 +456,7 @@ static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
>          /* nothing */
>          return true;
>      case QEMU_OPT_BOOL:
> -        return parse_option_bool(opt->name, opt->str, &opt->value.boolean,
> -                                 errp);
> +        return qapi_bool_parse(opt->name, opt->str, &opt->value.boolean, 
> errp);

Please break the line the same way as before.

>      case QEMU_OPT_NUMBER:
>          return parse_option_number(opt->name, opt->str, &opt->value.uint,
>                                     errp);

Nothing wrong, so:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Please consider my review comments anyway.