| Message ID | 20201105142731.623428-2-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | deprecate short-form boolean options | expand |
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 :)
One more thought... Markus Armbruster <armbru@redhat.com> writes: > Paolo Bonzini <pbonzini@redhat.com> 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. [...]
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, &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; } @@ -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; @@ -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) { @@ -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); }
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(-)