| Message ID | 20201109133931.979563-6-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Deprecate or forbid crazy QemuOpts cases | expand |
Paolo Bonzini <pbonzini@redhat.com> writes: > Right now, help options are parsed normally and then checked > specially in opt_validate, but only if coming from > qemu_opts_parse_noisily. has_help_option does the check on its own. > > Move the check from opt_validate to the parsing workhorse of QemuOpts, > get_opt_name_value. This will come in handy in the next patch, which > will raise a warning for "-object memory-backend-ram,share" ("flag" option > with no =on/=off part) but not for "-object memory-backend-ram,help". > > As a result: > > - opts_parse and opts_do_parse do not return an error anymore > when help is requested; qemu_opts_parse_noisily does not have > to work around that anymore. > > - various crazy ways to request help are not recognized anymore: > - "help=..." > - "nohelp" (sugar for "help=off") > - "?=..." > - "no?" (sugar for "?=off") > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > util/qemu-option.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 91f4120ce1..0ddf1f7b45 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -496,8 +496,7 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value, > return opt; > } > > -static bool opt_validate(QemuOpt *opt, bool *help_wanted, > - Error **errp) > +static bool opt_validate(QemuOpt *opt, Error **errp) > { > const QemuOptDesc *desc; > const QemuOptsList *list = opt->opts->list; > @@ -505,9 +504,6 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted, > desc = find_desc_by_name(list->desc, opt->name); > if (!desc && !opts_accepts_any(list)) { > error_setg(errp, QERR_INVALID_PARAMETER, opt->name); > - if (help_wanted && is_help_option(opt->name)) { > - *help_wanted = true; > - } > return false; > } > > @@ -524,7 +520,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value, > { > QemuOpt *opt = opt_create(opts, name, g_strdup(value), false); > > - if (!opt_validate(opt, NULL, errp)) { > + if (!opt_validate(opt, errp)) { > qemu_opt_del(opt); > return false; > } > @@ -760,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) > > static const char *get_opt_name_value(const char *params, > const char *firstname, > + bool *help_wanted, > char **name, char **value) > { > const char *p; > size_t len; > + bool is_help = false; > > len = strcspn(params, "=,"); > if (params[len] != '=') { > @@ -780,6 +778,7 @@ static const char *get_opt_name_value(const char *params, > *value = g_strdup("off"); > } else { > *value = g_strdup("on"); > + is_help = is_help_option(*name); > } > } > } else { > @@ -791,6 +790,9 @@ static const char *get_opt_name_value(const char *params, > } > > assert(!*p || *p == ','); > + if (help_wanted && is_help) { > + *help_wanted = true; > + } Note [1] for later: we only ever set *help_wanted to true. > if (*p == ',') { > p++; > } Note [2] for later: we always set *name and *value, even when we set *help_wanted = true. > @@ -806,7 +808,10 @@ static bool opts_do_parse(QemuOpts *opts, const char *params, > QemuOpt *opt; > > for (p = params; *p;) { > - p = get_opt_name_value(p, firstname, &option, &value); > + p = get_opt_name_value(p, firstname, help_wanted, &option, &value); > + if (help_wanted && *help_wanted) { > + return false; Doesn't this leak @option and @value? Remember, [2] get_opt_name_value() always sets *name and *value. > + } > firstname = NULL; > > if (!strcmp(option, "id")) { > @@ -817,7 +822,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params, > > opt = opt_create(opts, option, value, prepend); > g_free(option); > - if (!opt_validate(opt, help_wanted, errp)) { > + if (!opt_validate(opt, errp)) { > qemu_opt_del(opt); > return false; > } > @@ -832,7 +837,7 @@ static char *opts_parse_id(const char *params) > char *name, *value; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, &name, &value); > + p = get_opt_name_value(p, NULL, NULL, &name, &value); > if (!strcmp(name, "id")) { > g_free(name); > return value; This is one of two callers that passes null to help_wanted. If both callers can pass non-null, we can simplify get_opt_name_value() slightly. This one could just as well pass &dummy. Removes doubt that opts_parse_id() could parse differently than opts_do_parse(). opts_parse() relies on the two parsing the same. > @@ -851,8 +856,7 @@ bool has_help_option(const char *params) > bool ret; > > for (p = params; *p;) { > - p = get_opt_name_value(p, NULL, &name, &value); > - ret = is_help_option(name); > + p = get_opt_name_value(p, NULL, &ret, &name, &value); If @p doesn't contain a help request, &ret remains uninitialized. Remember, [1] get_opt_name_value() only ever sets *help_wanted to true. Suggest to make it set *help_wanted like this instead: if (help_wanted) { *help_wanted = is_help; } > g_free(name); > g_free(value); > if (ret) { > @@ -937,11 +941,13 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, > QemuOpts *opts; > bool help_wanted = false; > > - opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err); > - if (err) { > + opts = opts_parse(list, params, permit_abbrev, false, > + opts_accepts_any(list) ? NULL : &help_wanted, This recognizes help requests only when !opts_accepts_any(). Recall my review of v1 on opt_validate()'s behavior before the patch: * A request for help is recognized only when the option name is not recognized. Two cases: - When @opts accepts anything, we ignore cries for help. You recreate this here. Why here? opt_validate() has two callers: qemu_opt_set(), which passes null and is therefore unaffected, and opts_do_parse(), which is affected. opts_do_parse() is called by qemu_opts_do_parse(), which passes null and is therefore unaffected, and opts_parse(). opts_parse() is called by qemu_opts_parse() and qemu_opts_set_defaults(), which pass null and are therefore unaffected, and qemu_opts_parse_noisily(). Okay. A more verbose commit message could've saved me the digging. - Else, we recognize it only when there is no option named "help". You now recognize it even when there is an option named "help". No change as long as no such option exists. That's the case to the best of my knowledge. But the argument belongs into the commit message. > + &err); > + if (!opts) { > + assert(!!err + !!help_wanted == 1); Either err or help_wanted. This is logical inequality. I'd write it as assert(!err != !help_wanted); or maybe as assert(!err ^ !help_wanted); > if (help_wanted) { > qemu_opts_print_help(list, true); > - error_free(err); > } else { > error_report_err(err); > } I think we could pass &help_wanted unconditionally, then ignore the value of help_wanted if opts_accepts_any().
On 09/11/20 20:40, Markus Armbruster wrote: >> >> + p = get_opt_name_value(p, firstname, help_wanted, &option, &value); >> + if (help_wanted && *help_wanted) { >> + return false; > > Doesn't this leak @option and @value? Remember, [2] > get_opt_name_value() always sets *name and *value. Yes, it does. :( >> if (help_wanted) { >> qemu_opts_print_help(list, true); >> - error_free(err); >> } else { >> error_report_err(err); >> } > I think we could pass &help_wanted unconditionally, then ignore the > value of help_wanted if opts_accepts_any(). > Unfortunately not, because callers rely on "help" being added to the options for !opts_accepts_any. opts_do_parse however does: > + if (help_wanted && *help_wanted) { > + return false; > + } You made the same remark on the previous version, but unfortunately I couldn't make it actually produce simpler code. Paolo
diff --git a/util/qemu-option.c b/util/qemu-option.c index 91f4120ce1..0ddf1f7b45 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -496,8 +496,7 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value, return opt; } -static bool opt_validate(QemuOpt *opt, bool *help_wanted, - Error **errp) +static bool opt_validate(QemuOpt *opt, Error **errp) { const QemuOptDesc *desc; const QemuOptsList *list = opt->opts->list; @@ -505,9 +504,6 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted, desc = find_desc_by_name(list->desc, opt->name); if (!desc && !opts_accepts_any(list)) { error_setg(errp, QERR_INVALID_PARAMETER, opt->name); - if (help_wanted && is_help_option(opt->name)) { - *help_wanted = true; - } return false; } @@ -524,7 +520,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value, { QemuOpt *opt = opt_create(opts, name, g_strdup(value), false); - if (!opt_validate(opt, NULL, errp)) { + if (!opt_validate(opt, errp)) { qemu_opt_del(opt); return false; } @@ -760,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char *separator) static const char *get_opt_name_value(const char *params, const char *firstname, + bool *help_wanted, char **name, char **value) { const char *p; size_t len; + bool is_help = false; len = strcspn(params, "=,"); if (params[len] != '=') { @@ -780,6 +778,7 @@ static const char *get_opt_name_value(const char *params, *value = g_strdup("off"); } else { *value = g_strdup("on"); + is_help = is_help_option(*name); } } } else { @@ -791,6 +790,9 @@ static const char *get_opt_name_value(const char *params, } assert(!*p || *p == ','); + if (help_wanted && is_help) { + *help_wanted = true; + } if (*p == ',') { p++; } @@ -806,7 +808,10 @@ static bool opts_do_parse(QemuOpts *opts, const char *params, QemuOpt *opt; for (p = params; *p;) { - p = get_opt_name_value(p, firstname, &option, &value); + p = get_opt_name_value(p, firstname, help_wanted, &option, &value); + if (help_wanted && *help_wanted) { + return false; + } firstname = NULL; if (!strcmp(option, "id")) { @@ -817,7 +822,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params, opt = opt_create(opts, option, value, prepend); g_free(option); - if (!opt_validate(opt, help_wanted, errp)) { + if (!opt_validate(opt, errp)) { qemu_opt_del(opt); return false; } @@ -832,7 +837,7 @@ static char *opts_parse_id(const char *params) char *name, *value; for (p = params; *p;) { - p = get_opt_name_value(p, NULL, &name, &value); + p = get_opt_name_value(p, NULL, NULL, &name, &value); if (!strcmp(name, "id")) { g_free(name); return value; @@ -851,8 +856,7 @@ bool has_help_option(const char *params) bool ret; for (p = params; *p;) { - p = get_opt_name_value(p, NULL, &name, &value); - ret = is_help_option(name); + p = get_opt_name_value(p, NULL, &ret, &name, &value); g_free(name); g_free(value); if (ret) { @@ -937,11 +941,13 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params, QemuOpts *opts; bool help_wanted = false; - opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err); - if (err) { + opts = opts_parse(list, params, permit_abbrev, false, + opts_accepts_any(list) ? NULL : &help_wanted, + &err); + if (!opts) { + assert(!!err + !!help_wanted == 1); if (help_wanted) { qemu_opts_print_help(list, true); - error_free(err); } else { error_report_err(err); }
Right now, help options are parsed normally and then checked specially in opt_validate, but only if coming from qemu_opts_parse_noisily. has_help_option does the check on its own. Move the check from opt_validate to the parsing workhorse of QemuOpts, get_opt_name_value. This will come in handy in the next patch, which will raise a warning for "-object memory-backend-ram,share" ("flag" option with no =on/=off part) but not for "-object memory-backend-ram,help". As a result: - opts_parse and opts_do_parse do not return an error anymore when help is requested; qemu_opts_parse_noisily does not have to work around that anymore. - various crazy ways to request help are not recognized anymore: - "help=..." - "nohelp" (sugar for "help=off") - "?=..." - "no?" (sugar for "?=off") Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- util/qemu-option.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)