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

[Marc-André Lureau]

On Thu, Aug 16, 2018 at 12:01 PM Marc-André Lureau
<address@hidden> wrote:
>
> 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?

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