| Message ID | 20200409153041.17576-3-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-option: Fix corner cases and clean up | expand |
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 <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
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>
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.
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); } /**
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(-)