* [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases
@ 2020-11-09 13:39 Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 1/6] qemu-option: simplify search for end of key Paolo Bonzini
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
It's very hard to make QemuOpts fail. It's also very easy
to write command lines that QemuOpts accept but make no sense.
This series deals with three cases:
- QemuOpts accepts ids even for options that are meant to be singletons.
As a result, a command line option like "-M q35,id=ff" is ignored silently.
- QemuOpts simply matches "help" or "?" against the option name to
determine whether the user asked for help. Something like "nohelp" or
"?=please" will print the help message.
- 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, as in the previous example, because
it is not type safe. In case you need confirmation, "-device e1000,noid"
will create a device with id equal to "off".
Unfortunately, this last idiom has found wide use with -chardev (think
"server,nowait") and to a lesser extent -spice, so it can only be
deprecated. The other two are removed.
Patches 1-3 are cleanups. Patches 4-6 deal with the above issues one
by one. I have a seventh patch to remove the third argument to
qemu_opts_create, but it touches a few dozen files.
Paolo
Supersedes: <20201105142731.623428-1-pbonzini@redhat.com>
Paolo Bonzini (6):
qemu-option: simplify search for end of key
qemu-option: pass QemuOptsList to opts_accepts_any
qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
qemu-option: clean up id vs. list->merge_lists
qemu-option: move help handling to get_opt_name_value
qemu-option: warn for short-form boolean options
docs/system/deprecated.rst | 6 ++
include/qemu/option.h | 3 +-
softmmu/vl.c | 19 ++---
tests/test-qemu-opts.c | 26 ++++++-
util/qemu-option.c | 149 +++++++++++++++++++------------------
5 files changed, 113 insertions(+), 90 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/6] qemu-option: simplify search for end of key
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 14:48 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any Paolo Bonzini
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Use strcspn to find an equal or comma value, and pass the result directly
to get_opt_name to avoid another strchr.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/qemu-option.c | 35 +++++++++++++----------------------
1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index acefbc23fa..ab3b58599e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -38,27 +38,19 @@
#include "qemu/help_option.h"
/*
- * Extracts the name of an option from the parameter string (p points at the
+ * Extracts the name of an option from the parameter string (@p points at the
* first byte of the option name)
*
- * The option name is delimited by delim (usually , or =) or the string end
- * and is copied into option. The caller is responsible for free'ing option
- * when no longer required.
+ * The option name is @len characters long and is copied into @option. The
+ * caller is responsible for free'ing @option when no longer required.
*
* The return value is the position of the delimiter/zero byte after the option
- * name in p.
+ * name in @p.
*/
-static const char *get_opt_name(const char *p, char **option, char delim)
+static const char *get_opt_name(const char *p, char **option, size_t len)
{
- char *offset = strchr(p, delim);
-
- if (offset) {
- *option = g_strndup(p, offset - p);
- return offset;
- } else {
- *option = g_strdup(p);
- return p + strlen(p);
- }
+ *option = g_strndup(p, len);
+ return p + len;
}
/*
@@ -769,12 +761,11 @@ static const char *get_opt_name_value(const char *params,
const char *firstname,
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) {
/* implicitly named first option */
@@ -782,7 +773,7 @@ static const char *get_opt_name_value(const char *params,
p = get_opt_value(params, value);
} else {
/* option without value, must be a flag */
- p = get_opt_name(params, name, ',');
+ p = get_opt_name(params, name, len);
if (strncmp(*name, "no", 2) == 0) {
memmove(*name, *name + 2, strlen(*name + 2) + 1);
*value = g_strdup("off");
@@ -792,7 +783,7 @@ static const char *get_opt_name_value(const char *params,
}
} else {
/* found "foo=bar,more" */
- p = get_opt_name(params, name, '=');
+ p = get_opt_name(params, name, len);
assert(*p == '=');
p++;
p = get_opt_value(p, value);
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 1/6] qemu-option: simplify search for end of key Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 15:27 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts Paolo Bonzini
` (4 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
A QemuOptsList can be of one of two kinds: either it is pre-validated, or
it accepts any key and validation happens somewhere else (typically in
a Visitor or against a list of QOM properties). opts_accepts_any
returns true if a QemuOpts instance was created from a QemuOptsList of
the latter kind, but there is no function to do the check on a QemuOptsList.
We will need it in the next patch; since almost all callers of
opts_accepts_any use opts->list anyway, simply repurpose it instead
of adding a new function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/qemu-option.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index ab3b58599e..59be4f9d21 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -460,16 +460,16 @@ static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
}
}
-static bool opts_accepts_any(const QemuOpts *opts)
+static bool opts_accepts_any(const QemuOptsList *list)
{
- return opts->list->desc[0].name == NULL;
+ return list->desc[0].name == NULL;
}
int qemu_opt_unset(QemuOpts *opts, const char *name)
{
QemuOpt *opt = qemu_opt_find(opts, name);
- assert(opts_accepts_any(opts));
+ assert(opts_accepts_any(opts->list));
if (opt == NULL) {
return -1;
@@ -500,9 +500,10 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
Error **errp)
{
const QemuOptDesc *desc;
+ const QemuOptsList *list = opt->opts->list;
- desc = find_desc_by_name(opt->opts->list->desc, opt->name);
- if (!desc && !opts_accepts_any(opt->opts)) {
+ desc = find_desc_by_name(list->desc, opt->name);
+ if (!desc && !opts_accepts_any(list)) {
error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
if (help_wanted && is_help_option(opt->name)) {
*help_wanted = true;
@@ -535,9 +536,10 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
{
QemuOpt *opt;
const QemuOptDesc *desc;
+ const QemuOptsList *list = opts->list;
- desc = find_desc_by_name(opts->list->desc, name);
- if (!desc && !opts_accepts_any(opts)) {
+ desc = find_desc_by_name(list->desc, name);
+ if (!desc && !opts_accepts_any(list)) {
error_setg(errp, QERR_INVALID_PARAMETER, name);
return false;
}
@@ -557,9 +559,10 @@ bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
{
QemuOpt *opt;
const QemuOptDesc *desc;
+ const QemuOptsList *list = opts->list;
- desc = find_desc_by_name(opts->list->desc, name);
- if (!desc && !opts_accepts_any(opts)) {
+ desc = find_desc_by_name(list->desc, name);
+ if (!desc && !opts_accepts_any(list)) {
error_setg(errp, QERR_INVALID_PARAMETER, name);
return false;
}
@@ -1110,7 +1113,7 @@ bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
{
QemuOpt *opt;
- assert(opts_accepts_any(opts));
+ assert(opts_accepts_any(opts->list));
QTAILQ_FOREACH(opt, &opts->head, next) {
opt->desc = find_desc_by_name(desc, opt->name);
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 1/6] qemu-option: simplify search for end of key Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 15:55 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists Paolo Bonzini
` (3 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
qemu_opts_set is used to create default network backends and to
parse sugar options -kernel, -initrd, -append, -bios and -dtb.
Switch the former to qemu_opts_parse, so that qemu_opts_set
is now only used on merge-lists QemuOptsList (for which it makes
the most sense indeed)... except in the testcase, which is
changed to use a merge-list QemuOptsList.
With this change we can remove the id parameter. With the
parameter always NULL, we know that qemu_opts_create cannot fail
and can pass &error_abort to it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qemu/option.h | 3 +--
softmmu/vl.c | 19 +++++++------------
tests/test-qemu-opts.c | 24 +++++++++++++++++++++---
util/qemu-option.c | 9 +++------
4 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index ac69352e0e..f73e0dc7d9 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -119,8 +119,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
int fail_if_exists, Error **errp);
void qemu_opts_reset(QemuOptsList *list);
void qemu_opts_loc_restore(QemuOpts *opts);
-bool qemu_opts_set(QemuOptsList *list, const char *id,
- const char *name, const char *value, Error **errp);
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp);
const char *qemu_opts_id(QemuOpts *opts);
void qemu_opts_set_id(QemuOpts *opts, char *id);
void qemu_opts_del(QemuOpts *opts);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index a71164494e..65607fe55e 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3107,20 +3107,16 @@ void qemu_init(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_kernel:
- qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel", optarg,
- &error_abort);
+ qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg, &error_abort);
break;
case QEMU_OPTION_initrd:
- qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd", optarg,
- &error_abort);
+ qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg, &error_abort);
break;
case QEMU_OPTION_append:
- qemu_opts_set(qemu_find_opts("machine"), NULL, "append", optarg,
- &error_abort);
+ qemu_opts_set(qemu_find_opts("machine"), "append", optarg, &error_abort);
break;
case QEMU_OPTION_dtb:
- qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
- &error_abort);
+ qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg, &error_abort);
break;
case QEMU_OPTION_cdrom:
drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
@@ -3230,8 +3226,7 @@ void qemu_init(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_bios:
- qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg,
- &error_abort);
+ qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg, &error_abort);
break;
case QEMU_OPTION_singlestep:
singlestep = 1;
@@ -4258,9 +4253,9 @@ void qemu_init(int argc, char **argv, char **envp)
if (default_net) {
QemuOptsList *net = qemu_find_opts("net");
- qemu_opts_set(net, NULL, "type", "nic", &error_abort);
+ qemu_opts_parse(net, "nic", true, &error_abort);
#ifdef CONFIG_SLIRP
- qemu_opts_set(net, NULL, "type", "user", &error_abort);
+ qemu_opts_parse(net, "user", true, &error_abort);
#endif
}
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 297ffe79dd..322b32871b 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -84,11 +84,25 @@ static QemuOptsList opts_list_03 = {
},
};
+static QemuOptsList opts_list_04 = {
+ .name = "opts_list_04",
+ .head = QTAILQ_HEAD_INITIALIZER(opts_list_04.head),
+ .merge_lists = true,
+ .desc = {
+ {
+ .name = "str3",
+ .type = QEMU_OPT_STRING,
+ },
+ { /* end of list */ }
+ },
+};
+
static void register_opts(void)
{
qemu_add_opts(&opts_list_01);
qemu_add_opts(&opts_list_02);
qemu_add_opts(&opts_list_03);
+ qemu_add_opts(&opts_list_04);
}
static void test_find_unknown_opts(void)
@@ -402,17 +416,21 @@ static void test_qemu_opts_set(void)
QemuOpts *opts;
const char *opt;
- list = qemu_find_opts("opts_list_01");
+ list = qemu_find_opts("opts_list_04");
g_assert(list != NULL);
g_assert(QTAILQ_EMPTY(&list->head));
- g_assert_cmpstr(list->name, ==, "opts_list_01");
+ g_assert_cmpstr(list->name, ==, "opts_list_04");
/* should not find anything at this point */
opts = qemu_opts_find(list, NULL);
g_assert(opts == NULL);
/* implicitly create opts and set str3 value */
- qemu_opts_set(list, NULL, "str3", "value", &error_abort);
+ qemu_opts_set(list, "str3", "first_value", &error_abort);
+ g_assert(!QTAILQ_EMPTY(&list->head));
+
+ /* set it again */
+ qemu_opts_set(list, "str3", "value", &error_abort);
g_assert(!QTAILQ_EMPTY(&list->head));
/* get the just created opts */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 59be4f9d21..c88e159f18 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -665,15 +665,12 @@ void qemu_opts_loc_restore(QemuOpts *opts)
loc_restore(&opts->loc);
}
-bool qemu_opts_set(QemuOptsList *list, const char *id,
- const char *name, const char *value, Error **errp)
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp)
{
QemuOpts *opts;
- opts = qemu_opts_create(list, id, 1, errp);
- if (!opts) {
- return false;
- }
+ assert(list->merge_lists);
+ opts = qemu_opts_create(list, NULL, 0, &error_abort);
return qemu_opt_set(opts, name, value, errp);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
` (2 preceding siblings ...)
2020-11-09 13:39 ` [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 16:56 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value Paolo Bonzini
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Forbid ids if the option is intended to be a singleton, as indicated by
list->merge_lists. This avoids that "./qemu-system-x86_64 -M q35,id=ff"
uses a "pc" machine type. Instead it errors out. The affected options
are "qemu-img reopen -o", "qemu-io open -o", -rtc, -M, -boot, -name,
-m, -icount, -smp, -spice.
qemu_opts_create's fail_if_exists parameter is now unnecessary:
- it is unused if id is NULL
- opts_parse only passes false if reached from qemu_opts_set_defaults,
in which case this patch enforces that id must be NULL
- other callers that can pass a non-NULL id always set it to true
Assert that it is true in the only case where "fail_if_exists" matters,
i.e. "id && !lists->merge_lists". This means that if an id is present,
duplicates are always forbidden, which was already the status quo.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/qemu-option.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index c88e159f18..91f4120ce1 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -619,7 +619,17 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
{
QemuOpts *opts = NULL;
- if (id) {
+ if (list->merge_lists) {
+ if (id) {
+ error_setg(errp, QERR_INVALID_PARAMETER, "id");
+ return NULL;
+ }
+ opts = qemu_opts_find(list, NULL);
+ if (opts) {
+ return opts;
+ }
+ } else if (id) {
+ assert(fail_if_exists);
if (!id_wellformed(id)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
"an identifier");
@@ -629,17 +639,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
}
opts = qemu_opts_find(list, id);
if (opts != NULL) {
- if (fail_if_exists && !list->merge_lists) {
- error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
- return NULL;
- } else {
- return opts;
- }
- }
- } else if (list->merge_lists) {
- opts = qemu_opts_find(list, NULL);
- if (opts) {
- return opts;
+ error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
+ return NULL;
}
}
opts = g_malloc0(sizeof(*opts));
@@ -893,7 +894,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
* (if unlikely) future misuse:
*/
assert(!defaults || list->merge_lists);
- opts = qemu_opts_create(list, id, !defaults, errp);
+ opts = qemu_opts_create(list, id, !list->merge_lists, errp);
g_free(id);
if (opts == NULL) {
return NULL;
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
` (3 preceding siblings ...)
2020-11-09 13:39 ` [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 19:40 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 6/6] qemu-option: warn for short-form boolean options Paolo Bonzini
2020-11-09 13:54 ` [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases no-reply
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Right now, help options are parsed normally and then checked
specially in opt_validate, but only if coming from
qemu_opts_parse_noisily. has_help_option does the check on its own.
Move the check from opt_validate to the parsing workhorse of QemuOpts,
get_opt_name_value. 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".
As a result:
- opts_parse and opts_do_parse do not return an error anymore
when help is requested; qemu_opts_parse_noisily does not have
to work around that anymore.
- various crazy ways to request help are not recognized anymore:
- "help=..."
- "nohelp" (sugar for "help=off")
- "?=..."
- "no?" (sugar for "?=off")
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/qemu-option.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 91f4120ce1..0ddf1f7b45 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -496,8 +496,7 @@ 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;
const QemuOptsList *list = opt->opts->list;
@@ -505,9 +504,6 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
desc = find_desc_by_name(list->desc, opt->name);
if (!desc && !opts_accepts_any(list)) {
error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
- if (help_wanted && is_help_option(opt->name)) {
- *help_wanted = true;
- }
return false;
}
@@ -524,7 +520,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;
}
@@ -760,10 +756,12 @@ 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;
size_t len;
+ bool is_help = false;
len = strcspn(params, "=,");
if (params[len] != '=') {
@@ -780,6 +778,7 @@ static const char *get_opt_name_value(const char *params,
*value = g_strdup("off");
} else {
*value = g_strdup("on");
+ is_help = is_help_option(*name);
}
}
} else {
@@ -791,6 +790,9 @@ static const char *get_opt_name_value(const char *params,
}
assert(!*p || *p == ',');
+ if (help_wanted && is_help) {
+ *help_wanted = true;
+ }
if (*p == ',') {
p++;
}
@@ -806,7 +808,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")) {
@@ -817,7 +822,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;
}
@@ -832,7 +837,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;
@@ -851,8 +856,7 @@ bool has_help_option(const char *params)
bool ret;
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) {
@@ -937,11 +941,13 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
QemuOpts *opts;
bool help_wanted = false;
- opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
- if (err) {
+ opts = opts_parse(list, params, permit_abbrev, false,
+ opts_accepts_any(list) ? NULL : &help_wanted,
+ &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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 6/6] qemu-option: warn for short-form boolean options
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
` (4 preceding siblings ...)
2020-11-09 13:39 ` [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value Paolo Bonzini
@ 2020-11-09 13:39 ` Paolo Bonzini
2020-11-09 21:19 ` Markus Armbruster
2020-11-09 13:54 ` [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases no-reply
6 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
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 it and print a warning when it is encountered. In general,
this short form for boolean options only seems to be in wide use for
-chardev and -spice.
The extra boolean argument is not my favorite. In 6.0 I plan to remove
qemu_opts_set_defaults by switching -M to keyval, and therefore quite
a bit of QemuOpts code will go away.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/system/deprecated.rst | 6 ++++++
tests/test-qemu-opts.c | 2 +-
util/qemu-option.c | 29 ++++++++++++++++++-----------
3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 8c1dc7645d..f45938a5ff 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,12 @@ 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
+and will cause a warning.
QEMU Machine Protocol (QMP) commands
------------------------------------
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 322b32871b..e12fb51032 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -519,7 +519,7 @@ static void test_opts_parse(void)
error_free_or_abort(&err);
g_assert(!opts);
- /* Implied value */
+ /* Implied value (qemu_opts_parse does not warn) */
opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=",
false, &error_abort);
g_assert_cmpuint(opts_count(opts), ==, 3);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 0ddf1f7b45..23238f00ea 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -756,10 +756,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;
bool is_help = false;
@@ -776,10 +778,15 @@ 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");
is_help = is_help_option(*name);
}
+ if (!is_help && warn_on_flag) {
+ warn_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" */
@@ -801,14 +808,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;
}
@@ -837,7 +844,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;
@@ -856,7 +863,7 @@ bool has_help_option(const char *params)
bool ret;
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) {
@@ -876,12 +883,12 @@ 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,
bool permit_abbrev, bool defaults,
- bool *help_wanted, Error **errp)
+ bool warn_on_flag, bool *help_wanted, Error **errp)
{
const char *firstname;
char *id = opts_parse_id(params);
@@ -904,8 +911,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,
+ warn_on_flag, help_wanted, errp)) {
qemu_opts_del(opts);
return NULL;
}
@@ -923,7 +930,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
bool permit_abbrev, Error **errp)
{
- return opts_parse(list, params, permit_abbrev, false, NULL, errp);
+ return opts_parse(list, params, permit_abbrev, false, false, NULL, errp);
}
/**
@@ -941,7 +948,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
QemuOpts *opts;
bool help_wanted = false;
- opts = opts_parse(list, params, permit_abbrev, false,
+ opts = opts_parse(list, params, permit_abbrev, false, true,
opts_accepts_any(list) ? NULL : &help_wanted,
&err);
if (!opts) {
@@ -960,7 +967,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
{
QemuOpts *opts;
- opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
+ opts = opts_parse(list, params, permit_abbrev, true, false, NULL, NULL);
assert(opts);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
` (5 preceding siblings ...)
2020-11-09 13:39 ` [PATCH v2 6/6] qemu-option: warn for short-form boolean options Paolo Bonzini
@ 2020-11-09 13:54 ` no-reply
6 siblings, 0 replies; 26+ messages in thread
From: no-reply @ 2020-11-09 13:54 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, armbru
Patchew URL: https://patchew.org/QEMU/20201109133931.979563-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20201109133931.979563-1-pbonzini@redhat.com
Subject: [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases
Type: series
=== 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/20201104172512.2381656-1-ehabkost@redhat.com -> patchew/20201104172512.2381656-1-ehabkost@redhat.com
- [tag update] patchew/20201105171126.88014-1-richard.henderson@linaro.org -> patchew/20201105171126.88014-1-richard.henderson@linaro.org
- [tag update] patchew/20201105221905.1350-1-dbuono@linux.vnet.ibm.com -> patchew/20201105221905.1350-1-dbuono@linux.vnet.ibm.com
- [tag update] patchew/20201106124241.16950-1-vsementsov@virtuozzo.com -> patchew/20201106124241.16950-1-vsementsov@virtuozzo.com
* [new tag] patchew/20201109133931.979563-1-pbonzini@redhat.com -> patchew/20201109133931.979563-1-pbonzini@redhat.com
- [tag update] patchew/5FA41448.4040404@huawei.com -> patchew/5FA41448.4040404@huawei.com
Switched to a new branch 'test'
50c88b8 qemu-option: warn for short-form boolean options
5ab2220 qemu-option: move help handling to get_opt_name_value
fc2619d qemu-option: clean up id vs. list->merge_lists
e17617f qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
7739f06 qemu-option: pass QemuOptsList to opts_accepts_any
c1676e5 qemu-option: simplify search for end of key
=== OUTPUT BEGIN ===
1/6 Checking commit c1676e514286 (qemu-option: simplify search for end of key)
2/6 Checking commit 7739f060c567 (qemu-option: pass QemuOptsList to opts_accepts_any)
3/6 Checking commit e17617fd5081 (qemu-option: restrict qemu_opts_set to merge-lists QemuOpts)
WARNING: line over 80 characters
#32: FILE: include/qemu/option.h:122:
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp);
WARNING: line over 80 characters
#46: FILE: softmmu/vl.c:3110:
+ qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg, &error_abort);
WARNING: line over 80 characters
#51: FILE: softmmu/vl.c:3113:
+ qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg, &error_abort);
WARNING: line over 80 characters
#56: FILE: softmmu/vl.c:3116:
+ qemu_opts_set(qemu_find_opts("machine"), "append", optarg, &error_abort);
WARNING: line over 80 characters
#61: FILE: softmmu/vl.c:3119:
+ qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg, &error_abort);
ERROR: line over 90 characters
#71: FILE: softmmu/vl.c:3229:
+ qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg, &error_abort);
WARNING: line over 80 characters
#152: FILE: util/qemu-option.c:684:
+bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp)
total: 1 errors, 6 warnings, 120 lines checked
Patch 3/6 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/6 Checking commit fc2619d6e44c (qemu-option: clean up id vs. list->merge_lists)
5/6 Checking commit 5ab2220302ba (qemu-option: move help handling to get_opt_name_value)
6/6 Checking commit 50c88b8f7929 (qemu-option: warn for short-form boolean options)
WARNING: line over 80 characters
#81: FILE: util/qemu-option.c:803:
+ warn_report("short-form boolean option '%s%s' deprecated", prefix, *name);
WARNING: line over 80 characters
#100: FILE: util/qemu-option.c:834:
+ p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, &option, &value);
total: 0 errors, 2 warnings, 127 lines checked
Patch 6/6 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/20201109133931.979563-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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/6] qemu-option: simplify search for end of key
2020-11-09 13:39 ` [PATCH v2 1/6] qemu-option: simplify search for end of key Paolo Bonzini
@ 2020-11-09 14:48 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 14:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> Use strcspn to find an equal or comma value, and pass the result directly
> to get_opt_name to avoid another strchr.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/qemu-option.c | 35 +++++++++++++----------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index acefbc23fa..ab3b58599e 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -38,27 +38,19 @@
> #include "qemu/help_option.h"
>
> /*
> - * Extracts the name of an option from the parameter string (p points at the
> + * Extracts the name of an option from the parameter string (@p points at the
> * first byte of the option name)
> *
> - * The option name is delimited by delim (usually , or =) or the string end
> - * and is copied into option. The caller is responsible for free'ing option
> - * when no longer required.
> + * The option name is @len characters long and is copied into @option. The
> + * caller is responsible for free'ing @option when no longer required.
> *
> * The return value is the position of the delimiter/zero byte after the option
> - * name in p.
> + * name in @p.
> */
> -static const char *get_opt_name(const char *p, char **option, char delim)
> +static const char *get_opt_name(const char *p, char **option, size_t len)
> {
> - char *offset = strchr(p, delim);
> -
> - if (offset) {
> - *option = g_strndup(p, offset - p);
> - return offset;
> - } else {
> - *option = g_strdup(p);
> - return p + strlen(p);
> - }
> + *option = g_strndup(p, len);
> + return p + len;
> }
Hardly anything left; I believe this can be simplified further. Not a
reason to delay this series.
>
> /*
> @@ -769,12 +761,11 @@ static const char *get_opt_name_value(const char *params,
> const char *firstname,
> 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) {
> /* implicitly named first option */
> @@ -782,7 +773,7 @@ static const char *get_opt_name_value(const char *params,
> p = get_opt_value(params, value);
> } else {
> /* option without value, must be a flag */
> - p = get_opt_name(params, name, ',');
> + p = get_opt_name(params, name, len);
> if (strncmp(*name, "no", 2) == 0) {
> memmove(*name, *name + 2, strlen(*name + 2) + 1);
> *value = g_strdup("off");
> @@ -792,7 +783,7 @@ static const char *get_opt_name_value(const char *params,
> }
> } else {
> /* found "foo=bar,more" */
> - p = get_opt_name(params, name, '=');
> + p = get_opt_name(params, name, len);
> assert(*p == '=');
> p++;
> p = get_opt_value(p, value);
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any
2020-11-09 13:39 ` [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any Paolo Bonzini
@ 2020-11-09 15:27 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 15:27 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> A QemuOptsList can be of one of two kinds: either it is pre-validated, or
> it accepts any key and validation happens somewhere else (typically in
> a Visitor or against a list of QOM properties).
For a value of "typically" :)
> opts_accepts_any
> returns true if a QemuOpts instance was created from a QemuOptsList of
> the latter kind, but there is no function to do the check on a QemuOptsList.
>
> We will need it in the next patch; since almost all callers of
> opts_accepts_any use opts->list anyway,
I'm not sure I get the "since" part. Peeking ahead... you're lifting
the first -> into the callers, which is obviously safe. I *guess*
you're trying to say it's also not ugly: most callers already contain
the ->, which you reuse, so there's hardly any code duplication.
> simply repurpose it instead
> of adding a new function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/qemu-option.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index ab3b58599e..59be4f9d21 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -460,16 +460,16 @@ static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
> }
> }
>
> -static bool opts_accepts_any(const QemuOpts *opts)
> +static bool opts_accepts_any(const QemuOptsList *list)
> {
> - return opts->list->desc[0].name == NULL;
> + return list->desc[0].name == NULL;
> }
>
> int qemu_opt_unset(QemuOpts *opts, const char *name)
> {
> QemuOpt *opt = qemu_opt_find(opts, name);
>
> - assert(opts_accepts_any(opts));
> + assert(opts_accepts_any(opts->list));
>
> if (opt == NULL) {
> return -1;
Here, it's a straighforward lift of opts->.
> @@ -500,9 +500,10 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
> Error **errp)
> {
> const QemuOptDesc *desc;
> + const QemuOptsList *list = opt->opts->list;
>
> - desc = find_desc_by_name(opt->opts->list->desc, opt->name);
> - if (!desc && !opts_accepts_any(opt->opts)) {
> + desc = find_desc_by_name(list->desc, opt->name);
> + if (!desc && !opts_accepts_any(list)) {
> error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> if (help_wanted && is_help_option(opt->name)) {
> *help_wanted = true;
Here, you reuse the existing opts-> by splicing in a variable.
This isn't as obvious as the straighforward lift, and I guess this is
what made you mention "since" in the commit message.
> @@ -535,9 +536,10 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
> {
> QemuOpt *opt;
> const QemuOptDesc *desc;
> + const QemuOptsList *list = opts->list;
>
> - desc = find_desc_by_name(opts->list->desc, name);
> - if (!desc && !opts_accepts_any(opts)) {
> + desc = find_desc_by_name(list->desc, name);
> + if (!desc && !opts_accepts_any(list)) {
> error_setg(errp, QERR_INVALID_PARAMETER, name);
> return false;
> }
> @@ -557,9 +559,10 @@ bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
> {
> QemuOpt *opt;
> const QemuOptDesc *desc;
> + const QemuOptsList *list = opts->list;
>
> - desc = find_desc_by_name(opts->list->desc, name);
> - if (!desc && !opts_accepts_any(opts)) {
> + desc = find_desc_by_name(list->desc, name);
> + if (!desc && !opts_accepts_any(list)) {
> error_setg(errp, QERR_INVALID_PARAMETER, name);
> return false;
> }
> @@ -1110,7 +1113,7 @@ bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
> {
> QemuOpt *opt;
>
> - assert(opts_accepts_any(opts));
> + assert(opts_accepts_any(opts->list));
>
> QTAILQ_FOREACH(opt, &opts->head, next) {
> opt->desc = find_desc_by_name(desc, opt->name);
The commit message confused me a bit. Regardless:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
2020-11-09 13:39 ` [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts Paolo Bonzini
@ 2020-11-09 15:55 ` Markus Armbruster
2020-11-09 16:21 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 15:55 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> qemu_opts_set is used to create default network backends and to
> parse sugar options -kernel, -initrd, -append, -bios and -dtb.
> Switch the former to qemu_opts_parse, so that qemu_opts_set
> is now only used on merge-lists QemuOptsList (for which it makes
> the most sense indeed)... except in the testcase, which is
> changed to use a merge-list QemuOptsList.
>
> With this change we can remove the id parameter. With the
> parameter always NULL, we know that qemu_opts_create cannot fail
> and can pass &error_abort to it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qemu/option.h | 3 +--
> softmmu/vl.c | 19 +++++++------------
> tests/test-qemu-opts.c | 24 +++++++++++++++++++++---
> util/qemu-option.c | 9 +++------
> 4 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index ac69352e0e..f73e0dc7d9 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -119,8 +119,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
> int fail_if_exists, Error **errp);
> void qemu_opts_reset(QemuOptsList *list);
> void qemu_opts_loc_restore(QemuOpts *opts);
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> - const char *name, const char *value, Error **errp);
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp);
Long line. Please break before Error.
> const char *qemu_opts_id(QemuOpts *opts);
> void qemu_opts_set_id(QemuOpts *opts, char *id);
> void qemu_opts_del(QemuOpts *opts);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index a71164494e..65607fe55e 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3107,20 +3107,16 @@ void qemu_init(int argc, char **argv, char **envp)
> }
> break;
> case QEMU_OPTION_kernel:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "kernel", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "kernel", optarg, &error_abort);
> break;
> case QEMU_OPTION_initrd:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "initrd", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "initrd", optarg, &error_abort);
> break;
> case QEMU_OPTION_append:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "append", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "append", optarg, &error_abort);
> break;
> case QEMU_OPTION_dtb:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "dtb", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "dtb", optarg, &error_abort);
> break;
> case QEMU_OPTION_cdrom:
> drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
> @@ -3230,8 +3226,7 @@ void qemu_init(int argc, char **argv, char **envp)
> }
> break;
> case QEMU_OPTION_bios:
> - qemu_opts_set(qemu_find_opts("machine"), NULL, "firmware", optarg,
> - &error_abort);
> + qemu_opts_set(qemu_find_opts("machine"), "firmware", optarg, &error_abort);
> break;
> case QEMU_OPTION_singlestep:
> singlestep = 1;
Long lines. Please keep the line breaks.
> @@ -4258,9 +4253,9 @@ void qemu_init(int argc, char **argv, char **envp)
>
> if (default_net) {
> QemuOptsList *net = qemu_find_opts("net");
> - qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> + qemu_opts_parse(net, "nic", true, &error_abort);
> #ifdef CONFIG_SLIRP
> - qemu_opts_set(net, NULL, "type", "user", &error_abort);
> + qemu_opts_parse(net, "user", true, &error_abort);
> #endif
> }
>
Looks safe to me, but I don't quite get why you switch to
qemu_opts_parse(). The commit message explains it is "so that
qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
makes the most sense indeed)..." Is there anything wrong with using ot
on non-merge-lists QemuOptsList?
Am I missing something?
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 297ffe79dd..322b32871b 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -84,11 +84,25 @@ static QemuOptsList opts_list_03 = {
> },
> };
>
> +static QemuOptsList opts_list_04 = {
> + .name = "opts_list_04",
> + .head = QTAILQ_HEAD_INITIALIZER(opts_list_04.head),
> + .merge_lists = true,
> + .desc = {
> + {
> + .name = "str3",
> + .type = QEMU_OPT_STRING,
> + },
> + { /* end of list */ }
> + },
> +};
> +
> static void register_opts(void)
> {
> qemu_add_opts(&opts_list_01);
> qemu_add_opts(&opts_list_02);
> qemu_add_opts(&opts_list_03);
> + qemu_add_opts(&opts_list_04);
> }
>
> static void test_find_unknown_opts(void)
> @@ -402,17 +416,21 @@ static void test_qemu_opts_set(void)
> QemuOpts *opts;
> const char *opt;
>
> - list = qemu_find_opts("opts_list_01");
> + list = qemu_find_opts("opts_list_04");
> g_assert(list != NULL);
> g_assert(QTAILQ_EMPTY(&list->head));
> - g_assert_cmpstr(list->name, ==, "opts_list_01");
> + g_assert_cmpstr(list->name, ==, "opts_list_04");
>
> /* should not find anything at this point */
> opts = qemu_opts_find(list, NULL);
> g_assert(opts == NULL);
>
> /* implicitly create opts and set str3 value */
> - qemu_opts_set(list, NULL, "str3", "value", &error_abort);
> + qemu_opts_set(list, "str3", "first_value", &error_abort);
This part the commit message mentions.
> + g_assert(!QTAILQ_EMPTY(&list->head));
> +
> + /* set it again */
> + qemu_opts_set(list, "str3", "value", &error_abort);
> g_assert(!QTAILQ_EMPTY(&list->head));
This one not.
What are you trying to accomplish?
>
> /* get the just created opts */
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 59be4f9d21..c88e159f18 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -665,15 +665,12 @@ void qemu_opts_loc_restore(QemuOpts *opts)
> loc_restore(&opts->loc);
> }
>
> -bool qemu_opts_set(QemuOptsList *list, const char *id,
> - const char *name, const char *value, Error **errp)
> +bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, Error **errp)
Long line. Please break before Error.
> {
> QemuOpts *opts;
>
> - opts = qemu_opts_create(list, id, 1, errp);
> - if (!opts) {
> - return false;
> - }
> + assert(list->merge_lists);
> + opts = qemu_opts_create(list, NULL, 0, &error_abort);
> return qemu_opt_set(opts, name, value, errp);
> }
Yes, qemu_opts_create() can fail only if its second paramter is
non-null, or its thirs paramter is non-zero.
I can see quite a few such calls. Could be simplified with a wrapper
that takes just the first parameter and can't fail. Not now.
Just enough confusion on my part to withhold my R-by for now.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
2020-11-09 15:55 ` Markus Armbruster
@ 2020-11-09 16:21 ` Paolo Bonzini
2020-11-09 18:52 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 16:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 09/11/20 16:55, Markus Armbruster wrote:
>> QemuOptsList *net = qemu_find_opts("net");
>> - qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>> + qemu_opts_parse(net, "nic", true, &error_abort);
>> #ifdef CONFIG_SLIRP
>> - qemu_opts_set(net, NULL, "type", "user", &error_abort);
>> + qemu_opts_parse(net, "user", true, &error_abort);
>> #endif
>> }
>>
> Looks safe to me, but I don't quite get why you switch to
> qemu_opts_parse(). The commit message explains it is "so that
> qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
> makes the most sense indeed)..." Is there anything wrong with using ot
> on non-merge-lists QemuOptsList?
I would *expect* a function named qemu_opts_set to do two things:
1. setting an option in a merge-lists QemuOptsList, such as -kernel.
This is indeed what we mostly use qemu_opts_set for.
2. setting an option in a non-merge-lists QemuOptsList with non-NULL id,
similar to -set.
QEMU does not use qemu_opts_set for the latter (see qemu_set_option)
because it wants to use qemu_opts_find rather than qemu_opts_create. In
fact it wouldn't *work* to use qemu_opts_set for the latter because
qemu_opts_set uses fail_if_exists==1. So:
-> For non-merge-lists QemuOptsList and non-NULL id, it is
debatable that qemu_opts_set fails if the (QemuOptsList, id)
pair already exists
On the other hand, I would not *expect* qemu_opts_set to create a
non-merge-lists QemuOpts with a single option; which it does, though.
This leads us directly to:
-> For non-merge-lists QemuOptsList and NULL id, qemu_opts_set
hardly adds value over qemu_opts_parse. It does skip some
parsing and unescaping, but its two call sites don't really care.
So qemu_opts_set has warty behavior for non-merge-lists QemuOptsList if
id is non-NULL, and it's mostly pointless if id is NULL. My solution to
keeping the API as simple as possible is to limit qemu_opts_set to
merge-lists QemuOptsList. For them, it's useful (we don't want
comma-unescaping for -kernel) *and* has sane semantics.
>> + g_assert(!QTAILQ_EMPTY(&list->head));
>> +
>> + /* set it again */
>> + qemu_opts_set(list, "str3", "value", &error_abort);
>> g_assert(!QTAILQ_EMPTY(&list->head));
>
> This one not.
>
> What are you trying to accomplish?
Improve the testcase, though I should have mentioned it in the commit
message. Basically emulating "-kernel bc -kernel def".
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 13:39 ` [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists Paolo Bonzini
@ 2020-11-09 16:56 ` Markus Armbruster
2020-11-09 17:17 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 16:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> Forbid ids if the option is intended to be a singleton, as indicated by
> list->merge_lists.
Well, ->merge_lists need not imply singleton. Perhaps we only ever use
it that way. Careful review is called for.
> This avoids that "./qemu-system-x86_64 -M q35,id=ff"
> uses a "pc" machine type.
Just like any other QemuOptsList, "machine" may have any number of
QemuOpts. The ones with non-null ID happen to be ignored silently.
Known[*] trap for the unwary.
> Instead it errors out. The affected options
> are "qemu-img reopen -o",
reopen_opts in qemu-io-cmds.c
> "qemu-io open -o",
empty_opts in qemu-io.c
> -rtc, -M, -boot, -name,
> -m, -icount, -smp,
qemu_rtc_opts, qemu_machine_opts, qemu_boot_opts, qemu_name_opts,
qemu_mem_opts, qemu_icount_opts, qemu_smp_opts in vl.c
> -spice.
qemu_spice_opts in spice-core.c.
Are these all singletons?
There's also machine_opts in qemu-config.c, but that's only to make
query-command-line-options lie backward-compatibly.
>
> qemu_opts_create's fail_if_exists parameter is now unnecessary:
>
> - it is unused if id is NULL
>
> - opts_parse only passes false if reached from qemu_opts_set_defaults,
> in which case this patch enforces that id must be NULL
>
> - other callers that can pass a non-NULL id always set it to true
>
> Assert that it is true in the only case where "fail_if_exists" matters,
> i.e. "id && !lists->merge_lists". This means that if an id is present,
> duplicates are always forbidden, which was already the status quo.
Sounds like you're specializing the code (which might be good).
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/qemu-option.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index c88e159f18..91f4120ce1 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -619,7 +619,17 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
> {
> QemuOpts *opts = NULL;
>
> - if (id) {
> + if (list->merge_lists) {
> + if (id) {
> + error_setg(errp, QERR_INVALID_PARAMETER, "id");
> + return NULL;
> + }
> + opts = qemu_opts_find(list, NULL);
> + if (opts) {
> + return opts;
> + }
If lists>merge_lists, you no longer check id_wellformed(). Easy enough
to fix: lift the check before this conditional.
> + } else if (id) {
> + assert(fail_if_exists);
> if (!id_wellformed(id)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
> "an identifier");
> @@ -629,17 +639,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
> }
> opts = qemu_opts_find(list, id);
> if (opts != NULL) {
> - if (fail_if_exists && !list->merge_lists) {
> - error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
> - return NULL;
> - } else {
> - return opts;
> - }
> - }
> - } else if (list->merge_lists) {
> - opts = qemu_opts_find(list, NULL);
> - if (opts) {
> - return opts;
> + error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
> + return NULL;
> }
> }
> opts = g_malloc0(sizeof(*opts));
Paths through the function before the patch:
id fail_if_exists merge_lists | return
null don't care false | new opts
null don't care true | existing or else new opts
non-null false don't care | existing or else new opts
non-null true true | existing or else new opts
non-null true false | new opts / fail if exist
Too many cases.
After the patch:
id fail_if_exists merge_lists | return
non-null don't care true | fail
null don't care true | existing or else new opts
non-null false false | abort
non-null true false | new opts / fail if exist
null don't care false | new opts
Still too many :)
I'm running out of time for today, and I still need to convince myself
that the changes in behavior are all okay.
> @@ -893,7 +894,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> * (if unlikely) future misuse:
> */
> assert(!defaults || list->merge_lists);
> - opts = qemu_opts_create(list, id, !defaults, errp);
> + opts = qemu_opts_create(list, id, !list->merge_lists, errp);
> g_free(id);
> if (opts == NULL) {
> return NULL;
[*] Known to the QemuOpts cognoscenti, whose number could be
embarrasingly close to one.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 16:56 ` Markus Armbruster
@ 2020-11-09 17:17 ` Paolo Bonzini
2020-11-09 18:38 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 17:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 09/11/20 17:56, Markus Armbruster wrote:
> Just like any other QemuOptsList, "machine" may have any number of
> QemuOpts. The ones with non-null ID happen to be ignored silently.
> Known[*] trap for the unwary.
>
> Are these all singletons?
They are never qemu_opts_find'd with non-NULL id, so I'd say they are.
> If lists>merge_lists, you no longer check id_wellformed(). Easy enough
> to fix: lift the check before this conditional.
Intentional: we always error with INVALID_PARAMETER, so it's pointless
to check if the id is well-formed.
> After the patch:
>
> id fail_if_exists merge_lists | return
> non-null don't care true | fail
> null don't care true | existing or else new opts
> non-null false false | abort
> non-null true false | new opts / fail if exist
> null don't care false | new opts
>
> Still too many
Discounting the case that aborts as it's not user-controlled (it's
"just" a matter of inspecting qemu_opts_create callers), the rest can be
summarized as:
- merge_lists = false: singleton opts with NULL id; non-NULL id fails
- merge_lists = true: always return new opts; non-NULL id fails if dup
> [*] Known to the QemuOpts cognoscenti, whose number could be
> embarrasingly close to one.
Maybe not one, but a single hand certainly has a surplus of fingers.
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 17:17 ` Paolo Bonzini
@ 2020-11-09 18:38 ` Markus Armbruster
2020-11-09 18:59 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 18:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 09/11/20 17:56, Markus Armbruster wrote:
>> Just like any other QemuOptsList, "machine" may have any number of
>> QemuOpts. The ones with non-null ID happen to be ignored silently.
>> Known[*] trap for the unwary.
>>
>> Are these all singletons?
>
> They are never qemu_opts_find'd with non-NULL id, so I'd say they are.
We also need to check qemu_opts_foreach().
>> If lists>merge_lists, you no longer check id_wellformed(). Easy enough
>> to fix: lift the check before this conditional.
>
> Intentional: we always error with INVALID_PARAMETER, so it's pointless
> to check if the id is well-formed.
My head was about to explode, and then it farted instead. Sorry fore
the noise!
>> After the patch:
>>
>> id fail_if_exists merge_lists | return
>> non-null don't care true | fail
>> null don't care true | existing or else new opts
>> non-null false false | abort
>> non-null true false | new opts / fail if exist
>> null don't care false | new opts
>>
>> Still too many
>
> Discounting the case that aborts as it's not user-controlled (it's
> "just" a matter of inspecting qemu_opts_create callers), the rest can be
> summarized as:
>
> - merge_lists = false: singleton opts with NULL id; non-NULL id fails
Do you mean merge_lists = true here, and ...
> - merge_lists = true: always return new opts; non-NULL id fails if dup
... = false here?
>> [*] Known to the QemuOpts cognoscenti, whose number could be
>> embarrasingly close to one.
>
> Maybe not one, but a single hand certainly has a surplus of fingers.
>
> Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
2020-11-09 16:21 ` Paolo Bonzini
@ 2020-11-09 18:52 ` Markus Armbruster
2020-11-09 18:58 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 18:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 09/11/20 16:55, Markus Armbruster wrote:
>>> QemuOptsList *net = qemu_find_opts("net");
>>> - qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>>> + qemu_opts_parse(net, "nic", true, &error_abort);
>>> #ifdef CONFIG_SLIRP
>>> - qemu_opts_set(net, NULL, "type", "user", &error_abort);
>>> + qemu_opts_parse(net, "user", true, &error_abort);
>>> #endif
>>> }
>>>
>> Looks safe to me, but I don't quite get why you switch to
>> qemu_opts_parse(). The commit message explains it is "so that
>> qemu_opts_set is now only used on merge-lists QemuOptsList (for which it
>> makes the most sense indeed)..." Is there anything wrong with using ot
>> on non-merge-lists QemuOptsList?
>
> I would *expect* a function named qemu_opts_set to do two things:
>
> 1. setting an option in a merge-lists QemuOptsList, such as -kernel.
>
> This is indeed what we mostly use qemu_opts_set for.
>
>
> 2. setting an option in a non-merge-lists QemuOptsList with non-NULL
> id, similar to -set.
>
> QEMU does not use qemu_opts_set for the latter (see qemu_set_option)
> because it wants to use qemu_opts_find rather than qemu_opts_create.
> In fact it wouldn't *work* to use qemu_opts_set for the latter because
> qemu_opts_set uses fail_if_exists==1. So:
>
> -> For non-merge-lists QemuOptsList and non-NULL id, it is
> debatable that qemu_opts_set fails if the (QemuOptsList, id)
> pair already exists
>
>
> On the other hand, I would not *expect* qemu_opts_set to create a
> non-merge-lists QemuOpts with a single option; which it does, though.
> This leads us directly to:
>
> -> For non-merge-lists QemuOptsList and NULL id, qemu_opts_set
> hardly adds value over qemu_opts_parse. It does skip some
> parsing and unescaping, but its two call sites don't really care.
>
> So qemu_opts_set has warty behavior for non-merge-lists QemuOptsList
> if id is non-NULL, and it's mostly pointless if id is NULL. My
> solution to keeping the API as simple as possible is to limit
> qemu_opts_set to merge-lists QemuOptsList. For them, it's useful (we
> don't want comma-unescaping for -kernel) *and* has sane semantics.
Okay, makes sense.
Do you think working (some of) this into commit message would be worth
your while?
>>> + g_assert(!QTAILQ_EMPTY(&list->head));
>>> +
>>> + /* set it again */
>>> + qemu_opts_set(list, "str3", "value", &error_abort);
>>> g_assert(!QTAILQ_EMPTY(&list->head));
>> This one not.
>> What are you trying to accomplish?
>
> Improve the testcase, though I should have mentioned it in the commit
> message. Basically emulating "-kernel bc -kernel def".
Worth testing. But the case "-kernel bc" is also worth testing.
test_qemu_opts_get() tests both:
/* haven't set anything to str2 yet */
opt = qemu_opt_get(opts, "str2");
g_assert(opt == NULL);
qemu_opt_set(opts, "str2", "value", &error_abort);
/* now we have set str2, should know about it */
opt = qemu_opt_get(opts, "str2");
g_assert_cmpstr(opt, ==, "value");
qemu_opt_set(opts, "str2", "value2", &error_abort);
/* having reset the value, the returned should be the reset one */
opt = qemu_opt_get(opts, "str2");
g_assert_cmpstr(opt, ==, "value2");
I'm okay with not improving the test in this patch, or with strictly
extending coverage, preferably in a separate patch that goes before this
one.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts
2020-11-09 18:52 ` Markus Armbruster
@ 2020-11-09 18:58 ` Paolo Bonzini
0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 18:58 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 09/11/20 19:52, Markus Armbruster wrote:
> Do you think working (some of) this into commit message would be worth
> your while?
Easy and does not require a respin, so yes.
>> Improve the testcase, though I should have mentioned it in the commit
>> message. Basically emulating "-kernel bc -kernel def".
> Worth testing. But the case "-kernel bc" is also worth testing.
> test_qemu_opts_get() tests both:
>
> /* haven't set anything to str2 yet */
> opt = qemu_opt_get(opts, "str2");
> g_assert(opt == NULL);
>
> qemu_opt_set(opts, "str2", "value", &error_abort);
>
> /* now we have set str2, should know about it */
> opt = qemu_opt_get(opts, "str2");
> g_assert_cmpstr(opt, ==, "value");
>
> qemu_opt_set(opts, "str2", "value2", &error_abort);
>
> /* having reset the value, the returned should be the reset one */
> opt = qemu_opt_get(opts, "str2");
> g_assert_cmpstr(opt, ==, "value2");
Note opt_set vs. opts_set though.
> I'm okay with not improving the test in this patch, or with strictly
> extending coverage, preferably in a separate patch that goes before this
> one.
>
Ok, I'll just drop the testcase change for now.
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 18:38 ` Markus Armbruster
@ 2020-11-09 18:59 ` Paolo Bonzini
2020-11-10 8:29 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 18:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 09/11/20 19:38, Markus Armbruster wrote:
>> They are never qemu_opts_find'd with non-NULL id, so I'd say they are.
>
> We also need to check qemu_opts_foreach().
Using qemu_opts_foreach means that e.g. -name id=... was not ignored
unlike -M id=.... However, it will be an error now. We have to check
if the callback or its callees use the opt->id
Reminder of how the affected options are affected:
reopen_opts in qemu-io-cmds.c qemu_opts_find(&reopen_opts, NULL)
empty_opts in qemu-io.c qemu_opts_find(&empty_opts, NULL)
qemu_rtc_opts qemu_find_opts_singleton("rtc")
qemu_machine_opts qemu_find_opts_singleton("machine")
qemu_boot_opts
QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)
qemu_name_opts qemu_opts_foreach->parse_name
parse_name does not use id
qemu_mem_opts qemu_find_opts_singleton("memory")
qemu_icount_opts qemu_opts_foreach->do_configuree_icount
do_configure_icount->icount_configure
icount_configure does not use id
qemu_smp_opts
qemu_opts_find(qemu_find_opts("smp-opts"), NULL)
qemu_spice_opts QTAILQ_FIRST(&qemu_spice_opts.head)
To preempt your question, I can add this in the commit message. Anyway
I think it's relatively self-explanatory for most of these that they do
not need "id".
>> - merge_lists = false: singleton opts with NULL id; non-NULL id fails
>
> Do you mean merge_lists = true here, and ...
>
>> - merge_lists = true: always return new opts; non-NULL id fails if dup
>
> ... = false here?
Of course. 1-1 in the brain fart competition.
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value
2020-11-09 13:39 ` [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value Paolo Bonzini
@ 2020-11-09 19:40 ` Markus Armbruster
2020-11-09 19:47 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 19:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
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_noisily. has_help_option does the check on its own.
>
> Move the check from opt_validate to the parsing workhorse of QemuOpts,
> get_opt_name_value. 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".
>
> As a result:
>
> - opts_parse and opts_do_parse do not return an error anymore
> when help is requested; qemu_opts_parse_noisily does not have
> to work around that anymore.
>
> - various crazy ways to request help are not recognized anymore:
> - "help=..."
> - "nohelp" (sugar for "help=off")
> - "?=..."
> - "no?" (sugar for "?=off")
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/qemu-option.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 91f4120ce1..0ddf1f7b45 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -496,8 +496,7 @@ 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;
> const QemuOptsList *list = opt->opts->list;
> @@ -505,9 +504,6 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
> desc = find_desc_by_name(list->desc, opt->name);
> if (!desc && !opts_accepts_any(list)) {
> error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> - if (help_wanted && is_help_option(opt->name)) {
> - *help_wanted = true;
> - }
> return false;
> }
>
> @@ -524,7 +520,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;
> }
> @@ -760,10 +756,12 @@ 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;
> size_t len;
> + bool is_help = false;
>
> len = strcspn(params, "=,");
> if (params[len] != '=') {
> @@ -780,6 +778,7 @@ static const char *get_opt_name_value(const char *params,
> *value = g_strdup("off");
> } else {
> *value = g_strdup("on");
> + is_help = is_help_option(*name);
> }
> }
> } else {
> @@ -791,6 +790,9 @@ static const char *get_opt_name_value(const char *params,
> }
>
> assert(!*p || *p == ',');
> + if (help_wanted && is_help) {
> + *help_wanted = true;
> + }
Note [1] for later: we only ever set *help_wanted to true.
> if (*p == ',') {
> p++;
> }
Note [2] for later: we always set *name and *value, even when we set
*help_wanted = true.
> @@ -806,7 +808,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;
Doesn't this leak @option and @value? Remember, [2]
get_opt_name_value() always sets *name and *value.
> + }
> firstname = NULL;
>
> if (!strcmp(option, "id")) {
> @@ -817,7 +822,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;
> }
> @@ -832,7 +837,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;
This is one of two callers that passes null to help_wanted.
If both callers can pass non-null, we can simplify get_opt_name_value()
slightly.
This one could just as well pass &dummy. Removes doubt that
opts_parse_id() could parse differently than opts_do_parse().
opts_parse() relies on the two parsing the same.
> @@ -851,8 +856,7 @@ bool has_help_option(const char *params)
> bool ret;
>
> 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);
If @p doesn't contain a help request, &ret remains uninitialized.
Remember, [1] get_opt_name_value() only ever sets *help_wanted to true.
Suggest to make it set *help_wanted like this instead:
if (help_wanted) {
*help_wanted = is_help;
}
> g_free(name);
> g_free(value);
> if (ret) {
> @@ -937,11 +941,13 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
> QemuOpts *opts;
> bool help_wanted = false;
>
> - opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
> - if (err) {
> + opts = opts_parse(list, params, permit_abbrev, false,
> + opts_accepts_any(list) ? NULL : &help_wanted,
This recognizes help requests only when !opts_accepts_any().
Recall my review of v1 on opt_validate()'s behavior before the patch:
* 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.
You recreate this here. Why here?
opt_validate() has two callers: qemu_opt_set(), which passes null and is
therefore unaffected, and opts_do_parse(), which is affected.
opts_do_parse() is called by qemu_opts_do_parse(), which passes null and
is therefore unaffected, and opts_parse().
opts_parse() is called by qemu_opts_parse() and
qemu_opts_set_defaults(), which pass null and are therefore unaffected,
and qemu_opts_parse_noisily().
Okay. A more verbose commit message could've saved me the digging.
- Else, we recognize it only when there is no option named "help".
You now recognize it even when there is an option named "help". No
change as long as no such option exists. That's the case to the best of
my knowledge. But the argument belongs into the commit message.
> + &err);
> + if (!opts) {
> + assert(!!err + !!help_wanted == 1);
Either err or help_wanted. This is logical inequality. I'd write it as
assert(!err != !help_wanted);
or maybe as
assert(!err ^ !help_wanted);
> if (help_wanted) {
> qemu_opts_print_help(list, true);
> - error_free(err);
> } else {
> error_report_err(err);
> }
I think we could pass &help_wanted unconditionally, then ignore the
value of help_wanted if opts_accepts_any().
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value
2020-11-09 19:40 ` Markus Armbruster
@ 2020-11-09 19:47 ` Paolo Bonzini
0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 19:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 09/11/20 20:40, Markus Armbruster wrote:
>>
>> + p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
>> + if (help_wanted && *help_wanted) {
>> + return false;
>
> Doesn't this leak @option and @value? Remember, [2]
> get_opt_name_value() always sets *name and *value.
Yes, it does. :(
>> if (help_wanted) {
>> qemu_opts_print_help(list, true);
>> - error_free(err);
>> } else {
>> error_report_err(err);
>> }
> I think we could pass &help_wanted unconditionally, then ignore the
> value of help_wanted if opts_accepts_any().
>
Unfortunately not, because callers rely on "help" being added to the
options for !opts_accepts_any. opts_do_parse however does:
> + if (help_wanted && *help_wanted) {
> + return false;
> + }
You made the same remark on the previous version, but unfortunately I
couldn't make it actually produce simpler code.
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 6/6] qemu-option: warn for short-form boolean options
2020-11-09 13:39 ` [PATCH v2 6/6] qemu-option: warn for short-form boolean options Paolo Bonzini
@ 2020-11-09 21:19 ` Markus Armbruster
2020-11-09 21:42 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-09 21:19 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
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 it and print a warning when it is encountered. In general,
> this short form for boolean options only seems to be in wide use for
> -chardev and -spice.
>
> The extra boolean argument is not my favorite. In 6.0 I plan to remove
> qemu_opts_set_defaults by switching -M to keyval, and therefore quite
> a bit of QemuOpts code will go away.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> docs/system/deprecated.rst | 6 ++++++
> tests/test-qemu-opts.c | 2 +-
> util/qemu-option.c | 29 ++++++++++++++++++-----------
> 3 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 8c1dc7645d..f45938a5ff 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -146,6 +146,12 @@ 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
> +and will cause a warning.
>
> QEMU Machine Protocol (QMP) commands
> ------------------------------------
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 322b32871b..e12fb51032 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -519,7 +519,7 @@ static void test_opts_parse(void)
> error_free_or_abort(&err);
> g_assert(!opts);
>
> - /* Implied value */
> + /* Implied value (qemu_opts_parse does not warn) */
> opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=",
> false, &error_abort);
> g_assert_cmpuint(opts_count(opts), ==, 3);
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 0ddf1f7b45..23238f00ea 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -756,10 +756,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;
> bool is_help = false;
>
> @@ -776,10 +778,15 @@ 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");
> is_help = is_help_option(*name);
> }
> + if (!is_help && warn_on_flag) {
> + warn_report("short-form boolean option '%s%s' deprecated", prefix, *name);
> + error_printf("Please use %s=%s instead\n", *name, *value);
> + }
If @warn_on_flag, we warn except for "help" and "?". The exception
applies regardless of @help_wanted.
> }
> } else {
> /* found "foo=bar,more" */
> @@ -801,14 +808,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. Break it before the three output arguments.
> if (help_wanted && *help_wanted) {
> return false;
> }
> @@ -837,7 +844,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);
No change (we pass false to warn_on_flag).
> if (!strcmp(name, "id")) {
> g_free(name);
> return value;
> @@ -856,7 +863,7 @@ bool has_help_option(const char *params)
> bool ret;
>
> 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);
Likewise.
> g_free(name);
> g_free(value);
> if (ret) {
> @@ -876,12 +883,12 @@ 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);
Likewise.
> }
>
> static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> bool permit_abbrev, bool defaults,
> - bool *help_wanted, Error **errp)
> + bool warn_on_flag, bool *help_wanted, Error **errp)
> {
> const char *firstname;
> char *id = opts_parse_id(params);
> @@ -904,8 +911,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,
> + warn_on_flag, help_wanted, errp)) {
> qemu_opts_del(opts);
> return NULL;
> }
> @@ -923,7 +930,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
> bool permit_abbrev, Error **errp)
> {
> - return opts_parse(list, params, permit_abbrev, false, NULL, errp);
> + return opts_parse(list, params, permit_abbrev, false, false, NULL, errp);
Likewise.
> }
>
> /**
> @@ -941,7 +948,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
> QemuOpts *opts;
> bool help_wanted = false;
>
> - opts = opts_parse(list, params, permit_abbrev, false,
> + opts = opts_parse(list, params, permit_abbrev, false, true,
> opts_accepts_any(list) ? NULL : &help_wanted,
> &err);
> if (!opts) {
This function now warns, except for "help" and "?". The exception
applies even when we treat "help" and "?" as sugar for "help=on" and
"?=on" because opts_accepts_any().
> @@ -960,7 +967,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> {
> QemuOpts *opts;
>
> - opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
> + opts = opts_parse(list, params, permit_abbrev, true, false, NULL, NULL);
> assert(opts);
> }
No change (we pass false to warn_on_flag).
Summary: only qemu_opts_parse_noisily() warns. This is airtight only if
all user input flows through qemu_opts_parse_noisily(). It's too late
in my day for me to check.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 6/6] qemu-option: warn for short-form boolean options
2020-11-09 21:19 ` Markus Armbruster
@ 2020-11-09 21:42 ` Paolo Bonzini
2020-11-10 8:32 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-09 21:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 682 bytes --]
Il lun 9 nov 2020, 22:19 Markus Armbruster <armbru@redhat.com> ha scritto:
> This function now warns, except for "help" and "?". The exception
> applies even when we treat "help" and "?" as sugar for "help=on" and
> "?=on" because opts_accepts_any().
>
Right, because again help_wanted will be false for non-validated
QemuOptsList.
Summary: only qemu_opts_parse_noisily() warns. This is airtight only if
> all user input flows through qemu_opts_parse_noisily().
HMP doesn't. But that's too hard to change now, and it's not considered as
much of a stable interface as the command line.
Anyway I am not going to push this for 5.2. Thanks for the speedy reviews
anyway!
Paolo
[-- Attachment #2: Type: text/html, Size: 1415 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-09 18:59 ` Paolo Bonzini
@ 2020-11-10 8:29 ` Markus Armbruster
2020-11-10 8:39 ` Paolo Bonzini
0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2020-11-10 8:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 09/11/20 19:38, Markus Armbruster wrote:
>>> They are never qemu_opts_find'd with non-NULL id, so I'd say they are.
>>
>> We also need to check qemu_opts_foreach().
>
> Using qemu_opts_foreach means that e.g. -name id=... was not ignored
> unlike -M id=.... However, it will be an error now. We have to check
> if the callback or its callees use the opt->id
Yes.
> Reminder of how the affected options are affected:
In general, the patch rejects only options that used to be silently
ignored. As we will see below, there are exceptions where we reject
options that used to work. Do we want that?
If yes, discuss in commit message and release notes.
More below.
> reopen_opts in qemu-io-cmds.c qemu_opts_find(&reopen_opts, NULL)
qopts = qemu_opts_find(&reopen_opts, NULL);
opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
qemu_opts_reset(&reopen_opts);
I guess this could use qemu_find_opts_singleton(). Not sure we want it,
and even if we do, it's not this patch's job.
>
> empty_opts in qemu-io.c qemu_opts_find(&empty_opts, NULL)
Likewise.
> qemu_rtc_opts qemu_find_opts_singleton("rtc")
>
> qemu_machine_opts qemu_find_opts_singleton("machine")
No surprises or funnies here.
> qemu_boot_opts
> QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)
Spelled "boot-opts", and used with a variable spliced on, which defeated
my grep. It's in hw/nvram/fw_cfg.c and hw/s390x/ipl.c.
vl.c additionally has qemu_opts_find(qemu_find_opts("boot-opts"), NULL).
If the user passes multiple -boot with different ID, we merge the ones
with same ID, and then vl.c gets the (merged) one without ID, but the
other two get the (merged) one that contains the first -boot. All three
silently ignore the ones they don't get. Awesomely weird. I'd call it
a bug.
Test case:
$ upstream-qemu -S -display none -boot strict=on,id=id -boot strict=off
vl.c uses strict=off, but fw_cfg.c uses strinct=on,id=id.
Outlawing "id" with .merge_lists like this patch does turns the cases
where the two methods yield different results into errors. This could
break working (if crazy) configurations. That's okay; I can't see how
the craziness could be fixed without breaking crazy configurations.
I think the commit message should cover this.
I believe we could use qemu_find_opts_singleton() in all three spots.
Not this patch's job.
>
> qemu_name_opts qemu_opts_foreach->parse_name
> parse_name does not use id
First, we use .merge_lists to merge -name with the same ID into a single
QemuOpts, then we use code to merge the resulting QemuOpts, ignoring ID.
Stupid.
Outlawing "id" with .merge_lists like this patch does makes the second
merge a no-op.
This is one of the cases where we reject options that used to work.
If that's wanted, follow-up patch to drop the useless second merge.
If not, unset qemu_name_opts.merge_lists in a separate patch before this
one.
> qemu_mem_opts qemu_find_opts_singleton("memory")
No surprises or funnies here.
> qemu_icount_opts qemu_opts_foreach->do_configuree_icount
> do_configure_icount->icount_configure
> icount_configure does not use id
Same story as for qemu_name_opts.
> qemu_smp_opts
> qemu_opts_find(qemu_find_opts("smp-opts"), NULL)
No surprises or funnies here.
> qemu_spice_opts QTAILQ_FIRST(&qemu_spice_opts.head)
We use the merged one that contains the first -spice, and silently
ignore the rest. At least we're consistent here.
This is one of the cases where we reject options that used to work.
If that's wanted, follow-up patch to replace the QTAILQ_FIRST() by
something saner.
If not, unset qemu_spice_opts.merge_lists in a separate patch before
this one, and merge like we do for qemu_name_opts.
> To preempt your question, I can add this in the commit message. Anyway
> I think it's relatively self-explanatory for most of these that they do
> not need "id".
Except where they don't need it, but permit it to have an effect anyway.
One of the issues with QemuOpts is that there are too many "clever" ways
to use it.
>>> - merge_lists = false: singleton opts with NULL id; non-NULL id fails
>>
>> Do you mean merge_lists = true here, and ...
>>
>>> - merge_lists = true: always return new opts; non-NULL id fails if dup
>>
>> ... = false here?
>
> Of course. 1-1 in the brain fart competition.
Hah!
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 6/6] qemu-option: warn for short-form boolean options
2020-11-09 21:42 ` Paolo Bonzini
@ 2020-11-10 8:32 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-11-10 8:32 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il lun 9 nov 2020, 22:19 Markus Armbruster <armbru@redhat.com> ha scritto:
>
>> This function now warns, except for "help" and "?". The exception
>> applies even when we treat "help" and "?" as sugar for "help=on" and
>> "?=on" because opts_accepts_any().
>>
>
> Right, because again help_wanted will be false for non-validated
> QemuOptsList.
>
>> Summary: only qemu_opts_parse_noisily() warns. This is airtight only if
>> all user input flows through qemu_opts_parse_noisily().
>
>
> HMP doesn't. But that's too hard to change now, and it's not considered as
> much of a stable interface as the command line.
>
> Anyway I am not going to push this for 5.2. Thanks for the speedy reviews
> anyway!
You're welcome! It was worth a try. We can try again for 6.0 without
time pressure.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-10 8:29 ` Markus Armbruster
@ 2020-11-10 8:39 ` Paolo Bonzini
2020-11-10 9:54 ` Markus Armbruster
0 siblings, 1 reply; 26+ messages in thread
From: Paolo Bonzini @ 2020-11-10 8:39 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
On 10/11/20 09:29, Markus Armbruster wrote:
> As we will see below, there are exceptions where we reject
> options that used to work. Do we want that?
I think that, as long as we gain in consistency, we do. The special
casing of "id" is a wart of the current parser and the less it shows
that "id" is treated specially, the best.
Deprecating or downright rejecting working combinations is always
walking a fine line, but here we're almost in https://xkcd.com/1172/
territory.
Paolo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists
2020-11-10 8:39 ` Paolo Bonzini
@ 2020-11-10 9:54 ` Markus Armbruster
0 siblings, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2020-11-10 9:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 10/11/20 09:29, Markus Armbruster wrote:
>> As we will see below, there are exceptions where we reject
>> options that used to work. Do we want that?
>
> I think that, as long as we gain in consistency, we do. The special
> casing of "id" is a wart of the current parser and the less it shows
> that "id" is treated specially, the best.
>
> Deprecating or downright rejecting working combinations is always
> walking a fine line, but here we're almost in https://xkcd.com/1172/
> territory.
I find it hard to disagree ;)
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-11-10 9:55 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 13:39 [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 1/6] qemu-option: simplify search for end of key Paolo Bonzini
2020-11-09 14:48 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 2/6] qemu-option: pass QemuOptsList to opts_accepts_any Paolo Bonzini
2020-11-09 15:27 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 3/6] qemu-option: restrict qemu_opts_set to merge-lists QemuOpts Paolo Bonzini
2020-11-09 15:55 ` Markus Armbruster
2020-11-09 16:21 ` Paolo Bonzini
2020-11-09 18:52 ` Markus Armbruster
2020-11-09 18:58 ` Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 4/6] qemu-option: clean up id vs. list->merge_lists Paolo Bonzini
2020-11-09 16:56 ` Markus Armbruster
2020-11-09 17:17 ` Paolo Bonzini
2020-11-09 18:38 ` Markus Armbruster
2020-11-09 18:59 ` Paolo Bonzini
2020-11-10 8:29 ` Markus Armbruster
2020-11-10 8:39 ` Paolo Bonzini
2020-11-10 9:54 ` Markus Armbruster
2020-11-09 13:39 ` [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value Paolo Bonzini
2020-11-09 19:40 ` Markus Armbruster
2020-11-09 19:47 ` Paolo Bonzini
2020-11-09 13:39 ` [PATCH v2 6/6] qemu-option: warn for short-form boolean options Paolo Bonzini
2020-11-09 21:19 ` Markus Armbruster
2020-11-09 21:42 ` Paolo Bonzini
2020-11-10 8:32 ` Markus Armbruster
2020-11-09 13:54 ` [PATCH v2 for-5.2 0/6] Deprecate or forbid crazy QemuOpts cases no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.