Re: [PATCH] qapi: 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.

Mention the match is now case-insensitive?

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qapi/util.h          |  2 ++
>  qapi/opts-visitor.c          | 13 +------------
>  qapi/qapi-util.c             | 23 +++++++++++++++++++++++
>  qapi/qobject-input-visitor.c | 11 +----------
>  qapi/string-input-visitor.c  | 17 +----------------
>  5 files changed, 28 insertions(+), 38 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..9b3a735e6d 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c

Oh no, the options visitor!

> @@ -379,19 +379,8 @@ opts_type_bool(Visitor *v, const char *name, bool *obj, 
> Error **errp)
   /* mimics qemu-option.c::parse_option_bool() */

This comment becomes wrong.

   static bool
   opts_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
   {
       OptsVisitor *ov = to_ov(v);
       const QemuOpt *opt;

       opt = lookup_scalar(ov, name, 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(name, opt->str, obj, errp)) {

Exploits lookup_name() ensures name == opt->name.

Obviously true when ov->list_mode == LM_NONE: lookup_name() returns the
last QemuOpt of that name.

We don't get here when ov->list_mode == LM_TRAVERSED: lookup_name()
fails.

"Obviously" true when ov->list_mode == LM_IN_PROGRESS: lookup_name()
returns the next remaining QemuOpt of that name (I think).

>              return false;
>          }
>      } else {
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 29a6c98b53..8a98813e42 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")) {

I'd prefer breaking the lines before the || operator (Knuth style).

> +        *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 ? name : "null",
> +               "boolean (on/off, yes/no, true/false, y/n)");
> +    return false;

Baroque.  Not this patch's fault.  I'm half-tempted to deprecate
everything but 'on' and 'off', case-sensitive.

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.

You copied the name ?: "null" from the string input visitor.  It's bad
there (but nobody cares), and it's bad here (where I care).  I think it
should be left to callers.  See also next hunk.

> +}
> +
>  /*
>   * 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..183472e5e4 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -512,16 +512,7 @@ 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;
> +    return qapi_bool_parse(name, str, obj, errp);

Error message regresses from full_name(), which is never null, to name
?: "null".

Test case:

    $ qemu-storage-daemon --blockdev 
qcow2,node-name=node0,file.driver=file,file.filename=tmp.qcow2,file.x-check-cache-dropped=xxx
    qemu-storage-daemon: --blockdev 
qcow2,node-name=node0,file.driver=file,file.filename=tmp.qcow2,file.x-check-cache-dropped=xxx:
 Parameter 'file.x-check-cache-dropped' expects 'on' or 'off'

!name happens for ['bool'].  The error message is user-hostile then.  No
test case, because ['bool'] occurs only in tests/ right now.

>  }
>  
>  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..ba2697d70f 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c

Oh no, the string visitor!

> @@ -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, siv->string, obj, errp);
>  }
>  
>  static bool parse_type_str(Visitor *v, const char *name, char **obj,

Puh!