Re: [PATCH v2 04/11] qemu-option: Fix 'help' parameter parsing

[Fabiano Rosas <farosas@suse.de>]

marcandre.lureau@redhat.com 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 <farosas@suse.de>
>> Message-ID: <20260930221105.2262063-5-farosas@suse.de>
>>
>> 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!