Re: [PATCH v2 5/6] qemu-option: move help handling to get_opt_name_value

[Markus Armbruster]

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().