[2/2] qemu-option: warn for short-form boolean options

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 all this, except for -chardev and -spice where it is in
wide use.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 chardev/char.c             |  1 +
 docs/system/deprecated.rst |  7 +++++++
 include/qemu/option.h      |  1 +
 tests/test-qemu-opts.c     |  1 +
 ui/spice-core.c            |  1 +
 util/qemu-option.c         | 21 ++++++++++++++-------
 6 files changed, 25 insertions(+), 7 deletions(-)

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 all this, except for -chardev and -spice where it is in
> wide use.

I consider this a misuse of deprecation, to be frank.  If something is
known to be unused, we just remove it.  Deprecation is precisely for
things that are used.  I'm with Daniel here: let's deprecate this sugar
everywhere.

Wide use may justify extending the deprecation grace period.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  chardev/char.c             |  1 +
>  docs/system/deprecated.rst |  7 +++++++
>  include/qemu/option.h      |  1 +
>  tests/test-qemu-opts.c     |  1 +
>  ui/spice-core.c            |  1 +
>  util/qemu-option.c         | 21 ++++++++++++++-------
>  6 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 78553125d3..108da615df 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -829,6 +829,7 @@ Chardev *qemu_chr_find(const char *name)
>  
>  QemuOptsList qemu_chardev_opts = {
>      .name = "chardev",
> +    .allow_flag_options = true, /* server, nowait, etc. */
>      .implied_opt_name = "backend",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_chardev_opts.head),
>      .desc = {
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 32a0e620db..0e7edf7e56 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,13 @@ 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
> +for all command-line options except ``-chardev` and ``-spice``, for
> +which the short form was in wide use.

Unlike the commit message, the text here misleads readers into assuming
the sugar applies only to boolean options.

>  
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ac69352e0e..046ac06a1f 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -65,6 +65,7 @@ struct QemuOptsList {
>      const char *name;
>      const char *implied_opt_name;
>      bool merge_lists;  /* Merge multiple uses of option into a single list? */
> +    bool allow_flag_options; /* Whether to warn for short-form boolean options */

I disagree with having this option.  I'm reviewing it anyway.

Staying under the 80 characters limit is really, really easy here:

       bool allow_flag_options;    /* Warn on short-form boolean options? */

I had to read ahead to figure out that false means warn, true means
accept silently.  The comment is confusing because "whether to warn"
suggests "warn if true", which is wrong.

Clearer (I think), and even shorter:

       bool allow_bool_sugar;      /* Is boolean option sugar okay? */

>      QTAILQ_HEAD(, QemuOpts) head;
>      QemuOptDesc desc[];
>  };
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 297ffe79dd..a74c475773 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -77,6 +77,7 @@ static QemuOptsList opts_list_02 = {
>  static QemuOptsList opts_list_03 = {
>      .name = "opts_list_03",
>      .implied_opt_name = "implied",
> +    .allow_flag_options = true,
>      .head = QTAILQ_HEAD_INITIALIZER(opts_list_03.head),
>      .desc = {
>          /* no elements => accept any params */

Can you point me to the tests this hunk affects?

Do we have test coverage for boolean sugar with both values of
allow_flag_options?

If no, why is that okay?

> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index eea52f5389..08f834fa41 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -397,6 +397,7 @@ static SpiceChannelList *qmp_query_spice_channels(void)
>  
>  static QemuOptsList qemu_spice_opts = {
>      .name = "spice",
> +    .allow_flag_options = true, /* disable-agent-file-xfer, sasl, unix, etc. */
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
>      .merge_lists = true,
>      .desc = {
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 61fc96f9dd..858860377b 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -763,10 +763,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,

Two callers pass false, the other passes !list->allow_flag_options.
Having both allow_flag_options and warn_on_flag with the opposite sense
is confusing.  Please pick one.

>                                        bool *help_wanted,
>                                        char **name, char **value)
>  {
>      const char *p;
> +    const char *prefix = "";
>      size_t len;
>  
>      len = strcspn(params, "=,");
> @@ -784,9 +786,14 @@ static const char *get_opt_name_value(const char *params,
           /* found "foo,more" */
           if (firstname) {
               /* implicitly named first option */
               *name = g_strdup(firstname);
               p = get_opt_value(params, value);
           } else {
               /* option without value, must be a flag */
               p = get_opt_name(params, name, ',');
>              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");
>              }
> +            if (warn_on_flag) {
> +                error_report("short-form boolean option '%s%s' deprecated", prefix, *name);

warn_report(), please.  error_report() is strictly for error conditions,
i.e. things that fail.

> +                error_printf("Please use %s=%s instead\n", *name, *value);
> +            }
>          }
>      } else {
>          /* found "foo=bar,more" */
> @@ -805,14 +812,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.  Please break the line before the output arguments:

           p = get_opt_name_value(p, firstname, warn_on_flag,
                                  help_wanted, &option, &value);

>          if (help_wanted && *help_wanted) {
>              return false;
>          }
> @@ -841,7 +848,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;

Yes, we want don't want to warn here.

> @@ -860,7 +867,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) {

Likewise.

> @@ -880,7 +887,7 @@ 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);

Can you explain why this one doesn't warn?

>  }
>  
>  static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> @@ -908,8 +915,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,
> +                       !list->allow_flag_options, help_wanted, errp)) {
>          qemu_opts_del(opts);
>          return NULL;
>      }

Here's a funny one:

    void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
                                int permit_abbrev)
    {
        QemuOpts *opts;

        opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
        assert(opts);
    }

If someone manages to put bool sugar into
machine_class->default_machine_opts, we'll print a confusing warning on
startup.  It's a programming error, which means we should abort().

On 06/11/20 17:49, Markus Armbruster wrote:
>> Deprecate all this, except for -chardev and -spice where it is in
>> wide use.
> I consider this a misuse of deprecation, to be frank.  If something is
> known to be unused, we just remove it.  Deprecation is precisely for
> things that are used.  I'm with Daniel here: let's deprecate this sugar
> everywhere.
> 
> Wide use may justify extending the deprecation grace period.

Fair enough.  However now that I think of it I'd have to remove the 
coverage of the "feature" in tests, because they'd warn.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/11/20 17:49, Markus Armbruster wrote:
>>> Deprecate all this, except for -chardev and -spice where it is in
>>> wide use.
>> I consider this a misuse of deprecation, to be frank.  If something is
>> known to be unused, we just remove it.  Deprecation is precisely for
>> things that are used.  I'm with Daniel here: let's deprecate this sugar
>> everywhere.
>> Wide use may justify extending the deprecation grace period.
>
> Fair enough.  However now that I think of it I'd have to remove the
> coverage of the "feature" in tests, because they'd warn.

Either that, or do what we do elsewhere when warnings get in the way of
testing: suppress them when testing.  I wouldn't bother here unless it's
easy.