[PATCH for-5.2 0/2] deprecate short-form boolean options

[PATCH for-5.2 0/2] deprecate short-form boolean options

[Paolo Bonzini]

QemuOpts lets you write boolean options in "short form"
where "abc" means "abc=on" and "noabc" means "abc=off".
This is confusing, since it is not done for the first
key=value pair but only if there is an implied key;
it can also be grossly misused, for example "-device
e1000,noid" will create a device with id equal to "off".

Unfortunately, this idiom has found wide use with
-chardev (think "server,nowait") and to a lesser extent
-spice.

Patch 2 in this series deprecates it for all other option
groups.  The first patch avoids emitting the warning
for the "help" option.

Paolo

Paolo Bonzini (2):
  qemu-option: move help handling to get_opt_name_value
  qemu-option: warn for short-form boolean options

 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         | 55 +++++++++++++++++++++-----------------
 6 files changed, 42 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH 1/2] qemu-option: move help handling to get_opt_name_value

[Paolo Bonzini]

Right now, help options are parsed normally and then checked
specially in opt_validate. but only if coming from
qemu_opts_parse or qemu_opts_parse_noisily.
Move the check from opt_validate to the common workhorses
of qemu_opts_parse and qemu_opts_parse_noisily, opts_do_parse
and get_opt_name_value.

As a result, opts_parse and opts_do_parse do not return an error anymore
when help is requested---just like qemu_opts_parse_noisily.

This will come in handy in the next patch, which will
raise a warning for "-object memory-backend-ram,share"
("flag" option with no =on/=off part) but not for
"-object memory-backend-ram,help".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-option.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index acefbc23fa..61fc96f9dd 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -504,17 +504,13 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,
     return opt;
 }
 
-static bool opt_validate(QemuOpt *opt, bool *help_wanted,
-                         Error **errp)
+static bool opt_validate(QemuOpt *opt, Error **errp)
 {
     const QemuOptDesc *desc;
 
     desc = find_desc_by_name(opt->opts->list->desc, opt->name);
     if (!desc && !opts_accepts_any(opt->opts)) {
         error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
-        if (help_wanted && is_help_option(opt->name)) {
-            *help_wanted = true;
-        }
         return false;
     }
 
@@ -531,7 +527,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
 {
     QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
 
-    if (!opt_validate(opt, NULL, errp)) {
+    if (!opt_validate(opt, errp)) {
         qemu_opt_del(opt);
         return false;
     }
@@ -767,16 +763,18 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
 
 static const char *get_opt_name_value(const char *params,
                                       const char *firstname,
+                                      bool *help_wanted,
                                       char **name, char **value)
 {
-    const char *p, *pe, *pc;
-
-    pe = strchr(params, '=');
-    pc = strchr(params, ',');
+    const char *p;
+    size_t len;
 
-    if (!pe || (pc && pc < pe)) {
+    len = strcspn(params, "=,");
+    if (params[len] != '=') {
         /* found "foo,more" */
-        if (firstname) {
+        if (help_wanted && starts_with_help_option(params) == len) {
+            *help_wanted = true;
+        } else if (firstname) {
             /* implicitly named first option */
             *name = g_strdup(firstname);
             p = get_opt_value(params, value);
@@ -814,7 +812,10 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
     QemuOpt *opt;
 
     for (p = params; *p;) {
-        p = get_opt_name_value(p, firstname, &option, &value);
+        p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
+        if (help_wanted && *help_wanted) {
+            return false;
+        }
         firstname = NULL;
 
         if (!strcmp(option, "id")) {
@@ -825,7 +826,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
 
         opt = opt_create(opts, option, value, prepend);
         g_free(option);
-        if (!opt_validate(opt, help_wanted, errp)) {
+        if (!opt_validate(opt, errp)) {
             qemu_opt_del(opt);
             return false;
         }
@@ -840,7 +841,7 @@ static char *opts_parse_id(const char *params)
     char *name, *value;
 
     for (p = params; *p;) {
-        p = get_opt_name_value(p, NULL, &name, &value);
+        p = get_opt_name_value(p, NULL, NULL, &name, &value);
         if (!strcmp(name, "id")) {
             g_free(name);
             return value;
@@ -856,11 +857,10 @@ bool has_help_option(const char *params)
 {
     const char *p;
     char *name, *value;
-    bool ret;
+    bool ret = false;
 
     for (p = params; *p;) {
-        p = get_opt_name_value(p, NULL, &name, &value);
-        ret = is_help_option(name);
+        p = get_opt_name_value(p, NULL, &ret, &name, &value);
         g_free(name);
         g_free(value);
         if (ret) {
@@ -946,10 +946,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
     bool help_wanted = false;
 
     opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
-    if (err) {
+    if (!opts) {
+        assert(!!err + !!help_wanted == 1);
         if (help_wanted) {
             qemu_opts_print_help(list, true);
-            error_free(err);
         } else {
             error_report_err(err);
         }
-- 
2.26.2

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

[Paolo Bonzini]

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(-)

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.
 
 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 */
     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 */
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,
                                       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,
             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);
+                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);
         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;
@@ -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) {
@@ -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);
 }
 
 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;
     }
-- 
2.26.2

Re: [PATCH for-5.2 0/2] deprecate short-form boolean options

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201105142731.623428-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201105142731.623428-1-pbonzini@redhat.com
Subject: [PATCH for-5.2 0/2] deprecate short-form boolean options

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/160455661411.3455.4177953912304752892.stgit@pasha-ThinkPad-X280 -> patchew/160455661411.3455.4177953912304752892.stgit@pasha-ThinkPad-X280
 - [tag update]      patchew/20201105134112.25119-1-kraxel@redhat.com -> patchew/20201105134112.25119-1-kraxel@redhat.com
 * [new tag]         patchew/20201105142731.623428-1-pbonzini@redhat.com -> patchew/20201105142731.623428-1-pbonzini@redhat.com
Switched to a new branch 'test'
2b03c50 qemu-option: warn for short-form boolean options
62d2bb7 qemu-option: move help handling to get_opt_name_value

=== OUTPUT BEGIN ===
1/2 Checking commit 62d2bb78d3b3 (qemu-option: move help handling to get_opt_name_value)
2/2 Checking commit 2b03c50a991d (qemu-option: warn for short-form boolean options)
WARNING: line over 80 characters
#56: FILE: include/qemu/option.h:68:
+    bool allow_flag_options; /* Whether to warn for short-form boolean options */

ERROR: line over 90 characters
#110: FILE: util/qemu-option.c:810:
+                error_report("short-form boolean option '%s%s' deprecated", prefix, *name);

WARNING: line over 80 characters
#129: FILE: util/qemu-option.c:838:
+        p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value);

total: 1 errors, 2 warnings, 117 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201105142731.623428-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now, help options are parsed normally and then checked
> specially in opt_validate. but only if coming from
> qemu_opts_parse or qemu_opts_parse_noisily.
> Move the check from opt_validate to the common workhorses
> of qemu_opts_parse and qemu_opts_parse_noisily, opts_do_parse
> and get_opt_name_value.
>
> As a result, opts_parse and opts_do_parse do not return an error anymore
> when help is requested---just like qemu_opts_parse_noisily.
>
> This will come in handy in the next patch, which will
> raise a warning for "-object memory-backend-ram,share"
> ("flag" option with no =on/=off part) but not for
> "-object memory-backend-ram,help".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-option.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index acefbc23fa..61fc96f9dd 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -504,17 +504,13 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,
>      return opt;
>  }
>  
> -static bool opt_validate(QemuOpt *opt, bool *help_wanted,
> -                         Error **errp)
> +static bool opt_validate(QemuOpt *opt, Error **errp)
>  {
>      const QemuOptDesc *desc;
>  
>      desc = find_desc_by_name(opt->opts->list->desc, opt->name);
>      if (!desc && !opts_accepts_any(opt->opts)) {
>          error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> -        if (help_wanted && is_help_option(opt->name)) {
> -            *help_wanted = true;
> -        }
>          return false;
>      }

Two callers, one passes null (trivial: no change), one non-null (more
interesting).

Behavior before the patch is rather peculiar:

* The caller needs to opt into the help syntax by passing non-null
  @help_wanted.

* A request for help is recognized only when the option name is not
  recognized.  Two cases:

  - When @opts accepts anything, we ignore cries for help.

  - Else, we recognize it only when there is no option named "help".

* A help request is an ordinary option parameter (possibly sugared)
  where the parameter name is a "help option" (i.e. "help" or "?"), and
  the value doesn't matter.

  Examples:
  - "help=..."
  - "help" (sugar for "help=on")
  - "nohelp" (sugar for "help=off")
  - "?=..."
  - "?" (sugar for "?=on")
  - "no?" (sugar for "?=off")

* A request for help is treated as an error: we set @errp and return
  false.

>  
> @@ -531,7 +527,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
>  {
>      QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
>  
> -    if (!opt_validate(opt, NULL, errp)) {
> +    if (!opt_validate(opt, errp)) {
>          qemu_opt_del(opt);
>          return false;

This is the trivial caller.

>      }
> @@ -767,16 +763,18 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
>  
>  static const char *get_opt_name_value(const char *params,
>                                        const char *firstname,
> +                                      bool *help_wanted,
>                                        char **name, char **value)
>  {
> -    const char *p, *pe, *pc;
> -
> -    pe = strchr(params, '=');
> -    pc = strchr(params, ',');
> +    const char *p;
> +    size_t len;
>  
> -    if (!pe || (pc && pc < pe)) {
> +    len = strcspn(params, "=,");
> +    if (params[len] != '=') {
>          /* found "foo,more" */
> -        if (firstname) {
> +        if (help_wanted && starts_with_help_option(params) == len) {
> +            *help_wanted = true;
> +        } else if (firstname) {
>              /* implicitly named first option */
>              *name = g_strdup(firstname);
>              p = get_opt_value(params, value);

This function parses one parameter from @params into @name, @value, and
returns a pointer to the next parameter, or else to the terminating
'\0'.

Funny: it cannot fail.  QemuOpts is an indiscriminate omnivore ;)

The patch does two separate things:

1. It streamlines how we look ahead to '=', ',' or '\0'.

   Three cases: '=' comes first, '-' comes first, '\0' comes first.

   Before the patch: !pe || (pc && pc < pe) means there is no '=', or
   else there is ',' and it's before '='.  In other words, '=' does not
   come first.

   After the patch: params[len] != '=' where len = strcspn(params, "=,")
   means '=' does not come first.

   Okay, but make it a separate patch, please.

   The ,more in both comments is slightly misleading.  Observation, not
   demand.

2. Optional parsing of help (opt in by passing non-null @help_wanted).

   I wonder why this is opt-in.  I trust that'll become clear further
   down.

   If @params starts with "help option", and it's followed by ',' or
   '\0', set *help_wanted instead of *name and *value.  Okay.

   The function needed a written contract before, and now it needs it
   more.  Observation, not demand.

> @@ -814,7 +812,10 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
>      QemuOpt *opt;
>  
>      for (p = params; *p;) {
> -        p = get_opt_name_value(p, firstname, &option, &value);
> +        p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
> +        if (help_wanted && *help_wanted) {
> +            return false;
> +        }
>          firstname = NULL;
>  
>          if (!strcmp(option, "id")) {
> @@ -825,7 +826,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
>  
>          opt = opt_create(opts, option, value, prepend);
>          g_free(option);
> -        if (!opt_validate(opt, help_wanted, errp)) {
> +        if (!opt_validate(opt, errp)) {
>              qemu_opt_del(opt);
>              return false;
>          }

This is the interesting caller.

Before the patch:

* Success: add an option paramter to @opts, return true.

* Help wanted (only if caller opts in): set *help_wanted, set error,
  return false

* Error: set error, return false.

The patch changes two things:

1. We no longer set an error when the user asks for help.  Checking the
   callers:

   - qemu_opts_do_parse() is not affected, because it passes null
     @help_wanted.

   - opts_parse() passes on the change to its callers:

     * qemu_opts_parse() is not affected, because it passes null
       @help_wanted.

     * qemu_opts_parse_noisily() is affected; see below.

2. We only recognize "help" and "?".  We no longer recognize
   "help=...". "?=...", "nohelp", and "no?".  I'm okay with the change,
   but it needs to be explained in the commit message.  You decide
   whether to cover it in release notes.

> @@ -840,7 +841,7 @@ static char *opts_parse_id(const char *params)
>      char *name, *value;
>  
>      for (p = params; *p;) {
> -        p = get_opt_name_value(p, NULL, &name, &value);
> +        p = get_opt_name_value(p, NULL, NULL, &name, &value);
>          if (!strcmp(name, "id")) {
>              g_free(name);
>              return value;

opts_parse() parses an entire option argument.  It returns the value of
option parameter "id".  Everything else gets thrown away.

Note for later: your patch makes it opt out of help syntax.

> @@ -856,11 +857,10 @@ bool has_help_option(const char *params)
>  {
>      const char *p;
>      char *name, *value;
> -    bool ret;
> +    bool ret = false;
>  
>      for (p = params; *p;) {
> -        p = get_opt_name_value(p, NULL, &name, &value);
> -        ret = is_help_option(name);
> +        p = get_opt_name_value(p, NULL, &ret, &name, &value);
>          g_free(name);
>          g_free(value);
>          if (ret) {

has_help_option() parses an entire option argument.

Same syntax change as in opts_do_parse().

> @@ -946,10 +946,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>      bool help_wanted = false;
>  
>      opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
> -    if (err) {
> +    if (!opts) {
> +        assert(!!err + !!help_wanted == 1);
>          if (help_wanted) {
>              qemu_opts_print_help(list, true);
> -            error_free(err);
>          } else {
>              error_report_err(err);
>          }

Since opts_parse() no longer sets an error when the user asks for help,
this function needs an update.  Okay.


Now let's get back to "I wonder why this is opt-in?"

Only one caller does not opt in: opts_parse_id().  I'd try making the
help syntax unconditional.  get_opt_name_value() gets a bit simpler,
opts_parse_id() may get a bit more complex.  I'd expect that to be a
good trade.


QemuOpts patches tend to look more innocent than they are.  This one's
no exception :)

Re: [PATCH 2/2] 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 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().

Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value

[Markus Armbruster]

One more thought...

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
[...]
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
[...]
>> @@ -767,16 +763,18 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
>>  
>>  static const char *get_opt_name_value(const char *params,
>>                                        const char *firstname,
>> +                                      bool *help_wanted,
>>                                        char **name, char **value)
>>  {
>> -    const char *p, *pe, *pc;
>> -
>> -    pe = strchr(params, '=');
>> -    pc = strchr(params, ',');
>> +    const char *p;
>> +    size_t len;
>>  
>> -    if (!pe || (pc && pc < pe)) {
>> +    len = strcspn(params, "=,");
>> +    if (params[len] != '=') {
>>          /* found "foo,more" */
>> -        if (firstname) {
>> +        if (help_wanted && starts_with_help_option(params) == len) {
>> +            *help_wanted = true;
>> +        } else if (firstname) {
>>              /* implicitly named first option */
>>              *name = g_strdup(firstname);
>>              p = get_opt_value(params, value);
>
> This function parses one parameter from @params into @name, @value, and
> returns a pointer to the next parameter, or else to the terminating
> '\0'.

Like opt_validate() before, this sets *help_wanted only to true.
Callers must pass a pointer to false.  Perhaps having it set
*help_wanted always could simplify things overall.  Up to you.

[...]

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

[Paolo Bonzini]

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

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

[Markus Armbruster]

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.