Paolo Bonzini <pbonz...@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 <pbonz...@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);
> }
