| Message ID | 20200409153041.17576-6-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: > has_help_option() uses its own parser. It's inconsistent with > qemu_opts_parse(), as demonstrated by test-qemu-opts case > /qemu-opts/has_help_option. Fix by reusing the common parser. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/test-qemu-opts.c | 2 +- > util/qemu-option.c | 39 +++++++++++++++++++-------------------- > 2 files changed, 20 insertions(+), 21 deletions(-) > > diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c > index 27c24bb1a2..58a4ea2408 100644 > --- a/tests/test-qemu-opts.c > +++ b/tests/test-qemu-opts.c > @@ -744,7 +744,7 @@ static void test_has_help_option(void) > { "a,help", true, true, true }, > { "a=0,help,b", true, true, true }, > { "help,b=1", true, true, false }, > - { "a,b,,help", false /* BUG */, true, true }, > + { "a,b,,help", true, true, true }, Okay, this revisits my question from 1/8. I guess the argument is that since 'b,help' is NOT a valid option name (only as an option value), that we are instead parsing three separate options 'b', '', and 'help', and whether or not the empty option is valid, the face that 'help' is valid is what makes this return true? > +++ b/util/qemu-option.c > @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, > *ret = size; > } > > -bool has_help_option(const char *param) > -{ > - const char *p = param; > - bool result = false; > - > - while (*p && !result) { > - char *value; > - > - p = get_opt_value(p, &value); > - if (*p) { > - p++; > - } > - > - result = is_help_option(value); Old code: both 'help' and '?' are accepted. > +bool has_help_option(const char *params) > +{ > + const char *p; > + char *name, *value; > + bool ret; > + > + for (p = params; *p;) { > + p = get_opt_name_value(p, NULL, &name, &value); > + ret = !strcmp(name, "help"); New code: only 'help' is accepted. Is the loss of '?' intentional?
Eric Blake <eblake@redhat.com> writes: > On 4/9/20 10:30 AM, Markus Armbruster wrote: >> has_help_option() uses its own parser. It's inconsistent with >> qemu_opts_parse(), as demonstrated by test-qemu-opts case >> /qemu-opts/has_help_option. Fix by reusing the common parser. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> tests/test-qemu-opts.c | 2 +- >> util/qemu-option.c | 39 +++++++++++++++++++-------------------- >> 2 files changed, 20 insertions(+), 21 deletions(-) >> >> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c >> index 27c24bb1a2..58a4ea2408 100644 >> --- a/tests/test-qemu-opts.c >> +++ b/tests/test-qemu-opts.c >> @@ -744,7 +744,7 @@ static void test_has_help_option(void) >> { "a,help", true, true, true }, >> { "a=0,help,b", true, true, true }, >> { "help,b=1", true, true, false }, >> - { "a,b,,help", false /* BUG */, true, true }, >> + { "a,b,,help", true, true, true }, > > Okay, this revisits my question from 1/8. > > I guess the argument is that since 'b,help' is NOT a valid option name > (only as an option value), that we are instead parsing three separate > options 'b', '', and 'help', and whether or not the empty option is > valid, the face that 'help' is valid is what makes this return true? Parsing is oblivious of which option names are valid. It's actually oblivious of the entire QemuOpts definition. Desugaring may depend on the QemuOpts definition, however. "a,b,,help" gets parsed as four option parameters: "a", which gets desugared to either "a=on" or "firstname=a" "b", which gets desugared to "b=on" "" , which gets desugared to "=on" "help", which gets desugared to "help=on" A user of the parse that supports help should clue on the last one, throw away the parse, and provide help. The first desugaring is one that depends on the QemuOpts definition. >> +++ b/util/qemu-option.c >> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, >> *ret = size; >> } >> -bool has_help_option(const char *param) >> -{ >> - const char *p = param; >> - bool result = false; >> - >> - while (*p && !result) { >> - char *value; >> - >> - p = get_opt_value(p, &value); >> - if (*p) { >> - p++; >> - } >> - >> - result = is_help_option(value); > > Old code: both 'help' and '?' are accepted. > >> +bool has_help_option(const char *params) >> +{ >> + const char *p; >> + char *name, *value; >> + bool ret; >> + >> + for (p = params; *p;) { >> + p = get_opt_name_value(p, NULL, &name, &value); >> + ret = !strcmp(name, "help"); > > New code: only 'help' is accepted. Is the loss of '?' intentional? No. Will fix, thanks!
Am 14.04.2020 um 12:16 hat Markus Armbruster geschrieben: > Eric Blake <eblake@redhat.com> writes: > > > On 4/9/20 10:30 AM, Markus Armbruster wrote: > >> has_help_option() uses its own parser. It's inconsistent with > >> qemu_opts_parse(), as demonstrated by test-qemu-opts case > >> /qemu-opts/has_help_option. Fix by reusing the common parser. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, > >> *ret = size; > >> } > >> -bool has_help_option(const char *param) > >> -{ > >> - const char *p = param; > >> - bool result = false; > >> - > >> - while (*p && !result) { > >> - char *value; > >> - > >> - p = get_opt_value(p, &value); > >> - if (*p) { > >> - p++; > >> - } > >> - > >> - result = is_help_option(value); > > > > Old code: both 'help' and '?' are accepted. > > > >> +bool has_help_option(const char *params) > >> +{ > >> + const char *p; > >> + char *name, *value; > >> + bool ret; > >> + > >> + for (p = params; *p;) { > >> + p = get_opt_name_value(p, NULL, &name, &value); > >> + ret = !strcmp(name, "help"); > > > > New code: only 'help' is accepted. Is the loss of '?' intentional? > > No. Will fix, thanks! Please also add some '?' test cases while you're at it. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 14.04.2020 um 12:16 hat Markus Armbruster geschrieben: >> Eric Blake <eblake@redhat.com> writes: >> >> > On 4/9/20 10:30 AM, Markus Armbruster wrote: >> >> has_help_option() uses its own parser. It's inconsistent with >> >> qemu_opts_parse(), as demonstrated by test-qemu-opts case >> >> /qemu-opts/has_help_option. Fix by reusing the common parser. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> >> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, >> >> *ret = size; >> >> } >> >> -bool has_help_option(const char *param) >> >> -{ >> >> - const char *p = param; >> >> - bool result = false; >> >> - >> >> - while (*p && !result) { >> >> - char *value; >> >> - >> >> - p = get_opt_value(p, &value); >> >> - if (*p) { >> >> - p++; >> >> - } >> >> - >> >> - result = is_help_option(value); >> > >> > Old code: both 'help' and '?' are accepted. >> > >> >> +bool has_help_option(const char *params) >> >> +{ >> >> + const char *p; >> >> + char *name, *value; >> >> + bool ret; >> >> + >> >> + for (p = params; *p;) { >> >> + p = get_opt_name_value(p, NULL, &name, &value); >> >> + ret = !strcmp(name, "help"); >> > >> > New code: only 'help' is accepted. Is the loss of '?' intentional? >> >> No. Will fix, thanks! > > Please also add some '?' test cases while you're at it. Okay.
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 27c24bb1a2..58a4ea2408 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -744,7 +744,7 @@ static void test_has_help_option(void) { "a,help", true, true, true }, { "a=0,help,b", true, true, true }, { "help,b=1", true, true, false }, - { "a,b,,help", false /* BUG */, true, true }, + { "a,b,,help", true, true, true }, }; int i; QemuOpts *opts; diff --git a/util/qemu-option.c b/util/qemu-option.c index 6403e521fc..279f1b3fb3 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value, *ret = size; } -bool has_help_option(const char *param) -{ - const char *p = param; - bool result = false; - - while (*p && !result) { - char *value; - - p = get_opt_value(p, &value); - if (*p) { - p++; - } - - result = is_help_option(value); - g_free(value); - } - - return result; -} - bool is_valid_option_list(const char *p) { char *value = NULL; @@ -890,6 +870,25 @@ static char *opts_parse_id(const char *params) return NULL; } +bool has_help_option(const char *params) +{ + const char *p; + char *name, *value; + bool ret; + + for (p = params; *p;) { + p = get_opt_name_value(p, NULL, &name, &value); + ret = !strcmp(name, "help"); + g_free(name); + g_free(value); + if (ret) { + return true; + } + } + + return false; +} + /** * Store options parsed from @params into @opts. * If @firstname is non-null, the first key=value in @params may omit
has_help_option() uses its own parser. It's inconsistent with qemu_opts_parse(), as demonstrated by test-qemu-opts case /qemu-opts/has_help_option. Fix by reusing the common parser. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/test-qemu-opts.c | 2 +- util/qemu-option.c | 39 +++++++++++++++++++-------------------- 2 files changed, 20 insertions(+), 21 deletions(-)