| Message ID | 20201103151452.416784-1-pbonzini@redhat.com |
|---|---|
| Headers | show |
| Series | deprecate short-form boolean options | expand |
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
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 :|
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
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
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
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
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 > > <div dir="auto">I will just drop autofree usage completely, also because valgrind showed that GRegex does not support it and apparently is leaked.<div dir="auto"><br></div><div dir="auto">Paolo</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il mer 4 nov 2020, 08:44 Thomas Huth <<a href="mailto:thuth@redhat.com">thuth@redhat.com</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 03/11/2020 16.14, Paolo Bonzini wrote:<br> > device-introspect-test uses HMP, so it should escape the device name<br> > properly. Because of this, a few devices that had commas in their<br> > names were escaping testing.<br> > <br> > Signed-off-by: Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank" rel="noreferrer">pbonzini@redhat.com</a>><br> > ---<br> > tests/qtest/device-introspect-test.c | 9 ++++++---<br> > 1 file changed, 6 insertions(+), 3 deletions(-)<br> > <br> > diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c<br> > index 9f22340ee5..f471b0e0dd 100644<br> > --- a/tests/qtest/device-introspect-test.c<br> > +++ b/tests/qtest/device-introspect-test.c<br> > @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool abstract)<br> > static void test_one_device(QTestState *qts, const char *type)<br> > {<br> > QDict *resp;<br> > - char *help;<br> > + g_autofree char *help;<br> > + g_autofree GRegex *comma;<br> > + g_autofree char *escaped;<br> > <br> > g_test_message("Testing device '%s'", type);<br> > <br> > @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const char *type)<br> > type);<br> > qobject_unref(resp);<br> > <br> > - help = qtest_hmp(qts, "device_add \"%s,help\"", type);<br> > - g_free(help);<br> > + comma = g_regex_new(",", 0, 0, NULL);<br> > + escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);<br> > + help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);<br> > }<br> <br> Having "help =" as final statement now, this looks somewhat weird at a first<br> glance (until you look at the g_autofree at the beginning of the function).<br> Maybe it's better to drop the help variable completely and just do:<br> g_free(gtest_hmp(...)) ?<br> <br> Thomas<br> <br> </blockquote></div>
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 > >
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.
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
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.
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?
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
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.