[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qapi: make all parsing visitors parse boolean options the sa
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] qapi: make all parsing visitors parse boolean options the same |
Date: |
Tue, 03 Nov 2020 17:00:04 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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!