Eric Blake <[email protected]> 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 <[email protected]> >> --- >> 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 <[email protected]> Thanks!
