Eric Blake <[email protected]> writes: > On 4/9/20 10:30 AM, Markus Armbruster wrote: >> The two turn out to be inconsistent for "a,b,,help". Test case >> marked /* BUG */. >> >> Signed-off-by: Markus Armbruster <[email protected]> >> --- >> tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> > >> +static void test_has_help_option(void) >> +{ >> + static const struct { >> + const char *params; >> + /* expected value of has_help_option() */ >> + bool expect_has_help_option; >> + /* expected value of qemu_opt_has_help_opt() with implied=false */ >> + bool expect_opt_has_help_opt; >> + /* expected value of qemu_opt_has_help_opt() with implied=true */ >> + bool expect_opt_has_help_opt_implied; >> + } test[] = { >> + { "help", true, true, false }, >> + { "helpme", false, false, false }, >> + { "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 }, > > So which way are you calling the bug? Without looking at the code but > going off my intuition, I parse this as option 'a' and option > 'b,help'. The latter is not a normal option name because it contains a > ',', but is a valid option value. > > I agree that we have a bug, but I'm not yet sure in which direction > the bug lies (should has_help_option be fixed to report true, in which > case the substring ",help" has precedence over ',,' escaping; or > should qemu_opt_has_help_opt be fixed to report false, due to treating > 'b,help' after ',,' escape removal as an invalid option name). So the > placement of the /* BUG */ comment matters - where you placed it, I'm > presuming that later in the series you change has_help_option to > return true, even though that goes against my intuitive parse.
In addition to the canonical QemuOpts parser opts_do_parse(), we have several more, and of course they all differ from the canonical one for corner cases. I treat the canonical one as correct, and fix the others by eliminating the extra parsers. The others are: * has_help_option() Fixed in PATCH 5 by reusing the guts of opts_do_parse(). * is_valid_option_list() Fixed in PATCH 8 by not parsing. * "id" extraction in opts_parse() Lazy hack. Fixed in PATCH 3 by reusing the guts of opts_do_parse(). Back to your question: the value of has_help_option() differs from the value of qemu_opt_has_help_opt(). The latter uses the canonical parser, the former is one of the other parsers. I therefore judge the latter right and the former wrong. Clear now?
