[email protected] writes:
>> 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?:
>
It does already happen before the series, I'll add a test case for that.
Is it something we want to fix? The situation is that 'help' was present
but the QemuOptsList declares a set of parameters of which 'help' is a
part of. I'm not sure if this is an oversight in that it wants to reject
unlisted parameters, but 'help' should have been special-cased or if the
API is that options that list a set of parameters must also list the
'help'.
> 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
Indeed. Thanks!
