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

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".

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.