Re: [PATCH for-5.2 2/4] qemu-option: move help handling to get_opt_name_value

[Paolo Bonzini]

On 04/11/20 13:21, Markus Armbruster wrote:
> 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, not if coming
> > from qemu_opt_set.
> >
> > Instead, 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.
> >
> > This will come in handy in a subsequent 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>
>
> I'm afraid this fails my smoke test:
>
>      $ o=`sed -n '/HAS_ARG,/s/DEF("\([^"]*\)".*/\1/p' qemu-options.hx`
>      $ for i in $o; do echo "= $i"; upstream-qemu -$i help -version; done 2>&1 
> | egrep -v 'QEMU emulator|Copyright'
>
> Many output differences.  False positives due to help printing lists in
> random order.  Arbitrarily picked true positive:
>
>      $ upstream-qemu -msg help
>      msg options:
>        guest-name=<bool (on/off)> - Prepends guest name for error messages but 
> only if -name guest is set otherwise option is ignored
>
>        timestamp=<bool (on/off)>
>      $ echo $?
>      1
>
> regresses to silent failure.

Hmm, indeed. :/  Fortunately the fix is simple:

diff --git a/util/qemu-option.c b/util/qemu-option.c
index fcd1119a5d..5a3c287611 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -947,10 +947,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);
         }


I've queued 1 and 3 since they were reviewed already and are fixes for 
tests.  I'll run these two through the whole CI and repost.

Paolo