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:
pgpKCobaUuWMV.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list