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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
>> 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 },
>
> While we're talking about unintuitive, I feel the result for
> implied=true is confusing, too. Never noticed it before, but are we
> really sure that it is the best possible behaviour that '-chardev help'
> and '-chardev id=foo,help' print two entirely different help texts?
>
> I'm not requesting to change anything about this in this series, but
> just making the point that maybe sometimes the existing behaviour is
> questionable.

The QEMU CLI may sometimes be confusing?!?  I'm shocked, sir, I'm
shocked!

>> >> +        { "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.
>
> Shouldn't we also consider what users would reasonably expect?

Of course we should consider reasonable user expectations.

Grumpy aside: when I do, I commonly run into objections that users
reasonably expect things not to change.

> Getting it parsed as an empty option name (I assume with a default value
> of "on"?) certainly looks like something that would surprise most users
> and, as you can see, even some QEMU developers.

My preferred way to address QemuOpts parsing madness is replacing it
wholesale by keyval.c, but that's some time off, I'm afraid.

This series merely aims for more method to the same old madness.