[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: |
Markus Armbruster |
Subject: |
Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt() |
Date: |
Wed, 15 Apr 2020 16:45:00 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
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.
- [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up, Markus Armbruster, 2020/04/09
- [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, 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/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 <=
[PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite, Markus Armbruster, 2020/04/09
[PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper, Markus Armbruster, 2020/04/09