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

[Kevin Wolf]

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.

> > 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.

Kevin