| Message ID | 20201103142344.402353-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: make all parsing visitors parse boolean options the same | expand |
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!
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 @@ -379,19 +379,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(name, opt->str, obj, errp)) { 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")) { + *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; +} + /* * 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); } 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 @@ -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,
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. 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(-)