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

[Eric Blake <eblake@redhat.com>]

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]