Re: [PATCH 1/2] 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 or qemu_opts_parse_noisily.
> Move the check from opt_validate to the common workhorses
> of qemu_opts_parse and qemu_opts_parse_noisily, opts_do_parse
> and get_opt_name_value.
>
> As a result, opts_parse and opts_do_parse do not return an error anymore
> when help is requested---just like qemu_opts_parse_noisily.
>
> 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".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-option.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index acefbc23fa..61fc96f9dd 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -504,17 +504,13 @@ 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;
>  
>      desc = find_desc_by_name(opt->opts->list->desc, opt->name);
>      if (!desc && !opts_accepts_any(opt->opts)) {
>          error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> -        if (help_wanted && is_help_option(opt->name)) {
> -            *help_wanted = true;
> -        }
>          return false;
>      }

Two callers, one passes null (trivial: no change), one non-null (more
interesting).

Behavior before the patch is rather peculiar:

* The caller needs to opt into the help syntax by passing non-null
  @help_wanted.

* 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.

  - Else, we recognize it only when there is no option named "help".

* A help request is an ordinary option parameter (possibly sugared)
  where the parameter name is a "help option" (i.e. "help" or "?"), and
  the value doesn't matter.

  Examples:
  - "help=..."
  - "help" (sugar for "help=on")
  - "nohelp" (sugar for "help=off")
  - "?=..."
  - "?" (sugar for "?=on")
  - "no?" (sugar for "?=off")

* A request for help is treated as an error: we set @errp and return
  false.

>  
> @@ -531,7 +527,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;

This is the trivial caller.

>      }
> @@ -767,16 +763,18 @@ 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, *pe, *pc;
> -
> -    pe = strchr(params, '=');
> -    pc = strchr(params, ',');
> +    const char *p;
> +    size_t len;
>  
> -    if (!pe || (pc && pc < pe)) {
> +    len = strcspn(params, "=,");
> +    if (params[len] != '=') {
>          /* found "foo,more" */
> -        if (firstname) {
> +        if (help_wanted && starts_with_help_option(params) == len) {
> +            *help_wanted = true;
> +        } else if (firstname) {
>              /* implicitly named first option */
>              *name = g_strdup(firstname);
>              p = get_opt_value(params, value);

This function parses one parameter from @params into @name, @value, and
returns a pointer to the next parameter, or else to the terminating
'\0'.

Funny: it cannot fail.  QemuOpts is an indiscriminate omnivore ;)

The patch does two separate things:

1. It streamlines how we look ahead to '=', ',' or '\0'.

   Three cases: '=' comes first, '-' comes first, '\0' comes first.

   Before the patch: !pe || (pc && pc < pe) means there is no '=', or
   else there is ',' and it's before '='.  In other words, '=' does not
   come first.

   After the patch: params[len] != '=' where len = strcspn(params, "=,")
   means '=' does not come first.

   Okay, but make it a separate patch, please.

   The ,more in both comments is slightly misleading.  Observation, not
   demand.

2. Optional parsing of help (opt in by passing non-null @help_wanted).

   I wonder why this is opt-in.  I trust that'll become clear further
   down.

   If @params starts with "help option", and it's followed by ',' or
   '\0', set *help_wanted instead of *name and *value.  Okay.

   The function needed a written contract before, and now it needs it
   more.  Observation, not demand.

> @@ -814,7 +812,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")) {
> @@ -825,7 +826,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;
>          }

This is the interesting caller.

Before the patch:

* Success: add an option paramter to @opts, return true.

* Help wanted (only if caller opts in): set *help_wanted, set error,
  return false

* Error: set error, return false.

The patch changes two things:

1. We no longer set an error when the user asks for help.  Checking the
   callers:

   - qemu_opts_do_parse() is not affected, because it passes null
     @help_wanted.

   - opts_parse() passes on the change to its callers:

     * qemu_opts_parse() is not affected, because it passes null
       @help_wanted.

     * qemu_opts_parse_noisily() is affected; see below.

2. We only recognize "help" and "?".  We no longer recognize
   "help=...". "?=...", "nohelp", and "no?".  I'm okay with the change,
   but it needs to be explained in the commit message.  You decide
   whether to cover it in release notes.

> @@ -840,7 +841,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;

opts_parse() parses an entire option argument.  It returns the value of
option parameter "id".  Everything else gets thrown away.

Note for later: your patch makes it opt out of help syntax.

> @@ -856,11 +857,10 @@ bool has_help_option(const char *params)
>  {
>      const char *p;
>      char *name, *value;
> -    bool ret;
> +    bool ret = false;
>  
>      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) {

has_help_option() parses an entire option argument.

Same syntax change as in opts_do_parse().

> @@ -946,10 +946,10 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, 
> const char *params,
>      bool help_wanted = false;
>  
>      opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, 
> &err);
> -    if (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);
>          }

Since opts_parse() no longer sets an error when the user asks for help,
this function needs an update.  Okay.


Now let's get back to "I wonder why this is opt-in?"

Only one caller does not opt in: opts_parse_id().  I'd try making the
help syntax unconditional.  get_opt_name_value() gets a bit simpler,
opts_parse_id() may get a bit more complex.  I'd expect that to be a
good trade.


QemuOpts patches tend to look more innocent than they are.  This one's
no exception :)