Re: [Qemu-devel] [libvirt] [PATCH v3 2/2] query-command-line-options: query all the options in qemu-options.hx

[Amos Kong <akong@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

On Wed, Mar 05, 2014 at 11:50:22AM -0700, Eric Blake wrote:
> On 03/05/2014 08:58 AM, Eric Blake wrote:
> > On 03/04/2014 11:40 PM, Amos Kong wrote:
> > 
> >>> but the docs imply that I should now see:
> >>>
> >>> {"parameters": [], "option": "smbios", "argument": true}
> >>
> >> I really got : {"parameters": [], "option": "smbios", "argument": true}
> >>
> >> (I was testing with latest qemu upstream + my patches, attached the
> >> output file)
> > 
> > Hmm, maybe I was testing a stale binary.  Let me try re-running tests on
> > my end - I recently changed my choice of configure arguments to speed up
> > build time by building fewer binaries, so maybe I tested on a binary
> > that my configure arguments no longer rebuild.
> 
> Aha, it WAS my configure options at fault.  Apologies for the mis-test
> on my side.  I can now confirm that pre-patch, I see (rearranged a
> subset of entries, and newlines added for legibility):
> 
> {"parameters": [], "option": "smbios"},
> {"parameters": [{"name": "file", "type": "string"},
>    {"name": "events", "type": "string"}], "option": "trace"},
> 
> and no fips, while post-patch, I see:
> 
> {"parameters": [], "option": "enable-fips", "argument": false},
> {"parameters": [], "option": "smbios", "argument": true},
> {"parameters": [{"name": "file", "type": "string"},
>    {"name": "events", "type": "string"}], "option": "trace"},
> 
> which matches the docs.  However, I did notice that pre-patch, I see:
> 
> {"parameters": [], "option": "acpi"}
> 
> while post-patch, there is no hit for "acpi", but there is a new:
> 
> {"parameters": [], "option": "acpitable", "argument": true}
> 
> What's going on there?  It is a potential regression if we fail to list
> an option post-patch that was listed pre-patch.  Then again, looking at
> the actual -help text, I see my particular qemu binary mentions only
> "-acpitable [sig=str]..." in the help text (pre- and post-patch), as
> well as this test to prove there is no '-acpi':
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -acpi
> qemu-system-x86_64: -acpi: invalid option
> $ ./x86_64-softmmu/qemu-system-x86_64 -acpitable
> qemu-system-x86_64: -acpitable: requires an argument
> 
> so it looks like your patch was silently fixing a bug.  Indeed, reading
> vl.c, I see:
> 
>             case QEMU_OPTION_acpitable:
>                 opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1);
>                 if (!opts) {
>                     exit(1);
>                 }
>                 do_acpitable_option(opts);
> 
> so the option table named "acpi" is indeed for the command line argument
> "acpitable".

Can we update all the name in option tables to match with actual
command-line spelling? (we can use another patch to fix it)
 
> It would be nice to mention bonus bug fixes like that in the commit
> message (that is, you are not only adding support for flags like
> -enable-fips, you are also fixing options to match their actual
> command-line spelling rather than an alternate name associated with the
> option table in use by the command).
> 
> So, at this point, we still need a v4 to avoid the duplicate static
> tables, but I am now set up to give a Tested-by.

Thanks for your confirm, I will post a V4.

-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]