Re: [Qemu-devel] [PATCH 1/2] qemu-option: add help fallback to print the list of options

[Marc-André Lureau]

Hi

On Thu, Aug 16, 2018 at 9:19 AM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> 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 <address@hidden>
> > ---
> >  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?

-- 
Marc-André Lureau