[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_o
From: |
Kevin Wolf |
Subject: |
Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt() |
Date: |
Tue, 14 Apr 2020 15:13:39 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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.
> >> + { "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?
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.
Kevin
- Re: [PATCH for-5.1 7/8] qemu-img: Factor out accumulate_options() helper, (continued)
[PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/09
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Eric Blake, 2020/04/09
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/14
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(),
Kevin Wolf <=
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/14
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Kevin Wolf, 2020/04/14
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/14
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Kevin Wolf, 2020/04/15
- Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/15
Re: [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up, no-reply, 2020/04/09