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(-)
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.
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)");
+ 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);
}
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);
case QEMU_OPT_NUMBER:
return parse_option_number(opt->name, opt->str, &opt->value.uint,
errp);
--
2.26.2
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.
On 04/11/20 08:35, Markus Armbruster wrote: >> + "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. > I went for "boolean (on/off) value". > > 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; Can't refuse. >> >> 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. Why? It's not even 80 characters. Paolo
On Tue, Nov 03, 2020 at 11:13:39AM -0500, Paolo Bonzini wrote: > 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. FWIW, libvirt does not appear to use true/false or y/n, nor ever use uppercase / mixed case. IOW this level of back compat may well be overkill. I'd particular suggest deprecating case-insensitivity, as Yes, YES, yEs feel unlikely to be important or widely used. Based on libvirt test suite QEMU CLI args, I see primarily on|off, with a small amount of yes|no: 3dnowext=on 3dnow=on 3dnowprefetch=off aarch64=off acpi=on adjust=on adjvtime=on aefsi=on aen=on apft=on ap=on append=on apqci=on apqi=on assist=on ats=off ats=on avx2=off avx=off bpb=on cache=off cache=on clflushopt=on clwb=on cmmnt=on cmm=on cmp_legacy=on compression=off compression=on core=off core=on cpl=on csske=on csum=off cte=on data=yes dedicated=on deflate=on dfppc=on display=on ds_cpl=on edat2=on edat=on eim=on eoi=off eoi=on erms=on esop=on esort=on etoken=on event_idx=off event_idx=on f16c=off failover=on fma=off fxsr_opt=off fxsr_opt=on gl=on gs=on gso=off guest_csum=off guest_ecn=off guest_tso4=off guest_tso6=off guest_ufo=off hmat=on host_ecn=off host_tso4=off host_tso6=off host_ufo=off hotplug=off hotplug=on htm=on hugetlb=yes hv=off hypervisor=on iep=on info=off info=on intremap=on invtsc=on ioeventfd=off ioeventfd=on iommu=on iommu_platform=off iommu_platform=on iotlb=on ipter=on kvmclock=off kvm=off kvm_pv_eoi=off kvm_pv_eoi=on kvm_pv_unhalt=off kvm_pv_unhalt=on la57=on lahf_lm=on legacy=off legacy=on lm=on lock=off lock=on logappend=off logappend=on master=off master=on menu=off menu=on mepoch=on mepochptff=on merge=off migration=on misalignsse=off mlock=off mlock=on mode=on modern=off mouse=off mpx=on mq=on mrg_rxbuf=off msa1=on msa2=on msa3=on msa4=on msa5=on msa6=on msa7=on msa8=on msa9=on msa9_pckmo=on msi=on msr=on multifunction=off multifunction=on nb=on ni=off nodeid_msr=on npt=on nvdimm=on oom=off oom=on opt=on osvw=off packed=off packed=on pclmuldq=on pclmulqdq=on pcommit=on pconfig=off pdpe1gb=on peer=yes perfctr_core=on perfctr_nb=on pku=on pmem=yes pmu=off pmu=on ppa15=on prealloc=yes ramfb=on rdrand=off rdseed=off read=on readonly=on readonly=on' removable=off removable=on reporting=on ri=on rw=on save=off scsi=off scsi=on sea_esop2=on share=no share=yes smm=on ssbd=on ss=on stckf=off sthyi=on strict=on sve128=on sve256=on sve384=off sve512=on sve=on te=on threads=on timestamp=on topoext=off tsc_adjust=on umip=on unarmed=on unhalt=off unhalt=on usb=off value=on vhost=on virgl=on vme=off vmport=off vmx=on vxeh2=on vxeh=on vx=on vxpdeh=on vxpd=on wrap=off wrap=on xsavec=off xsaves=on zeroes=on zpci=on Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 03/11/20 17:25, Daniel P. Berrangé wrote: >> 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. > FWIW, libvirt does not appear to use true/false or y/n, nor > ever use uppercase / mixed case. > > IOW this level of back compat may well be overkill. > > I'd particular suggest deprecating case-insensitivity, as > Yes, YES, yEs feel unlikely to be important or widely used. True; at least it's type-safe code unlike the short-form boolean option. It only hurts in the odd case of a boolean option becoming on/off/auto or on/off/split. I didn't want to introduce deprecation at this point, because consistency is better anyway even if we plan to later deprecate something. For example, since there is a common parser now, introducing deprecation would be much easier. It also lets us switch parsers even during the deprecation period (which is how I got into this mess). Paolo
On Tue, Nov 03, 2020 at 05:33:47PM +0100, Paolo Bonzini wrote: > On 03/11/20 17:25, Daniel P. Berrangé wrote: > >> 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. > > FWIW, libvirt does not appear to use true/false or y/n, nor > > ever use uppercase / mixed case. > > > > IOW this level of back compat may well be overkill. > > > > I'd particular suggest deprecating case-insensitivity, as > > Yes, YES, yEs feel unlikely to be important or widely used. > > True; at least it's type-safe code unlike the short-form boolean option. > It only hurts in the odd case of a boolean option becoming on/off/auto > or on/off/split. > > I didn't want to introduce deprecation at this point, because > consistency is better anyway even if we plan to later deprecate > something. For example, since there is a common parser now, introducing > deprecation would be much easier. It also lets us switch parsers even > during the deprecation period (which is how I got into this mess). Ok, Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Paolo Bonzini <pbonzini@redhat.com> writes: > On 03/11/20 17:25, Daniel P. Berrangé wrote: >>> 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. >> FWIW, libvirt does not appear to use true/false or y/n, nor >> ever use uppercase / mixed case. >> >> IOW this level of back compat may well be overkill. >> >> I'd particular suggest deprecating case-insensitivity, as >> Yes, YES, yEs feel unlikely to be important or widely used. > > True; at least it's type-safe code unlike the short-form boolean option. > It only hurts in the odd case of a boolean option becoming on/off/auto > or on/off/split. Another argument for deprecating values other than "on" and "off". > I didn't want to introduce deprecation at this point, because Not wanting to rush deprecation this close to the release is a valid point. So is not wanting to rush in additional sugar :) > consistency is better anyway even if we plan to later deprecate > something. For example, since there is a common parser now, introducing > deprecation would be much easier. It also lets us switch parsers even > during the deprecation period (which is how I got into this mess). On the other hand, we'll have more to deprecate. Status quo: on/off yes/no y/n true/false case-sensitive? qemu_opt_get_bool() X no keyval visitor X no string visitor X X X yes opts visitor X X X no For once, there is no way to blame QemuOpts ;) qemu_opt_get_bool() is everywhere in CLI and HMP. The keyval visitor is used for QAPIfied human-friendly interfaces: block stuff, -display, -audio. The string input visitor is used for -object, -global, HMP object_add, qom-set, migrate_set_parameter. The opts visitor is used for -mon and its sugared forms (new in 5.2!), -net, -netdev, -acpitable, -numa. I'd very much prefer to deprecate the odd ones in string and opts visitor, and not copy them elsewhere. We can still factor out a common parsing function if we like: pass a flag that makes it recognize deprecated forms. A single flag should suffice; it lets odd ones spread from string to opts visitor and vice versa, which I find tolerable.
On 04/11/20 09:29, Markus Armbruster wrote: >> It only hurts in the odd case of a boolean option becoming on/off/auto >> or on/off/split. > Another argument for deprecating values other than "on" and "off". Unfortunately I'm fairly sure that I've seen yes/no in use. I can buy insta-removal (not deprecation) of case-insensitivity. Paolo
On Wed, Nov 04, 2020 at 12:31:40PM +0100, Paolo Bonzini wrote: > On 04/11/20 09:29, Markus Armbruster wrote: > > > It only hurts in the odd case of a boolean option becoming on/off/auto > > > or on/off/split. > > Another argument for deprecating values other than "on" and "off". > > Unfortunately I'm fairly sure that I've seen yes/no in use. I can buy > insta-removal (not deprecation) of case-insensitivity. Seems a couple of example usages are my fault as I documented them :-( docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ include/authz/listfile.h: * filename=/etc/qemu/myvm-vnc.acl,refresh=yes qemu-options.hx: ``-object authz-listfile,id=id,filename=path,refresh=yes|no`` qemu-options.hx: -object authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=yes \\ We should fix thos docs in QEMU at least, and unfortunately it seems I missed that libvirt did use verify-peer=yes Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Nov 04, 2020 at 12:31:40PM +0100, Paolo Bonzini wrote: >> On 04/11/20 09:29, Markus Armbruster wrote: >> > > It only hurts in the odd case of a boolean option becoming on/off/auto >> > > or on/off/split. >> > Another argument for deprecating values other than "on" and "off". >> >> Unfortunately I'm fairly sure that I've seen yes/no in use. If it wasn't, we'd remove rather than deprecate :) >> I can buy >> insta-removal (not deprecation) of case-insensitivity. Since case-insensitivity is undocumented, and we're not aware of actual use, no objection. > Seems a couple of example usages are my fault as I documented them :-( > > docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ > docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ > docs/system/vnc-security.rst: -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu,endpoint=server,verify-peer=yes \ > include/authz/listfile.h: * filename=/etc/qemu/myvm-vnc.acl,refresh=yes > qemu-options.hx: ``-object authz-listfile,id=id,filename=path,refresh=yes|no`` > qemu-options.hx: -object authz-simple,id=auth0,filename=/etc/qemu/vnc-sasl.acl,refresh=yes \\ > > We should fix thos docs in QEMU at least, and unfortunately it seems I > missed that libvirt did use verify-peer=yes Yes, please.
© 2016 - 2024 Red Hat, Inc.