The next commits will put it to use.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
1 file changed, 56 insertions(+), 46 deletions(-)
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..f08f4bc458 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -805,61 +805,71 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
}
}
+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, ',');
+
+ if (!pe || (pc && pc < pe)) {
+ /* found "foo,more" */
+ if (firstname) {
+ /* implicitly named first option */
+ *name = g_strdup(firstname);
+ p = get_opt_value(params, value);
+ } else {
+ /* option without value, must be a flag */
+ p = get_opt_name(params, name, ',');
+ if (strncmp(*name, "no", 2) == 0) {
+ memmove(*name, *name + 2, strlen(*name + 2) + 1);
+ *value = g_strdup("off");
+ } else {
+ *value = g_strdup("on");
+ }
+ }
+ } else {
+ /* found "foo=bar,more" */
+ p = get_opt_name(params, name, '=');
+ assert(*p == '=');
+ p++;
+ p = get_opt_value(p, value);
+ }
+
+ assert(!*p || *p == ',');
+ if (*p == ',') {
+ p++;
+ }
+ return p;
+}
+
static void opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, bool prepend,
bool *invalidp, Error **errp)
{
- char *option = NULL;
- char *value = NULL;
- const char *p,*pe,*pc;
Error *local_err = NULL;
+ char *option, *value;
+ const char *p;
- for (p = params; *p != '\0'; p++) {
- pe = strchr(p, '=');
- pc = strchr(p, ',');
- if (!pe || (pc && pc < pe)) {
- /* found "foo,more" */
- if (p == params && firstname) {
- /* implicitly named first option */
- option = g_strdup(firstname);
- p = get_opt_value(p, &value);
- } else {
- /* option without value, probably a flag */
- p = get_opt_name(p, &option, ',');
- if (strncmp(option, "no", 2) == 0) {
- memmove(option, option+2, strlen(option+2)+1);
- value = g_strdup("off");
- } else {
- value = g_strdup("on");
- }
- }
- } else {
- /* found "foo=bar,more" */
- p = get_opt_name(p, &option, '=');
- assert(*p == '=');
- p++;
- p = get_opt_value(p, &value);
- }
- if (strcmp(option, "id") != 0) {
- /* store and parse */
- opt_set(opts, option, value, prepend, invalidp, &local_err);
- value = NULL;
- if (local_err) {
- error_propagate(errp, local_err);
- goto cleanup;
- }
- }
- if (*p != ',') {
- break;
+ for (p = params; *p;) {
+ p = get_opt_name_value(p, firstname, &option, &value);
+ firstname = NULL;
+
+ if (!strcmp(option, "id")) {
+ g_free(option);
+ g_free(value);
+ continue;
}
+
+ opt_set(opts, option, value, prepend, invalidp, &local_err);
g_free(option);
- g_free(value);
- option = value = NULL;
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
}
-
- cleanup:
- g_free(option);
- g_free(value);
}
/**
--
2.21.1
On 4/9/20 10:30 AM, Markus Armbruster wrote:
> The next commits will put it to use.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
> 1 file changed, 56 insertions(+), 46 deletions(-)
>
> +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, ',');
> +
> + if (!pe || (pc && pc < pe)) {
> + /* found "foo,more" */
> + if (firstname) {
> + /* implicitly named first option */
> + *name = g_strdup(firstname);
> + p = get_opt_value(params, value);
Is this correct even when params is "foo,,more"? But...
> static void opts_do_parse(QemuOpts *opts, const char *params,
> const char *firstname, bool prepend,
> bool *invalidp, Error **errp)
> {
> - char *option = NULL;
> - char *value = NULL;
> - const char *p,*pe,*pc;
> Error *local_err = NULL;
> + char *option, *value;
> + const char *p;
>
> - for (p = params; *p != '\0'; p++) {
> - pe = strchr(p, '=');
> - pc = strchr(p, ',');
> - if (!pe || (pc && pc < pe)) {
> - /* found "foo,more" */
> - if (p == params && firstname) {
> - /* implicitly named first option */
> - option = g_strdup(firstname);
> - p = get_opt_value(p, &value);
...in this patch, it is just code motion, so if it is a bug, it's
pre-existing and worth a separate fix.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> The next commits will put it to use.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>> 1 file changed, 56 insertions(+), 46 deletions(-)
>>
>
>> +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, ',');
>> +
>> + if (!pe || (pc && pc < pe)) {
>> + /* found "foo,more" */
>> + if (firstname) {
>> + /* implicitly named first option */
>> + *name = g_strdup(firstname);
>> + p = get_opt_value(params, value);
>
> Is this correct even when params is "foo,,more"? But...
>
>> static void opts_do_parse(QemuOpts *opts, const char *params,
>> const char *firstname, bool prepend,
>> bool *invalidp, Error **errp)
>> {
>> - char *option = NULL;
>> - char *value = NULL;
>> - const char *p,*pe,*pc;
>> Error *local_err = NULL;
>> + char *option, *value;
>> + const char *p;
>> - for (p = params; *p != '\0'; p++) {
>> - pe = strchr(p, '=');
>> - pc = strchr(p, ',');
>> - if (!pe || (pc && pc < pe)) {
>> - /* found "foo,more" */
>> - if (p == params && firstname) {
>> - /* implicitly named first option */
>> - option = g_strdup(firstname);
>> - p = get_opt_value(p, &value);
>
> ...in this patch, it is just code motion, so if it is a bug, it's
> pre-existing and worth a separate fix.
If @p points to "foo,,more", possibly followed by additional characters,
then we have pc && pc < pe, and enter this conditional. This means that
foo,,more=42
gets parsed as two name=value, either as
name value
-----------------------
@firstname "foo,more"
"" "42"
if @firstname is non-null, or else as
name value
-----------------
"foo,,more" "on"
"" "42"
Has always been that way; see commit e27c88fe9e "QemuOpts: framework for
storing and parsing options.", v0.12.0.
You might expect
name value
-----------------
"foo,,more" "42"
or
name value
-----------------
"foo,more" "42"
instead. Close, but no cigar.
A language without syntax errors is bound to surprise.
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
>
> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> The next commits will put it to use.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >> util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
> >> 1 file changed, 56 insertions(+), 46 deletions(-)
> >>
> >
> >> +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, ',');
> >> +
> >> + if (!pe || (pc && pc < pe)) {
> >> + /* found "foo,more" */
> >> + if (firstname) {
> >> + /* implicitly named first option */
> >> + *name = g_strdup(firstname);
> >> + p = get_opt_value(params, value);
> >
> > Is this correct even when params is "foo,,more"? But...
> >
> >> static void opts_do_parse(QemuOpts *opts, const char *params,
> >> const char *firstname, bool prepend,
> >> bool *invalidp, Error **errp)
> >> {
> >> - char *option = NULL;
> >> - char *value = NULL;
> >> - const char *p,*pe,*pc;
> >> Error *local_err = NULL;
> >> + char *option, *value;
> >> + const char *p;
> >> - for (p = params; *p != '\0'; p++) {
> >> - pe = strchr(p, '=');
> >> - pc = strchr(p, ',');
> >> - if (!pe || (pc && pc < pe)) {
> >> - /* found "foo,more" */
> >> - if (p == params && firstname) {
> >> - /* implicitly named first option */
> >> - option = g_strdup(firstname);
> >> - p = get_opt_value(p, &value);
> >
> > ...in this patch, it is just code motion, so if it is a bug, it's
> > pre-existing and worth a separate fix.
>
> If @p points to "foo,,more", possibly followed by additional characters,
> then we have pc && pc < pe, and enter this conditional. This means that
>
> foo,,more=42
>
> gets parsed as two name=value, either as
>
> name value
> -----------------------
> @firstname "foo,more"
> "" "42"
>
> if @firstname is non-null
Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
a single option @firstname with a value of "foo,more=42"?
name value
-----------------------
@firstname "foo,more=42"
> or else as
>
> name value
> -----------------
> "foo,,more" "on"
> "" "42"
Here get_opt_name() stops at the first comma. You get a positive flag
"foo" and the remaing option string starts with a comma, so the
condition will still be true for the next loop iteration. Now you get a
positive flag with an empty name "". Finally, the rest is parsed as an
option, so we get:
name value
-----------------------
"foo" "on"
"" "on"
"more" "42"
Actually, at this point I thought I could just check, and gdb confirms
my reading for both cases.
This is still crazy, but perhaps less so than the interpretations you
suggested.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>>
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> The next commits will put it to use.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >> util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>> >> 1 file changed, 56 insertions(+), 46 deletions(-)
>> >>
>> >
>> >> +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, ',');
>> >> +
>> >> + if (!pe || (pc && pc < pe)) {
>> >> + /* found "foo,more" */
>> >> + if (firstname) {
>> >> + /* implicitly named first option */
>> >> + *name = g_strdup(firstname);
>> >> + p = get_opt_value(params, value);
>> >
>> > Is this correct even when params is "foo,,more"? But...
>> >
>> >> static void opts_do_parse(QemuOpts *opts, const char *params,
>> >> const char *firstname, bool prepend,
>> >> bool *invalidp, Error **errp)
>> >> {
>> >> - char *option = NULL;
>> >> - char *value = NULL;
>> >> - const char *p,*pe,*pc;
>> >> Error *local_err = NULL;
>> >> + char *option, *value;
>> >> + const char *p;
>> >> - for (p = params; *p != '\0'; p++) {
>> >> - pe = strchr(p, '=');
>> >> - pc = strchr(p, ',');
>> >> - if (!pe || (pc && pc < pe)) {
>> >> - /* found "foo,more" */
>> >> - if (p == params && firstname) {
>> >> - /* implicitly named first option */
>> >> - option = g_strdup(firstname);
>> >> - p = get_opt_value(p, &value);
>> >
>> > ...in this patch, it is just code motion, so if it is a bug, it's
>> > pre-existing and worth a separate fix.
>>
>> If @p points to "foo,,more", possibly followed by additional characters,
>> then we have pc && pc < pe, and enter this conditional. This means that
>>
>> foo,,more=42
>>
>> gets parsed as two name=value, either as
>>
>> name value
>> -----------------------
>> @firstname "foo,more"
>> "" "42"
>>
>> if @firstname is non-null
>
> Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
> a single option @firstname with a value of "foo,more=42"?
>
> name value
> -----------------------
> @firstname "foo,more=42"
>
>> or else as
>>
>> name value
>> -----------------
>> "foo,,more" "on"
>> "" "42"
>
> Here get_opt_name() stops at the first comma. You get a positive flag
> "foo" and the remaing option string starts with a comma, so the
> condition will still be true for the next loop iteration. Now you get a
> positive flag with an empty name "". Finally, the rest is parsed as an
> option, so we get:
>
> name value
> -----------------------
> "foo" "on"
> "" "on"
> "more" "42"
>
> Actually, at this point I thought I could just check, and gdb confirms
> my reading for both cases.
You're right. I should know better than trying to predict what the
QemuOpts parser does.
> This is still crazy, but perhaps less so than the interpretations you
> suggested.
Permitting anti-social characters in the NAME part of NAME=VALUE is
crazy. To findout which kind of crazy exactly, run the code and
observe. Do not blindly trust the maintainer's explanations, because
he's quite possibly just as confused as everybody else.
Am 09.04.2020 um 17:30 hat Markus Armbruster geschrieben: > The next commits will put it to use. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
© 2016 - 2026 Red Hat, Inc.