Re: [PATCH v2 6/6] qemu-option: warn for short-form boolean options

[Markus Armbruster]

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.
>
> The extra boolean argument is not my favorite.  In 6.0 I plan to remove
> qemu_opts_set_defaults by switching -M to keyval, and therefore quite
> a bit of QemuOpts code will go away.
>
> 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 8c1dc7645d..f45938a5ff 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,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 322b32871b..e12fb51032 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -519,7 +519,7 @@ static void test_opts_parse(void)
>      error_free_or_abort(&err);
>      g_assert(!opts);
>  
> -    /* Implied value */
> +    /* Implied value (qemu_opts_parse does not warn) */
>      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 0ddf1f7b45..23238f00ea 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.

>          }
>      } 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) {
>              return false;
>          }
> @@ -837,7 +844,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);

No change (we pass false to warn_on_flag).

>          if (!strcmp(name, "id")) {
>              g_free(name);
>              return value;
> @@ -856,7 +863,7 @@ bool has_help_option(const char *params)
>      bool ret;
>  
>      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);

Likewise.

>          g_free(name);
>          g_free(value);
>          if (ret) {
> @@ -876,12 +883,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);

Likewise.

>  }
>  
>  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);
> @@ -904,8 +911,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;
>      }
> @@ -923,7 +930,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);

Likewise.

>  }
>  
>  /**
> @@ -941,7 +948,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().

> @@ -960,7 +967,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);
>  }

No change (we pass false to warn_on_flag).

Summary: only qemu_opts_parse_noisily() warns.  This is airtight only if
all user input flows through qemu_opts_parse_noisily().  It's too late
in my day for me to check.