Re: [PATCH 1/2] qemu-option: move help handling to get_opt_name_value
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
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
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