Kevin Wolf <kw...@redhat.com> writes: > Am 14.04.2020 um 22:13 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 14.04.2020 um 15:36 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben: >> >> >> Eric Blake <ebl...@redhat.com> 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.