Re: [PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
>> uses has_help_option() to decide whether to print help.  This parses
>> the input string a second time, in a slightly different way.
>>
>> Easy to avoid: replace @invalidp by @help_wanted.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   util/qemu-option.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>
>> -    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
>> +    opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, 
>> &err);
>>       if (err) {
>> -        if (invalidp && has_help_option(params)) {
>> +        if (help_wanted) {
>
> Yep, that is cleaner.  Should there be testsuite coverage changing as
> a result of this?

Hmm.  I guess the actual question is whether this patch changes
behavior.

@invalidp gets set to true when opt_set() runs into an unknown @name.

Aside: can't happend when opts_accepts_any(); such options can't rely on
qemu_opts_parse_noisily() providing help.

One reason for unknown @name is the user asking for help.  We want to
provide it then.  To find out, we use has_help_option(), which parses
certain corner cases differently.  PATCH 1 demonstrates it can return
false incorrectly for certain corner cases.  This patch fixes
qemu_opts_parse_noisily() to provide help as it should even for these
corner cases.

What about this:

* I put PATCH 5 "qemu-option: Fix has_help_option()'s sloppy parsing"
  before this one, so that this one doesn't change behavior.

* I adjust this one's commit message accordingly: scratch ", in a
  slightly different way".

Do we care enough to further document the bug fix in PATCH 5's commit
message?  Even find an actual CLI option that reproduces the bug?  I
doubt it.  This is all about silly corner cases involving ','.

Perhaps has_help_option() can also return true incorrectly.  I doubt we
care.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!