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 22:13 hat Markus Armbruster geschrieben:
>> 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".
>
> opts_do_parse() can return an error. So maybe what we should do is
> rejecting empty option names there?
>
>> (2) This series tries hard not to write QemuOpts parsing code.  It
>>     throws away QemuOpts parsing code.
>
> Arguably, an additional error is writing QemuOpts parsing code, but
> maybe little enough that it's tolerable?

The patch would be simple enough, but I dread writing the commit
message.

I permitted myself to get sidetracked into this series, I'm desperate to
get it out of the way and return to the work I actually want to do.