Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>   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?