> Reject invalid values such as help= and help=foo.
>
> Reject valid, but not applicable values such has help=on and help=off.
>
> To achieve the above:
>
> Add an 'is_help' member to QemuOpt so that information can be stored
> while creating the option. This allows moving the help handling out of
> get_opt_name_value(), which is necessary because that function doesn't
> take an Error.
>
> At the caller, opts_do_parse(), let the QemuOpt be created with the
> name and value fields as they come out of get_opt_name_value().
>
> With a new routine, validate the new QemuOpt against the 'help'
> parameter rules (to fix the bug), then let the existing opt_validate()
> reject any NULL values. The two validations need to be separate
> because in between there needs to be a setting of opt->str to "on"
> whenever the special short-form parameters require it.
>
> The "on" value is required because of the conversions of QemuOpts
> from/to QDict since QDict cannot take a NULL value. The conversions
> also cause opt_validate() to be reentrant via qemu_opt_set(), so the
> initial validation of 'help' needs to be out of that function,
> otherwise there's no way to distinguish an user-set help=on vs. the
> help=on set for compatibility with QDict (the "in between" part from
> the previous paragraph).
>
> The passing around of help_wanted becomes redundant since opt->is_help
> can be checked throughout. Remove that argument everywhere.
>
> Stop returning false from opts_do_parse() when is_help=true. Only
> return false when there's an error and let the callers decide what to
> do when is_help=true. However, still break the loop when is_help=true
> to keep the behavior of stopping parsing once the 'help' parameter is
> seen.
>
> At qemu_opts_parse_noisily(), due to the change of return value
> mentioned above, it becomes clear when to report the error and when to
> print the help text. Use qemu_opt_has_help_opt() to obtain the
> is_help information from QemuOpt.
>
> Since get_opt_name_value() can now return while value=NULL, the other
> two callsites need to be updated. At opts_parse_id(), assert value !=
> NULL. At has_help_option(), use the NULL value to keep the existing
> behavior of rejecting anything but 'help'.
>
> The unit tests are updated accordingly.
>
> Signed-off-by: Fabiano Rosas <[email protected]>
> Message-ID: <[email protected]>
>
> diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
> index 5dd9a5162db9..c31bd6e3aba1 100644
> --- a/include/qemu/option_int.h
> +++ b/include/qemu/option_int.h
> @@ -32,6 +32,7 @@
> struct QemuOpt {
> char *name;
> char *str;
> + bool is_help;
>
> const QemuOptDesc *desc;
> union {
> diff --git a/tests/unit/test-qemu-opts.c b/tests/unit/test-qemu-opts.c
> index 07ede509204a..7cedfba33278 100644
> --- a/tests/unit/test-qemu-opts.c
> +++ b/tests/unit/test-qemu-opts.c
> @@ -777,16 +777,15 @@ static void test_has_help_option_with_value(void)
> };
> int i;
> QemuOpts *opts;
> + Error *err = NULL;
>
> for (i = 0; i < ARRAY_SIZE(test); i++) {
> /* reject all */
> g_assert_cmpint(has_help_option(test[i].params), ==, false);
>
> - /* accept all */
> - opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
> - &error_abort);
> - g_assert_cmpint(qemu_opt_has_help_opt(opts), ==, true);
> - qemu_opts_del(opts);
> + opts = qemu_opts_parse(&opts_list_03, test[i].params, false, &err);
> + error_free_or_abort(&err);
> + g_assert(!opts);
> }
> }
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 8ac848bbc06f..483e6cbf70a1 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -331,7 +331,7 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
> QemuOpt *opt;
>
> QTAILQ_FOREACH_REVERSE(opt, &opts->head, next) {
> - if (is_help_option(opt->name)) {
> + if (opt->is_help) {
> return true;
> }
> }
> @@ -499,6 +499,7 @@ static QemuOpt *opt_create(QemuOpts *opts, const char
> *name, char *value)
> opt->name = g_strdup(name);
> opt->str = value;
> opt->opts = opts;
> + opt->is_help = is_help_option(name);
> QTAILQ_INSERT_TAIL(&opts->head, opt, next);
>
> return opt;
> @@ -760,12 +761,10 @@ 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] != '=') {
> @@ -776,22 +775,6 @@ static const char *get_opt_name_value(const char *params,
> p = get_opt_value(params, value);
> } else {
> p = get_opt_name(params, name, len);
> -
> - /*
> - * Short-form flags (i.e. without any value) are not
> - * supported, except for two cases:
> - */
> -
> - /* the 'help' or '?' flag */
> - is_help = is_help_option(*name);
> - if (is_help) {
> - *value = g_strdup("on");
> - }
> -
> - /* a missing, non-implicit key, i.e. a single comma ',' */
> - if (g_str_equal(*name, "") && *p == ',') {
> - *value = g_strdup("on");
> - }
> }
> } else {
> /* found "foo=bar,more" */
> @@ -802,18 +785,46 @@ static const char *get_opt_name_value(const char
> *params,
> }
>
> assert(!*p || *p == ',');
> - if (help_wanted && is_help) {
> - *help_wanted = true;
> - }
> if (*p == ',') {
> p++;
> }
> return p;
> }
>
> +/*
> + * Short-form parameters (i.e. without any value) are not
> + * supported, except for two cases:
> + *
> + * - the 'help' or '?' flag
> + * - the empty, non-implicit key combined with an empty
> + * non-implicit value, i.e. a single comma ','
> + *
> + * The rest of the code is not prepared to deal with empty values, set
> + * the special cases to "on".
> + */
> +static bool opt_short_form_compat(QemuOpt *opt, Error **errp)
> +{
> + if (opt->str) {
> + /*
> + * Validate the 'help' parameter at this point because the
> + * opt_validate() routine can be reentrant and this very check
> + * would fail once the "on" value is set below.
> + */
> + if (opt->is_help) {
> + error_setg(errp, "Parameter '%s' doesn't take any values",
> + opt->name);
> + return false;
> + }
> + } else {
> + if (opt->is_help || g_str_equal(opt->name, "")) {
> + opt->str = g_strdup("on");
> + }
> + }
> + return true;
> +}
> +
> static bool opts_do_parse(QemuOpts *opts, const char *params,
> - const char *firstname,
> - bool *help_wanted, Error **errp)
> + const char *firstname, Error **errp)
> {
> const char *p;
> QemuOpt *opt;
> @@ -822,10 +833,7 @@ static bool opts_do_parse(QemuOpts *opts, const char
> *params,
> g_autofree char *option = NULL;
> g_autofree char *value = NULL;
>
> - p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
> - if (help_wanted && *help_wanted) {
> - return false;
> - }
> + p = get_opt_name_value(p, firstname, &option, &value);
> firstname = NULL;
>
> if (!strcmp(option, "id")) {
> @@ -833,10 +841,15 @@ static bool opts_do_parse(QemuOpts *opts, const char
> *params,
> }
>
> opt = opt_create(opts, option, g_steal_pointer(&value));
> - if (!opt_validate(opt, errp)) {
> + if (!opt_short_form_compat(opt, errp) ||
> + !opt_validate(opt, errp)) {
opt_validate() will reject "help" as an invalid parameter beofre the is_help
check below. Change it to?:
opt = opt_create(opts, option, g_steal_pointer(&value));
if (!opt_short_form_compat(opt, errp)) {
qemu_opt_del(opt);
return false;
}
if (opt->is_help) {
return true;
}
if (!opt_validate(opt, errp)) {
qemu_opt_del(opt);
return false;
}
> qemu_opt_del(opt);
> return false;
> }
> +
> + if (opt->is_help) {
> + return true;
> + }
> }
>
> return true;
> @@ -850,8 +863,9 @@ static char *opts_parse_id(const char *params)
> g_autofree char *name = NULL;
> g_autofree char *value = NULL;
>
> - p = get_opt_name_value(p, NULL, NULL, &name, &value);
> + p = get_opt_name_value(p, NULL, &name, &value);
> if (!strcmp(name, "id")) {
> + assert(value);
> return g_steal_pointer(&value);
> }
> }
> @@ -862,14 +876,13 @@ static char *opts_parse_id(const char *params)
> bool has_help_option(const char *params)
> {
> const char *p;
> - bool ret = false;
>
> for (p = params; *p;) {
> g_autofree char *name = NULL;
> g_autofree char *value = NULL;
>
> - p = get_opt_name_value(p, NULL, &ret, &name, &value);
> - if (ret) {
> + p = get_opt_name_value(p, NULL, &name, &value);
> + if (is_help_option(name) && !value) {
> return true;
> }
> }
> @@ -886,11 +899,11 @@ bool has_help_option(const char *params)
> bool qemu_opts_do_parse(QemuOpts *opts, const char *params,
> const char *firstname, Error **errp)
> {
> - return opts_do_parse(opts, params, firstname, NULL, errp);
> + return opts_do_parse(opts, params, firstname, errp);
> }
>
> static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
> - bool permit_abbrev, bool *help_wanted, Error
> **errp)
> + bool permit_abbrev, Error **errp)
> {
> const char *firstname;
> char *id = opts_parse_id(params);
> @@ -905,7 +918,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const
> char *params,
> return NULL;
> }
>
> - if (!opts_do_parse(opts, params, firstname, help_wanted, errp)) {
> + if (!opts_do_parse(opts, params, firstname, errp)) {
> qemu_opts_del(opts);
> return NULL;
> }
> @@ -923,7 +936,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const
> char *params,
> QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
> bool permit_abbrev, Error **errp)
> {
> - return opts_parse(list, params, permit_abbrev, NULL, errp);
> + return opts_parse(list, params, permit_abbrev, errp);
> }
>
> /**
> @@ -939,17 +952,16 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list,
> const char *params,
> {
> Error *err = NULL;
> QemuOpts *opts;
> - bool help_wanted = false;
>
> - opts = opts_parse(list, params, permit_abbrev,
> - opts_accepts_any(list) ? NULL : &help_wanted, &err);
> + opts = opts_parse(list, params, permit_abbrev, &err);
> if (!opts) {
> - assert(!!err + !!help_wanted == 1);
> - if (help_wanted) {
> - qemu_opts_print_help(list, true);
> - } else {
> - error_report_err(err);
> - }
> + error_report_err(err);
> + return NULL;
> + }
> +
> + if (qemu_opt_has_help_opt(opts) && !opts_accepts_any(list)) {
> + qemu_opts_print_help(list, true);
qemu_opts_del(opts) missing
--
Marc-André Lureau <[email protected]>
