Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value

2020-11-06 Thread Markus Armbruster 
One more thought...

Markus Armbruster  writes:

> Paolo Bonzini  writes:
[...]
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
[...]
>> @@ -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'.

Like opt_validate() before, this sets *help_wanted only to true.
Callers must pass a pointer to false.  Perhaps having it set
*help_wanted always could simplify things overall.  Up to you.

[...]

Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value

2020-11-06 Thread Markus Armbruster 
Paolo Bonzini  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 
> ---
>  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 @@

[PATCH 1/2] qemu-option: move help handling to get_opt_name_value

2020-11-05 Thread Paolo Bonzini 
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 
---
 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;
 }
 
@@ -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;
 }
@@ -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);
@@ -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, , );
+p = get_opt_name_value(p, firstname, help_wanted, , );
+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;
 }
@@ -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, , );
+p = get_opt_name_value(p, NULL, NULL, , );
 if (!strcmp(name, "id")) {
 g_free(name);
 return value;
@@ -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, , );
-ret = is_help_option(name);
+p = get_opt_name_value(p, NULL, , , );
 g_free(name);
 g_free(value);
 if (ret) {
@@ -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, _wanted, );
-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);
 }
-- 
2.26.2