On Thu, Aug 16, 2018 at 12:01 PM Marc-André Lureau
<marcandre.lur...@gmail.com> wrote:
>
> Hi
>
> On Thu, Aug 16, 2018 at 9:19 AM Markus Armbruster <arm...@redhat.com> wrote:
> >
> > Marc-André Lureau <marcandre.lur...@redhat.com> writes:
> >
> > > QDev options accept '?' or 'help' in the list of parameters, which is
> > > really handy to list the available options.
> > >
> > > Unfortunately, this isn't built in QemuOpts. qemu_opts_parse_noisily()
> > > seems to be the common path for command line options, so place a
> > > fallback to check for '?' and print help listing available options.
> >
> > My immediate reaction was "how come this doesn't break -device help"?
> > It doesn't, because...
> >
> > > This is very handy, for example with qemu "-spice ?".
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > ---
> > > util/qemu-option.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > > index 01886efe90..8839ee6523 100644
> > > --- a/util/qemu-option.c
> > > +++ b/util/qemu-option.c
> > > @@ -889,7 +889,12 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList
> > > *list, const char *params,
> > >
> > > opts = opts_parse(list, params, permit_abbrev, false, &err);
> > > if (err) {
> > > - error_report_err(err);
> > > + if (has_help_option(params)) {
> > > + qemu_opts_print_help(list);
> > > + error_free(err);
> > > + } else {
> > > + error_report_err(err);
> > > + }
> > > }
> > > return opts;
> > > }
> >
> > ... it only kicks in when parsing failed, and it doesn't for option
> > argument "help".
> >
> > However, it *can* break for some option arguments:
> >
> > $ upstream-qemu -device e1000,id=@
> > upstream-qemu: -device e1000,id=@: Parameter 'id' expects an identifier
> > Identifiers consist of letters, digits, '-', '.', '_', starting with a
> > letter.
> > [Exit 1 ]
> > $ upstream-qemu -device e1000,id=@,help
> > [Exit 1 ]
> >
> > In the second test case, the error message gets replaced by (empty)
> > help.
>
> Good catch
>
> >
> > I love patches improving help, but I can't immediately see how to
> > salvage this one.
> >
>
> We can pass a with_help option down to opt_set(), and print help
> there? Alternatively, pass a bool *invalid_parameter, to be set in
> this case, so qemu_opts_parse_noisly() would only print help when it
> is set.
>
> Do you see a problem with any of those 2 options?
This is the second option, that I can amend and send v2 if you are ok with it:
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9b99f0d0be..a54cf74b49 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -526,7 +526,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
}
static void opt_set(QemuOpts *opts, const char *name, char *value,
- bool prepend, Error **errp)
+ bool prepend, bool *invalidp, Error **errp)
{
QemuOpt *opt;
const QemuOptDesc *desc;
@@ -536,6 +536,9 @@ static void opt_set(QemuOpts *opts, const char
*name, char *value,
if (!desc && !opts_accepts_any(opts)) {
g_free(value);
error_setg(errp, QERR_INVALID_PARAMETER, name);
+ if (invalidp) {
+ *invalidp = true;
+ }
return;
}
@@ -559,7 +562,7 @@ static void opt_set(QemuOpts *opts, const char
*name, char *value,
void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
Error **errp)
{
- opt_set(opts, name, g_strdup(value), false, errp);
+ opt_set(opts, name, g_strdup(value), false, NULL, errp);
}
void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
@@ -790,7 +793,8 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
}
static void opts_do_parse(QemuOpts *opts, const char *params,
- const char *firstname, bool prepend, Error **errp)
+ const char *firstname, bool prepend,
+ bool *invalidp, Error **errp)
{
char *option = NULL;
char *value = NULL;
@@ -825,7 +829,7 @@ static void opts_do_parse(QemuOpts *opts, const
char *params,
}
if (strcmp(option, "id") != 0) {
/* store and parse */
- opt_set(opts, option, value, prepend, &local_err);
+ opt_set(opts, option, value, prepend, invalidp, &local_err);
value = NULL;
if (local_err) {
error_propagate(errp, local_err);
@@ -854,11 +858,12 @@ static void opts_do_parse(QemuOpts *opts, const
char *params,
void qemu_opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, Error **errp)
{
- opts_do_parse(opts, params, firstname, false, errp);
+ opts_do_parse(opts, params, firstname, false, NULL, errp);
}
static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
- bool permit_abbrev, bool defaults, Error **errp)
+ bool permit_abbrev, bool defaults,
+ bool *invalidp, Error **errp)
{
const char *firstname;
char *id = NULL;
@@ -890,7 +895,7 @@ static QemuOpts *opts_parse(QemuOptsList *list,
const char *params,
return NULL;
}
- opts_do_parse(opts, params, firstname, defaults, &local_err);
+ opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err);
if (local_err) {
error_propagate(errp, local_err);
qemu_opts_del(opts);
@@ -910,7 +915,7 @@ static QemuOpts *opts_parse(QemuOptsList *list,
const char *params,
QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
bool permit_abbrev, Error **errp)
{
- return opts_parse(list, params, permit_abbrev, false, errp);
+ return opts_parse(list, params, permit_abbrev, false, NULL, errp);
}
/**
@@ -926,10 +931,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList
*list, const char *params,
{
Error *err = NULL;
QemuOpts *opts;
+ bool invalidp = false;
- opts = opts_parse(list, params, permit_abbrev, false, &err);
+ opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
if (err) {
- if (has_help_option(params)) {
+ if (invalidp && has_help_option(params)) {
qemu_opts_print_help(list);
error_free(err);
} else {
@@ -944,7 +950,7 @@ void qemu_opts_set_defaults(QemuOptsList *list,
const char *params,
{
QemuOpts *opts;
- opts = opts_parse(list, params, permit_abbrev, true, NULL);
+ opts = opts_parse(list, params, permit_abbrev, true, NULL, NULL);
assert(opts);
}
--
Marc-André Lureau
