| Message ID | 20200409153041.17576-5-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-option: Fix corner cases and clean up | expand |
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 <armbru@redhat.com> > --- > 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? Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com> >> --- >> 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 <eblake@redhat.com> Thanks!
diff --git a/util/qemu-option.c b/util/qemu-option.c index d2956082bd..6403e521fc 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -539,7 +539,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name) } static void opt_set(QemuOpts *opts, const char *name, char *value, - bool prepend, bool *invalidp, Error **errp) + bool prepend, bool *help_wanted, Error **errp) { QemuOpt *opt; const QemuOptDesc *desc; @@ -549,8 +549,8 @@ static void opt_set(QemuOpts *opts, const char *name, char *value, if (!desc && !opts_accepts_any(opts)) { g_free(value); error_setg(errp, QERR_INVALID_PARAMETER, name); - if (invalidp) { - *invalidp = true; + if (help_wanted && is_help_option(name)) { + *help_wanted = true; } return; } @@ -847,7 +847,7 @@ static const char *get_opt_name_value(const char *params, static void opts_do_parse(QemuOpts *opts, const char *params, const char *firstname, bool prepend, - bool *invalidp, Error **errp) + bool *help_wanted, Error **errp) { Error *local_err = NULL; char *option, *value; @@ -863,7 +863,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params, continue; } - opt_set(opts, option, value, prepend, invalidp, &local_err); + opt_set(opts, option, value, prepend, help_wanted, &local_err); g_free(option); if (local_err) { error_propagate(errp, local_err); @@ -904,7 +904,7 @@ void qemu_opts_do_parse(QemuOpts *opts, const char *params, static QemuOpts *opts_parse(QemuOptsList *list, const char *params, bool permit_abbrev, bool defaults, - bool *invalidp, Error **errp) + bool *help_wanted, Error **errp) { const char *firstname; char *id = opts_parse_id(params); @@ -929,7 +929,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params, return NULL; } - opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err); + opts_do_parse(opts, params, firstname, defaults, help_wanted, &local_err); if (local_err) { error_propagate(errp, local_err); qemu_opts_del(opts); @@ -965,11 +965,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, { Error *err = NULL; QemuOpts *opts; - bool invalidp = false; + bool help_wanted = false; - 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) { qemu_opts_print_help(list, true); error_free(err); } else {
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 <armbru@redhat.com> --- util/qemu-option.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)