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

[PATCH for-5.2 0/4] 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 4 in this series deprecates it for all other option
groups.  The first three patches avoid emitting the warning
in tests (which in one case were buggy, see patch 3) or
for the "help" option.

Paolo Bonzini (4):
  ivshmem-test: do not use short-form boolean option
  qemu-option: move help handling to get_opt_name_value
  qtest: escape device name in device-introspect-test
  qemu-option: warn for short-form boolean options

 chardev/char.c                       |  1 +
 docs/system/deprecated.rst           |  7 ++++
 include/qemu/option.h                |  1 +
 tests/qtest/device-introspect-test.c |  9 +++--
 tests/qtest/ivshmem-test.c           |  2 +-
 tests/test-qemu-opts.c               |  1 +
 ui/spice-core.c                      |  1 +
 util/qemu-option.c                   | 51 ++++++++++++++++------------
 8 files changed, 48 insertions(+), 25 deletions(-)

-- 
2.26.2

[PATCH for-5.2 1/4] ivshmem-test: do not use short-form boolean option

[Paolo Bonzini]

This QemuOpts idiom will be deprecated, so get rid of it in the tests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qtest/ivshmem-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index d5c8b9f128..dfa69424ed 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -135,7 +135,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
 static void setup_vm(IVState *s)
 {
     char *cmd = g_strdup_printf("-object memory-backend-file"
-                                ",id=mb1,size=1M,share,mem-path=/dev/shm%s"
+                                ",id=mb1,size=1M,share=on,mem-path=/dev/shm%s"
                                 " -device ivshmem-plain,memdev=mb1", tmpshm);
 
     setup_vm_cmd(s, cmd, false);
-- 
2.26.2

[PATCH for-5.2 2/4] 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, not if coming
from qemu_opt_set.

Instead, 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.

This will come in handy in a subsequent 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 | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index b9f93a7f8b..5824b7afe2 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -520,17 +520,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;
     }
 
@@ -547,7 +543,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;
     }
@@ -783,14 +779,17 @@ 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] != '=') {
+        if (help_wanted && starts_with_help_option(params) == len) {
+            *help_wanted = true;
+        }
         /* found "foo,more" */
         if (firstname) {
             /* implicitly named first option */
@@ -830,7 +829,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")) {
@@ -841,7 +843,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;
         }
@@ -856,7 +858,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;
@@ -872,11 +874,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) {
-- 
2.26.2

[PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Paolo Bonzini]

device-introspect-test uses HMP, so it should escape the device name
properly.  Because of this, a few devices that had commas in their
names were escaping testing.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qtest/device-introspect-test.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
index 9f22340ee5..f471b0e0dd 100644
--- a/tests/qtest/device-introspect-test.c
+++ b/tests/qtest/device-introspect-test.c
@@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool abstract)
 static void test_one_device(QTestState *qts, const char *type)
 {
     QDict *resp;
-    char *help;
+    g_autofree char *help;
+    g_autofree GRegex *comma;
+    g_autofree char *escaped;
 
     g_test_message("Testing device '%s'", type);
 
@@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const char *type)
                type);
     qobject_unref(resp);
 
-    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
-    g_free(help);
+    comma = g_regex_new(",", 0, 0, NULL);
+    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
+    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
 }
 
 static void test_device_intro_list(void)
-- 
2.26.2

[PATCH for-5.2 4/4] 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         | 22 +++++++++++++++-------
 6 files changed, 26 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 5824b7afe2..8b6050fbf7 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -779,16 +779,19 @@ 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, "=,");
     if (params[len] != '=') {
         if (help_wanted && starts_with_help_option(params) == len) {
             *help_wanted = true;
+            warn_on_flag = false;
         }
         /* found "foo,more" */
         if (firstname) {
@@ -801,9 +804,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" */
@@ -822,14 +830,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;
         }
@@ -858,7 +866,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;
@@ -877,7 +885,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) {
@@ -897,7 +905,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,
@@ -925,8 +933,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/4] deprecate short-form boolean options

[no-reply]

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



Hi,

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

Type: series
Message-id: 20201103151452.416784-1-pbonzini@redhat.com
Subject: [PATCH for-5.2 0/4] 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
 * [new tag]         patchew/20201103151452.416784-1-pbonzini@redhat.com -> patchew/20201103151452.416784-1-pbonzini@redhat.com
Switched to a new branch 'test'
1b42d99 qemu-option: warn for short-form boolean options
9927d00 qtest: escape device name in device-introspect-test
dd427b7 qemu-option: move help handling to get_opt_name_value
e1273b2 ivshmem-test: do not use short-form boolean option

=== OUTPUT BEGIN ===
1/4 Checking commit e1273b2eab2e (ivshmem-test: do not use short-form boolean option)
2/4 Checking commit dd427b742e3b (qemu-option: move help handling to get_opt_name_value)
3/4 Checking commit 9927d0090494 (qtest: escape device name in device-introspect-test)
4/4 Checking commit 1b42d9947c18 (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
#117: FILE: util/qemu-option.c:812:
+                error_report("short-form boolean option '%s%s' deprecated", prefix, *name);

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

total: 1 errors, 2 warnings, 124 lines checked

Patch 4/4 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/20201103151452.416784-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 for-5.2 4/4] qemu-option: warn for short-form boolean options

[Daniel P. Berrangé]

On Tue, Nov 03, 2020 at 10:14:52AM -0500, Paolo Bonzini wrote:
> 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         | 22 +++++++++++++++-------
>  6 files changed, 26 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.

So IIUC, the short form was possible to use for absolutely /any/
boolean property ?

IMHO if we're going to deprecate short forms, we should do it
universally including chardev and spice. Arguably spice/chardev
are the most important ones to give an explicit warning about
precisely because their widespread usage means a heads up is
important to users.  For chardev in particular it is possible
we might end up wanting to wait longer than the usual 2 cycles
before removal. So if we're serious about removing the short
forms long term, the sooner we deprecate & warn the better
for chardev.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Paolo Bonzini]

On 03/11/20 17:08, Daniel P. Berrangé wrote:
>> +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.
> 
> So IIUC, the short form was possible to use for absolutely /any/
> boolean property ?

s/boolean// (yikes)

> IMHO if we're going to deprecate short forms, we should do it
> universally including chardev and spice. Arguably spice/chardev
> are the most important ones to give an explicit warning about
> precisely because their widespread usage means a heads up is
> important to users.

Chardevs will probably become user-creatable objects; for -spice I was
hoping that it would be QAPIfied as "-display spice" which does not
support short forms, but I'm not sure if Gerd agrees.  In both cases,
the problem would be taken care of in a different way.

I can certainly warn for all of them, but I was thinking of the
lowest-impact option for 5.2 since we're already in soft freeze.

Paolo

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

[Igor Mammedov]

On Tue, 3 Nov 2020 16:08:43 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Nov 03, 2020 at 10:14:52AM -0500, Paolo Bonzini wrote:
> > 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         | 22 +++++++++++++++-------
> >  6 files changed, 26 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.  
> 
> So IIUC, the short form was possible to use for absolutely /any/
> boolean property ?
> 
> IMHO if we're going to deprecate short forms, we should do it
> universally including chardev and spice. Arguably spice/chardev
> are the most important ones to give an explicit warning about
> precisely because their widespread usage means a heads up is
> important to users.  For chardev in particular it is possible
> we might end up wanting to wait longer than the usual 2 cycles
> before removal. So if we're serious about removing the short
> forms long term, the sooner we deprecate & warn the better
> for chardev.

shall we also deprecate short forms for -cpu model,[feat|+feat|-feat]
and in the end allow only -device compatible form i.e. -cpu type,feat=[on|off]

that would let us drop custom
  x86_cpu_parse_featurestr,
  ppc_cpu_parse_featurestr,
  sparc_cpu_parse_features

and a bunch of cpu_class_by_name, where almost each target does its
magic conversion of cpu_model to the type (which ranges from various
prefix/suffix shuffling to completely ignoring cpu_model and returning
a fixed cpu type)


> Regards,
> Daniel

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

[Paolo Bonzini]

On 03/11/20 22:22, Igor Mammedov wrote:
> shall we also deprecate short forms for -cpu model,[feat|+feat|-feat]
> and in the end allow only -device compatible form i.e. -cpu type,feat=[on|off]
> 
> that would let us drop custom
>    x86_cpu_parse_featurestr,
>    ppc_cpu_parse_featurestr,
>    sparc_cpu_parse_features
> 
> and a bunch of cpu_class_by_name, where almost each target does its
> magic conversion of cpu_model to the type (which ranges from various
> prefix/suffix shuffling to completely ignoring cpu_model and returning
> a fixed cpu type)

Yes please. :)  But I would prefer someone else to do the work because I 
have quite some KVM backlog...

Paolo

Re: [PATCH for-5.2 1/4] ivshmem-test: do not use short-form boolean option

[Thomas Huth]

On 03/11/2020 16.14, Paolo Bonzini wrote:
> This QemuOpts idiom will be deprecated, so get rid of it in the tests.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qtest/ivshmem-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
> index d5c8b9f128..dfa69424ed 100644
> --- a/tests/qtest/ivshmem-test.c
> +++ b/tests/qtest/ivshmem-test.c
> @@ -135,7 +135,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
>  static void setup_vm(IVState *s)
>  {
>      char *cmd = g_strdup_printf("-object memory-backend-file"
> -                                ",id=mb1,size=1M,share,mem-path=/dev/shm%s"
> +                                ",id=mb1,size=1M,share=on,mem-path=/dev/shm%s"
>                                  " -device ivshmem-plain,memdev=mb1", tmpshm);
>  
>      setup_vm_cmd(s, cmd, false);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Thomas Huth]

On 03/11/2020 16.14, Paolo Bonzini wrote:
> device-introspect-test uses HMP, so it should escape the device name
> properly.  Because of this, a few devices that had commas in their
> names were escaping testing.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qtest/device-introspect-test.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
> index 9f22340ee5..f471b0e0dd 100644
> --- a/tests/qtest/device-introspect-test.c
> +++ b/tests/qtest/device-introspect-test.c
> @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool abstract)
>  static void test_one_device(QTestState *qts, const char *type)
>  {
>      QDict *resp;
> -    char *help;
> +    g_autofree char *help;
> +    g_autofree GRegex *comma;
> +    g_autofree char *escaped;
>  
>      g_test_message("Testing device '%s'", type);
>  
> @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const char *type)
>                 type);
>      qobject_unref(resp);
>  
> -    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
> -    g_free(help);
> +    comma = g_regex_new(",", 0, 0, NULL);
> +    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
> +    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
>  }

Having "help =" as final statement now, this looks somewhat weird at a first
glance (until you look at the g_autofree at the beginning of the function).
Maybe it's better to drop the help variable completely and just do:
g_free(gtest_hmp(...)) ?

 Thomas

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

I will just drop autofree usage completely, also because valgrind showed
that GRegex does not support it and apparently is leaked.

Paolo

Il mer 4 nov 2020, 08:44 Thomas Huth <thuth@redhat.com> ha scritto:

> On 03/11/2020 16.14, Paolo Bonzini wrote:
> > device-introspect-test uses HMP, so it should escape the device name
> > properly.  Because of this, a few devices that had commas in their
> > names were escaping testing.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  tests/qtest/device-introspect-test.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/qtest/device-introspect-test.c
> b/tests/qtest/device-introspect-test.c
> > index 9f22340ee5..f471b0e0dd 100644
> > --- a/tests/qtest/device-introspect-test.c
> > +++ b/tests/qtest/device-introspect-test.c
> > @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool
> abstract)
> >  static void test_one_device(QTestState *qts, const char *type)
> >  {
> >      QDict *resp;
> > -    char *help;
> > +    g_autofree char *help;
> > +    g_autofree GRegex *comma;
> > +    g_autofree char *escaped;
> >
> >      g_test_message("Testing device '%s'", type);
> >
> > @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const
> char *type)
> >                 type);
> >      qobject_unref(resp);
> >
> > -    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
> > -    g_free(help);
> > +    comma = g_regex_new(",", 0, 0, NULL);
> > +    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0,
> NULL);
> > +    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
> >  }
>
> Having "help =" as final statement now, this looks somewhat weird at a
> first
> glance (until you look at the g_autofree at the beginning of the function).
> Maybe it's better to drop the help variable completely and just do:
> g_free(gtest_hmp(...)) ?
>
>  Thomas
>
>

[-- Attachment #2: Type: text/html, Size: 2674 bytes --]

Re: [PATCH for-5.2 1/4] ivshmem-test: do not use short-form boolean option

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This QemuOpts idiom will be deprecated, so get rid of it in the tests.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qtest/ivshmem-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
> index d5c8b9f128..dfa69424ed 100644
> --- a/tests/qtest/ivshmem-test.c
> +++ b/tests/qtest/ivshmem-test.c
> @@ -135,7 +135,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
>  static void setup_vm(IVState *s)
>  {
>      char *cmd = g_strdup_printf("-object memory-backend-file"
> -                                ",id=mb1,size=1M,share,mem-path=/dev/shm%s"
> +                                ",id=mb1,size=1M,share=on,mem-path=/dev/shm%s"
>                                  " -device ivshmem-plain,memdev=mb1", tmpshm);
>  
>      setup_vm_cmd(s, cmd, false);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Igor Mammedov]

On Tue, 3 Nov 2020 22:41:40 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 03/11/20 22:22, Igor Mammedov wrote:
> > shall we also deprecate short forms for -cpu model,[feat|+feat|-feat]
> > and in the end allow only -device compatible form i.e. -cpu type,feat=[on|off]
> > 
> > that would let us drop custom
> >    x86_cpu_parse_featurestr,
> >    ppc_cpu_parse_featurestr,
> >    sparc_cpu_parse_features
> > 
> > and a bunch of cpu_class_by_name, where almost each target does its
> > magic conversion of cpu_model to the type (which ranges from various
> > prefix/suffix shuffling to completely ignoring cpu_model and returning
> > a fixed cpu type)  
> 
> Yes please. :)  But I would prefer someone else to do the work because I 
> have quite some KVM backlog...
> 

I'll look into it.

> Paolo
> 
> 

Re: [PATCH for-5.2 2/4] 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, not if coming
> from qemu_opt_set.
>
> Instead, 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.
>
> This will come in handy in a subsequent 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>

I'm afraid this fails my smoke test:

    $ o=`sed -n '/HAS_ARG,/s/DEF("\([^"]*\)".*/\1/p' qemu-options.hx`
    $ for i in $o; do echo "= $i"; upstream-qemu -$i help -version; done 2>&1 | egrep -v 'QEMU emulator|Copyright'

Many output differences.  False positives due to help printing lists in
random order.  Arbitrarily picked true positive:

    $ upstream-qemu -msg help
    msg options:
      guest-name=<bool (on/off)> - Prepends guest name for error messages but only if -name guest is set otherwise option is ignored

      timestamp=<bool (on/off)>
    $ echo $?
    1

regresses to silent failure.

Re: [PATCH for-5.2 2/4] qemu-option: move help handling to get_opt_name_value

[Paolo Bonzini]

On 04/11/20 13:21, Markus Armbruster wrote:
> 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, not if coming
>> from qemu_opt_set.
>>
>> Instead, 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.
>>
>> This will come in handy in a subsequent 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>
> 
> I'm afraid this fails my smoke test:
> 
>      $ o=`sed -n '/HAS_ARG,/s/DEF("\([^"]*\)".*/\1/p' qemu-options.hx`
>      $ for i in $o; do echo "= $i"; upstream-qemu -$i help -version; done 2>&1 | egrep -v 'QEMU emulator|Copyright'
> 
> Many output differences.  False positives due to help printing lists in
> random order.  Arbitrarily picked true positive:
> 
>      $ upstream-qemu -msg help
>      msg options:
>        guest-name=<bool (on/off)> - Prepends guest name for error messages but only if -name guest is set otherwise option is ignored
> 
>        timestamp=<bool (on/off)>
>      $ echo $?
>      1
> 
> regresses to silent failure.

Hmm, indeed. :/  Fortunately the fix is simple:

diff --git a/util/qemu-option.c b/util/qemu-option.c
index fcd1119a5d..5a3c287611 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -947,10 +947,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);
          }


I've queued 1 and 3 since they were reviewed already and are fixes for 
tests.  I'll run these two through the whole CI and repost.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 03/11/20 17:08, Daniel P. Berrangé wrote:
>>> +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.
>> 
>> So IIUC, the short form was possible to use for absolutely /any/
>> boolean property ?
>
> s/boolean// (yikes)

Yup.  "-device virtio-blk,drive=blk0,serial" gives you the lovely serial
number "on".

>> IMHO if we're going to deprecate short forms, we should do it
>> universally including chardev and spice. Arguably spice/chardev
>> are the most important ones to give an explicit warning about
>> precisely because their widespread usage means a heads up is
>> important to users.
>
> Chardevs will probably become user-creatable objects; for -spice I was
> hoping that it would be QAPIfied as "-display spice" which does not
> support short forms, but I'm not sure if Gerd agrees.  In both cases,
> the problem would be taken care of in a different way.

Taken care of only if we deprecate -chardev and -spice wholesale, not if
we keep them forever as sugar for -object.

> I can certainly warn for all of them, but I was thinking of the
> lowest-impact option for 5.2 since we're already in soft freeze.

I'm quite interested in getting rid of this sugar.  I'm not particular
on how exactly, and I understand your reluctance to mess with 5.2.

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> device-introspect-test uses HMP, so it should escape the device name
> properly.  Because of this, a few devices that had commas in their
> names were escaping testing.
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

$ git-grep '\.name *= *"[^"]*,' | cat
hw/block/fdc.c:    .name          = "SUNW,fdtwo"

Any others?

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Paolo Bonzini]

On 06/11/20 14:15, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> device-introspect-test uses HMP, so it should escape the device name
>> properly.  Because of this, a few devices that had commas in their
>> names were escaping testing.
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> $ git-grep '\.name *= *"[^"]*,' | cat
> hw/block/fdc.c:    .name          = "SUNW,fdtwo"
> 
> Any others?

Not that I know, but this is a bug anyway. :)

Paolo

Re: [PATCH for-5.2 3/4] qtest: escape device name in device-introspect-test

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/11/20 14:15, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> device-introspect-test uses HMP, so it should escape the device name
>>> properly.  Because of this, a few devices that had commas in their
>>> names were escaping testing.
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> $ git-grep '\.name *= *"[^"]*,' | cat
>> hw/block/fdc.c:    .name          = "SUNW,fdtwo"
>> Any others?
>
> Not that I know, but this is a bug anyway. :)

Yes, but "a few devices" made me curious.