| Message ID | 20201105142731.623428-1-pbonzini@redhat.com |
|---|---|
| Headers | show |
| Series | deprecate short-form boolean options | expand |
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
Paolo Bonzini <pbonzini@redhat.com> writes: > Options such as "server" or "nowait", that are commonly found in -chardev, > are sugar for "server=on" and "wait=off". This is quite surprising and > also does not have any notion of typing attached. It is even possible to > do "-device e1000,noid" and get a device with "id=off". > > Deprecate all this, except for -chardev and -spice where it is in > wide use. I consider this a misuse of deprecation, to be frank. If something is known to be unused, we just remove it. Deprecation is precisely for things that are used. I'm with Daniel here: let's deprecate this sugar everywhere. Wide use may justify extending the deprecation grace period. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > chardev/char.c | 1 + > docs/system/deprecated.rst | 7 +++++++ > include/qemu/option.h | 1 + > tests/test-qemu-opts.c | 1 + > ui/spice-core.c | 1 + > util/qemu-option.c | 21 ++++++++++++++------- > 6 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/chardev/char.c b/chardev/char.c > index 78553125d3..108da615df 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -829,6 +829,7 @@ Chardev *qemu_chr_find(const char *name) > > QemuOptsList qemu_chardev_opts = { > .name = "chardev", > + .allow_flag_options = true, /* server, nowait, etc. */ > .implied_opt_name = "backend", > .head = QTAILQ_HEAD_INITIALIZER(qemu_chardev_opts.head), > .desc = { > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 32a0e620db..0e7edf7e56 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -146,6 +146,13 @@ Drives with interface types other than ``if=none`` are for onboard > devices. It is possible to use drives the board doesn't pick up with > -device. This usage is now deprecated. Use ``if=none`` instead. > > +Short-form boolean options (since 5.2) > +'''''''''''''''''''''''''''''''''''''' > + > +Boolean options such as ``share=on``/``share=off`` can be written > +in short form as ``share`` and ``noshare``. This is deprecated > +for all command-line options except ``-chardev` and ``-spice``, for > +which the short form was in wide use. Unlike the commit message, the text here misleads readers into assuming the sugar applies only to boolean options. > > QEMU Machine Protocol (QMP) commands > ------------------------------------ > diff --git a/include/qemu/option.h b/include/qemu/option.h > index ac69352e0e..046ac06a1f 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -65,6 +65,7 @@ struct QemuOptsList { > const char *name; > const char *implied_opt_name; > bool merge_lists; /* Merge multiple uses of option into a single list? */ > + bool allow_flag_options; /* Whether to warn for short-form boolean options */ I disagree with having this option. I'm reviewing it anyway. Staying under the 80 characters limit is really, really easy here: bool allow_flag_options; /* Warn on short-form boolean options? */ I had to read ahead to figure out that false means warn, true means accept silently. The comment is confusing because "whether to warn" suggests "warn if true", which is wrong. Clearer (I think), and even shorter: bool allow_bool_sugar; /* Is boolean option sugar okay? */ > QTAILQ_HEAD(, QemuOpts) head; > QemuOptDesc desc[]; > }; > diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c > index 297ffe79dd..a74c475773 100644 > --- a/tests/test-qemu-opts.c > +++ b/tests/test-qemu-opts.c > @@ -77,6 +77,7 @@ static QemuOptsList opts_list_02 = { > static QemuOptsList opts_list_03 = { > .name = "opts_list_03", > .implied_opt_name = "implied", > + .allow_flag_options = true, > .head = QTAILQ_HEAD_INITIALIZER(opts_list_03.head), > .desc = { > /* no elements => accept any params */ Can you point me to the tests this hunk affects? Do we have test coverage for boolean sugar with both values of allow_flag_options? If no, why is that okay? > diff --git a/ui/spice-core.c b/ui/spice-core.c > index eea52f5389..08f834fa41 100644 > --- a/ui/spice-core.c > +++ b/ui/spice-core.c > @@ -397,6 +397,7 @@ static SpiceChannelList *qmp_query_spice_channels(void) > > static QemuOptsList qemu_spice_opts = { > .name = "spice", > + .allow_flag_options = true, /* disable-agent-file-xfer, sasl, unix, etc. */ > .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head), > .merge_lists = true, > .desc = { > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 61fc96f9dd..858860377b 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -763,10 +763,12 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) > > static const char *get_opt_name_value(const char *params, > const char *firstname, > + bool warn_on_flag, Two callers pass false, the other passes !list->allow_flag_options. Having both allow_flag_options and warn_on_flag with the opposite sense is confusing. Please pick one. > bool *help_wanted, > char **name, char **value) > { > const char *p; > + const char *prefix = ""; > size_t len; > > len = strcspn(params, "=,"); > @@ -784,9 +786,14 @@ static const char *get_opt_name_value(const char *params, /* found "foo,more" */ if (firstname) { /* implicitly named first option */ *name = g_strdup(firstname); p = get_opt_value(params, value); } else { /* option without value, must be a flag */ p = get_opt_name(params, name, ','); > if (strncmp(*name, "no", 2) == 0) { > memmove(*name, *name + 2, strlen(*name + 2) + 1); > *value = g_strdup("off"); > + prefix = "no"; > } else { > *value = g_strdup("on"); > } > + if (warn_on_flag) { > + error_report("short-form boolean option '%s%s' deprecated", prefix, *name); warn_report(), please. error_report() is strictly for error conditions, i.e. things that fail. > + error_printf("Please use %s=%s instead\n", *name, *value); > + } > } > } else { > /* found "foo=bar,more" */ > @@ -805,14 +812,14 @@ static const char *get_opt_name_value(const char *params, > > static bool opts_do_parse(QemuOpts *opts, const char *params, > const char *firstname, bool prepend, > - bool *help_wanted, Error **errp) > + bool warn_on_flag, bool *help_wanted, Error **errp) > { > char *option, *value; > const char *p; > QemuOpt *opt; > > for (p = params; *p;) { > - p = get_opt_name_value(p, firstname, help_wanted, &option, &value); > + p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value); Long line. Please break the line before the output arguments: p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value); > if (help_wanted && *help_wanted) { > return false; > } > @@ -841,7 +848,7 @@ static char *opts_parse_id(const char *params) > char *name, *value; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, NULL, &name, &value); > + p = get_opt_name_value(p, NULL, false, NULL, &name, &value); > if (!strcmp(name, "id")) { > g_free(name); > return value; Yes, we want don't want to warn here. > @@ -860,7 +867,7 @@ bool has_help_option(const char *params) > bool ret = false; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, &ret, &name, &value); > + p = get_opt_name_value(p, NULL, false, &ret, &name, &value); > g_free(name); > g_free(value); > if (ret) { Likewise. > @@ -880,7 +887,7 @@ bool has_help_option(const char *params) > bool qemu_opts_do_parse(QemuOpts *opts, const char *params, > const char *firstname, Error **errp) > { > - return opts_do_parse(opts, params, firstname, false, NULL, errp); > + return opts_do_parse(opts, params, firstname, false, false, NULL, errp); Can you explain why this one doesn't warn? > } > > static QemuOpts *opts_parse(QemuOptsList *list, const char *params, > @@ -908,8 +915,8 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, > return NULL; > } > > - if (!opts_do_parse(opts, params, firstname, defaults, help_wanted, > - errp)) { > + if (!opts_do_parse(opts, params, firstname, defaults, > + !list->allow_flag_options, help_wanted, errp)) { > qemu_opts_del(opts); > return NULL; > } Here's a funny one: void qemu_opts_set_defaults(QemuOptsList *list, const char *params, int permit_abbrev) { QemuOpts *opts; opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL); assert(opts); } If someone manages to put bool sugar into machine_class->default_machine_opts, we'll print a confusing warning on startup. It's a programming error, which means we should abort().
On 06/11/20 17:49, Markus Armbruster wrote: >> Deprecate all this, except for -chardev and -spice where it is in >> wide use. > I consider this a misuse of deprecation, to be frank. If something is > known to be unused, we just remove it. Deprecation is precisely for > things that are used. I'm with Daniel here: let's deprecate this sugar > everywhere. > > Wide use may justify extending the deprecation grace period. Fair enough. However now that I think of it I'd have to remove the coverage of the "feature" in tests, because they'd warn. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 06/11/20 17:49, Markus Armbruster wrote: >>> Deprecate all this, except for -chardev and -spice where it is in >>> wide use. >> I consider this a misuse of deprecation, to be frank. If something is >> known to be unused, we just remove it. Deprecation is precisely for >> things that are used. I'm with Daniel here: let's deprecate this sugar >> everywhere. >> Wide use may justify extending the deprecation grace period. > > Fair enough. However now that I think of it I'd have to remove the > coverage of the "feature" in tests, because they'd warn. Either that, or do what we do elsewhere when warnings get in the way of testing: suppress them when testing. I wouldn't bother here unless it's easy.