| Message ID | 20210118163113.780171-4-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing | expand |
Paolo Bonzini <pbonzini@redhat.com> writes: > Options such as "server" or "nowait", that are commonly found in -chardev, > are sugar for "server=on" and "wait=off". This is quite surprising and > also does not have any notion of typing attached. It is even possible to > do "-device e1000,noid" and get a device with "id=off". > > Deprecate it and print a warning when it is encountered. In general, > this short form for boolean options only seems to be in wide use for > -chardev and -spice. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > docs/system/deprecated.rst | 6 ++++++ > tests/test-qemu-opts.c | 2 +- > util/qemu-option.c | 29 ++++++++++++++++++----------- > 3 files changed, 25 insertions(+), 12 deletions(-) > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index e20bfcb17a..e71faefbe5 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -127,6 +127,12 @@ Drives with interface types other than ``if=none`` are for onboard > devices. It is possible to use drives the board doesn't pick up with > -device. This usage is now deprecated. Use ``if=none`` instead. > > +Short-form boolean options (since 5.2) 6.0 > +'''''''''''''''''''''''''''''''''''''' > + > +Boolean options such as ``share=on``/``share=off`` can be written > +in short form as ``share`` and ``noshare``. This is deprecated > +and will cause a warning. > > QEMU Machine Protocol (QMP) commands > ------------------------------------ > diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c > index 2aab831d10..8bbb17b1c7 100644 > --- a/tests/test-qemu-opts.c > +++ b/tests/test-qemu-opts.c > @@ -515,7 +515,7 @@ static void test_opts_parse(void) > error_free_or_abort(&err); > g_assert(!opts); > > - /* Implied value */ > + /* Implied value (qemu_opts_parse warns but accepts it) */ > opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", > false, &error_abort); > g_assert_cmpuint(opts_count(opts), ==, 3); > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 5f27d4369d..40564a12eb 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -756,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) > > static const char *get_opt_name_value(const char *params, > const char *firstname, > + bool warn_on_flag, > bool *help_wanted, > char **name, char **value) > { > const char *p; > + const char *prefix = ""; > size_t len; > bool is_help = false; > > @@ -776,10 +778,15 @@ static const char *get_opt_name_value(const char *params, > if (strncmp(*name, "no", 2) == 0) { > memmove(*name, *name + 2, strlen(*name + 2) + 1); > *value = g_strdup("off"); > + prefix = "no"; > } else { > *value = g_strdup("on"); > is_help = is_help_option(*name); > } > + if (!is_help && warn_on_flag) { > + warn_report("short-form boolean option '%s%s' deprecated", prefix, *name); > + error_printf("Please use %s=%s instead\n", *name, *value); > + } If @warn_on_flag, we warn except for "help" and "?". The exception applies regardless of @help_wanted. Shouldn't we except *only* recognized help requests? > } > } else { > /* found "foo=bar,more" */ > @@ -801,14 +808,14 @@ static const char *get_opt_name_value(const char *params, > > static bool opts_do_parse(QemuOpts *opts, const char *params, > const char *firstname, bool prepend, > - bool *help_wanted, Error **errp) > + bool warn_on_flag, bool *help_wanted, Error **errp) > { > char *option, *value; > const char *p; > QemuOpt *opt; > > for (p = params; *p;) { > - p = get_opt_name_value(p, firstname, help_wanted, &option, &value); > + p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value); Long line. Break it before the three output arguments. > if (help_wanted && *help_wanted) { > g_free(option); > g_free(value); > @@ -839,7 +846,7 @@ static char *opts_parse_id(const char *params) > char *name, *value; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, NULL, &name, &value); > + p = get_opt_name_value(p, NULL, false, NULL, &name, &value); > if (!strcmp(name, "id")) { > g_free(name); > return value; > @@ -858,7 +865,7 @@ bool has_help_option(const char *params) > bool ret = false; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, &ret, &name, &value); > + p = get_opt_name_value(p, NULL, false, &ret, &name, &value); > g_free(name); > g_free(value); > if (ret) { > @@ -878,12 +885,12 @@ bool has_help_option(const char *params) > bool qemu_opts_do_parse(QemuOpts *opts, const char *params, > const char *firstname, Error **errp) > { > - return opts_do_parse(opts, params, firstname, false, NULL, errp); > + return opts_do_parse(opts, params, firstname, false, false, NULL, errp); > } > > static QemuOpts *opts_parse(QemuOptsList *list, const char *params, > bool permit_abbrev, bool defaults, > - bool *help_wanted, Error **errp) > + bool warn_on_flag, bool *help_wanted, Error **errp) > { > const char *firstname; > char *id = opts_parse_id(params); > @@ -906,8 +913,8 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, > return NULL; > } > > - if (!opts_do_parse(opts, params, firstname, defaults, help_wanted, > - errp)) { > + if (!opts_do_parse(opts, params, firstname, defaults, > + warn_on_flag, help_wanted, errp)) { > qemu_opts_del(opts); > return NULL; > } > @@ -925,7 +932,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, > QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, > bool permit_abbrev, Error **errp) > { > - return opts_parse(list, params, permit_abbrev, false, NULL, errp); > + return opts_parse(list, params, permit_abbrev, false, false, NULL, errp); > } > > /** > @@ -943,7 +950,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, > QemuOpts *opts; > bool help_wanted = false; > > - opts = opts_parse(list, params, permit_abbrev, false, > + opts = opts_parse(list, params, permit_abbrev, false, true, > opts_accepts_any(list) ? NULL : &help_wanted, > &err); > if (!opts) { This function now warns, except for "help" and "?". The exception applies even when we treat "help" and "?" as sugar for "help=on" and "?=on" because opts_accepts_any(). It is the only spot that enables the warning. Does all user input flow through qemu_opts_parse_noisily()? > @@ -962,7 +969,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params, > { > QemuOpts *opts; > > - opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL); > + opts = opts_parse(list, params, permit_abbrev, true, false, NULL, NULL); > assert(opts); > }
On 19/01/21 16:56, Markus Armbruster wrote: >> + if (!is_help && warn_on_flag) { >> + warn_report("short-form boolean option '%s%s' deprecated", prefix, *name); >> + error_printf("Please use %s=%s instead\n", *name, *value); >> + } > > If @warn_on_flag, we warn except for "help" and "?". The exception > applies regardless of @help_wanted. Shouldn't we except*only* > recognized help requests? Suggesting "help=yes" would be worse. >> >> - opts = opts_parse(list, params, permit_abbrev, false, >> + opts = opts_parse(list, params, permit_abbrev, false, true, >> opts_accepts_any(list) ? NULL : &help_wanted, >> &err); >> if (!opts) { > > This function now warns, except for "help" and "?". The exception > applies even when we treat "help" and "?" as sugar for "help=on" and > "?=on" because opts_accepts_any(). > > It is the only spot that enables the warning. > > Does all user input flow through qemu_opts_parse_noisily()? > I was going to say yes, but -vnc (and worse, the QMP version of "change vnc") is parsed by qemu_opts_parse() via ui/vnc.c (besides being used by lots of tests). -vnc has several boolean options, and though Libvirt only uses "sasl" it does so in the short form. My solution would be to deprecate the QMP "change vnc" command, and postpone switching -vnc to qemu_opts_parse_noisily to 6.2. The main reason to warn for short-form boolean options, is to block them for command line options that are switched to keyval[1]. Adding a warning does not necessarily imply removing in two releases. Paolo [1] This series already does that for -M, -accel and -object. This means that applying this series would change the command line incompatibly without a two-release deprecation. It's up for discussion whether to do so, or delay the application of those patches to 6.2. It would be a pity to hold the dependent changes for effectively a year, but it's not a big deal.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 19/01/21 16:56, Markus Armbruster wrote: >>> + if (!is_help && warn_on_flag) { >>> + warn_report("short-form boolean option '%s%s' deprecated", prefix, *name); >>> + error_printf("Please use %s=%s instead\n", *name, *value); >>> + } >> >> If @warn_on_flag, we warn except for "help" and "?". The exception >> applies regardless of @help_wanted. Shouldn't we except*only* >> recognized help requests? > > Suggesting "help=yes" would be worse. Would it? get_opt_name_value() parses one parameter from params into *name and *value. if help_wanted && is_help, it additionally sets *help_wanted to true. is_help is true when the parameter is "help" or "?". How could a parameter "help" be handled? get_opt_name_value() will set *name = g_strdup("help"); *value = g_strdup("on"); If help_wanted, additionally: *help_wanted = true; Callers that pass non-null help_wanted can do whatever they want with that. The actual callers do honor the help request. The deprecation warning obviously needs to be suppressed for them. Callers that pass null help_wanted will treat this just like any other parameter. Use of the boolean sugar is just as deprecated for this parameter as it is for all the others. Suppressing the deprecation warning feels wrong. The alternative is to *outlaw* parameters "help" and "?" in QemuOpts. I'd be cool with that. >>> - opts = opts_parse(list, params, permit_abbrev, false, >>> + opts = opts_parse(list, params, permit_abbrev, false, true, >>> opts_accepts_any(list) ? NULL : &help_wanted, >>> &err); >>> if (!opts) { >> >> This function now warns, except for "help" and "?". The exception >> applies even when we treat "help" and "?" as sugar for "help=on" and >> "?=on" because opts_accepts_any(). >> >> It is the only spot that enables the warning. >> >> Does all user input flow through qemu_opts_parse_noisily()? >> > > I was going to say yes, but -vnc (and worse, the QMP version of "change > vnc") is parsed by qemu_opts_parse() via ui/vnc.c (besides being used by > lots of tests). -vnc has several boolean options, and though Libvirt > only uses "sasl" it does so in the short form. > > My solution would be to deprecate the QMP "change vnc" command, and > postpone switching -vnc to qemu_opts_parse_noisily to 6.2. QMP command 'change' was deprecated long ago, in 2.5.0 (commit 24fb41330, in 2015). This predated appendix "Deprecated features" (which has since become docs/system/deprecated.rst), and remained missing there until I corrected it in commit 6d570ca10 (v4.2.0). > The main reason to warn for short-form boolean options, is to block them > for command line options that are switched to keyval[1]. Adding a > warning does not necessarily imply removing in two releases. Understand. > Paolo > > [1] This series already does that for -M, -accel and -object. This > means that applying this series would change the command line > incompatibly without a two-release deprecation. It's up for discussion > whether to do so, or delay the application of those patches to 6.2. It > would be a pity to hold the dependent changes for effectively a year, > but it's not a big deal. Concur.
On 20/01/21 09:42, Markus Armbruster wrote: > The alternative is to *outlaw* parameters "help" and "?" in QemuOpts. > I'd be cool with that. > >> My solution would be to deprecate the QMP "change vnc" command, and >> postpone switching -vnc to qemu_opts_parse_noisily to 6.2. > > QMP command 'change' was deprecated long ago, in 2.5.0 (commit > 24fb41330, in 2015). This predated appendix "Deprecated features" > (which has since become docs/system/deprecated.rst), and remained > missing there until I corrected it in commit 6d570ca10 (v4.2.0). Removal patch coming then, together with switching vnc_parse to qemu_opts_parse_noisily. That would restrict qemu_opts_parse to tests, and implicitly outlaw parameters "help" and "?". The other problem would be solved, albeit a bit indirectly. >> The main reason to warn for short-form boolean options, is to block them >> for command line options that are switched to keyval[1]. Adding a >> warning does not necessarily imply removing in two releases. > > Understand. > >> [1] This series already does that for -M, -accel and -object. This >> means that applying this series would change the command line >> incompatibly without a two-release deprecation. It's up for discussion >> whether to do so, or delay the application of those patches to 6.2. It >> would be a pity to hold the dependent changes for effectively a year, >> but it's not a big deal. > > Concur. Verbose please. :) Do you think we should delay the conversion of -M/-accel/-object to keyval until 6.2? Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 20/01/21 09:42, Markus Armbruster wrote: >> The alternative is to *outlaw* parameters "help" and "?" in QemuOpts. >> I'd be cool with that. > >>> My solution would be to deprecate the QMP "change vnc" command, and >>> postpone switching -vnc to qemu_opts_parse_noisily to 6.2. >> >> QMP command 'change' was deprecated long ago, in 2.5.0 (commit >> 24fb41330, in 2015). This predated appendix "Deprecated features" >> (which has since become docs/system/deprecated.rst), and remained >> missing there until I corrected it in commit 6d570ca10 (v4.2.0). > > Removal patch coming then, together with switching vnc_parse to > qemu_opts_parse_noisily. > > That would restrict qemu_opts_parse to tests, and implicitly outlaw > parameters "help" and "?". The other problem would be solved, albeit a > bit indirectly. Please remember to mention the outlaws in a commit message. >>> The main reason to warn for short-form boolean options, is to block them >>> for command line options that are switched to keyval[1]. Adding a >>> warning does not necessarily imply removing in two releases. >> >> Understand. >> >>> [1] This series already does that for -M, -accel and -object. This >>> means that applying this series would change the command line >>> incompatibly without a two-release deprecation. It's up for discussion >>> whether to do so, or delay the application of those patches to 6.2. It >>> would be a pity to hold the dependent changes for effectively a year, >>> but it's not a big deal. >> >> Concur. > > Verbose please. :) Do you think we should delay the conversion of > -M/-accel/-object to keyval until 6.2? I concurred with "it's up for discussion". I'm happy do discuss, of course. Delaying reduces (but does not eliminate) the risk of breaking stuff that uses the sugar. Is that worth the loss of momentum? Hard to say. I think we can ignore non-boolean parameters, because if you're using the sugar with those, you're kind of begging for some punishment :) What are the boolean parameters, and is the *any* evidence of use with the sugar in the wild?
On 20/01/21 13:59, Markus Armbruster wrote: >> Verbose please. Do you think we should delay the conversion of >> -M/-accel/-object to keyval until 6.2? > > I concurred with "it's up for discussion". I'm happy do discuss, of > course. > What are the boolean parameters, and is there *any* evidence of use with > the sugar in the wild? There is no evidence for -machine, -accel or -object. Notably (and for Libvirt only): - '-object' takes a detour through JSON, and uses sometimes =on/=off or sometimes =yes/=no. - '-accel' is not used at all - '-machine' does not use short-form boolean options by sheer luck. The only known usage in Libvirt, for instance, is for "-chardev" and "-vnc". Paolo
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index e20bfcb17a..e71faefbe5 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -127,6 +127,12 @@ Drives with interface types other than ``if=none`` are for onboard devices. It is possible to use drives the board doesn't pick up with -device. This usage is now deprecated. Use ``if=none`` instead. +Short-form boolean options (since 5.2) +'''''''''''''''''''''''''''''''''''''' + +Boolean options such as ``share=on``/``share=off`` can be written +in short form as ``share`` and ``noshare``. This is deprecated +and will cause a warning. QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 2aab831d10..8bbb17b1c7 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -515,7 +515,7 @@ static void test_opts_parse(void) error_free_or_abort(&err); g_assert(!opts); - /* Implied value */ + /* Implied value (qemu_opts_parse warns but accepts it) */ opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=", false, &error_abort); g_assert_cmpuint(opts_count(opts), ==, 3); diff --git a/util/qemu-option.c b/util/qemu-option.c index 5f27d4369d..40564a12eb 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -756,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) static const char *get_opt_name_value(const char *params, const char *firstname, + bool warn_on_flag, bool *help_wanted, char **name, char **value) { const char *p; + const char *prefix = ""; size_t len; bool is_help = false; @@ -776,10 +778,15 @@ static const char *get_opt_name_value(const char *params, if (strncmp(*name, "no", 2) == 0) { memmove(*name, *name + 2, strlen(*name + 2) + 1); *value = g_strdup("off"); + prefix = "no"; } else { *value = g_strdup("on"); is_help = is_help_option(*name); } + if (!is_help && warn_on_flag) { + warn_report("short-form boolean option '%s%s' deprecated", prefix, *name); + error_printf("Please use %s=%s instead\n", *name, *value); + } } } else { /* found "foo=bar,more" */ @@ -801,14 +808,14 @@ static const char *get_opt_name_value(const char *params, static bool opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, bool prepend, - bool *help_wanted, Error **errp) + bool warn_on_flag, bool *help_wanted, Error **errp) { char *option, *value; const char *p; QemuOpt *opt; for (p = params; *p;) { - p = get_opt_name_value(p, firstname, help_wanted, &option, &value); + p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value); if (help_wanted && *help_wanted) { g_free(option); g_free(value); @@ -839,7 +846,7 @@ static char *opts_parse_id(const char *params) char *name, *value; for (p = params; *p;) { - p = get_opt_name_value(p, NULL, NULL, &name, &value); + p = get_opt_name_value(p, NULL, false, NULL, &name, &value); if (!strcmp(name, "id")) { g_free(name); return value; @@ -858,7 +865,7 @@ bool has_help_option(const char *params) bool ret = false; for (p = params; *p;) { - p = get_opt_name_value(p, NULL, &ret, &name, &value); + p = get_opt_name_value(p, NULL, false, &ret, &name, &value); g_free(name); g_free(value); if (ret) { @@ -878,12 +885,12 @@ bool has_help_option(const char *params) bool qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, Error **errp) { - return opts_do_parse(opts, params, firstname, false, NULL, errp); + return opts_do_parse(opts, params, firstname, false, false, NULL, errp); } static QemuOpts *opts_parse(QemuOptsList *list, const char *params, bool permit_abbrev, bool defaults, - bool *help_wanted, Error **errp) + bool warn_on_flag, bool *help_wanted, Error **errp) { const char *firstname; char *id = opts_parse_id(params); @@ -906,8 +913,8 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, return NULL; } - if (!opts_do_parse(opts, params, firstname, defaults, help_wanted, - errp)) { + if (!opts_do_parse(opts, params, firstname, defaults, + warn_on_flag, help_wanted, errp)) { qemu_opts_del(opts); return NULL; } @@ -925,7 +932,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, bool permit_abbrev, Error **errp) { - return opts_parse(list, params, permit_abbrev, false, NULL, errp); + return opts_parse(list, params, permit_abbrev, false, false, NULL, errp); } /** @@ -943,7 +950,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, QemuOpts *opts; bool help_wanted = false; - opts = opts_parse(list, params, permit_abbrev, false, + opts = opts_parse(list, params, permit_abbrev, false, true, opts_accepts_any(list) ? NULL : &help_wanted, &err); if (!opts) { @@ -962,7 +969,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params, { QemuOpts *opts; - opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL); + opts = opts_parse(list, params, permit_abbrev, true, false, NULL, NULL); assert(opts); }
Options such as "server" or "nowait", that are commonly found in -chardev, are sugar for "server=on" and "wait=off". This is quite surprising and also does not have any notion of typing attached. It is even possible to do "-device e1000,noid" and get a device with "id=off". Deprecate it and print a warning when it is encountered. In general, this short form for boolean options only seems to be in wide use for -chardev and -spice. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- docs/system/deprecated.rst | 6 ++++++ tests/test-qemu-opts.c | 2 +- util/qemu-option.c | 29 ++++++++++++++++++----------- 3 files changed, 25 insertions(+), 12 deletions(-)