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 15:36 hat Markus Armbruster geschrieben:
>> 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:
>> >> >> +        { "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.
>
> Fair point. It's not always easy to tell whether something should be
> considered a bug in the external interface (and consequently be fixed)
> or just an idiosyncrasy that people may have get used to (and therefore
> requires deprecation before improving it).
>
> In this specific case, I'm not aware of empty option names actually
> doing anything useful anywhere, so I think it might be clearer in this
> case that it's indeed a bug.

You're right in that backward compatibility is not a convincing argument
for stuff that has no known productive uses, and is bonkers to boot.

>> > 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.
>
> I understand. Though I think replacing with keyval will be potentially
> less problematic if QemuOpts already behaved more similar.
>
> If I were writing the code, I think I would use existing bugs and
> inconsistencies as an excuse to make QemuOpts behave more like what
> keyval can easily handle by declaring whatever is closest to keyval as
> the correct interpretation.

Fair enough.

However,

(1) is_valid_option_list()'s and opts_do_parse()'s parse of "a,b,,help"
    are equidistant from keyval_parse()'s:

    opts_do_parse() splits it into four parts:

        "a"     (desugared to a=on)
        "b"     (desugared to b=on)
        ""      (desugared to =on)
        "help"  (desugared to help=on)

    has_help_option() splits it into two:

        "a"
        "b,help"

    keyval_parse() fails:

        Expected '=' after parameter 'a'

    If I it implemented boolean sugar, then it would fail at the third
    comma, just like ",help" fails now:

        Invalid parameter ''

    Fails because ",help" does not start with a valid name.

    Thus, the answer to the question which of the two functions covered
    by the test are wrong would be "both".

(2) This series tries hard not to write QemuOpts parsing code.  It
    throws away QemuOpts parsing code.