On 03/21/13 00:45, Eric Blake wrote:
> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> qapi-schema.json | 58
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> No counterpart change to qmp-commands.hx showing a valid usage?
This is not a qmp command, but a productive (ie. by-design)
repurposement of the schema. Please see commit
eb7ee2cb qapi: introduce OptsVisitor
and the somewhat heated
- http://thread.gmane.org/gmane.comp.emulators.qemu/193702/focus=194579
- http://thread.gmane.org/gmane.comp.emulators.qemu/193702/focus=194585
>> +#
>> +# @oem_id: #optional OEM identifier (6 bytes)
>
> s/oem_id/oem-id/
>
> In general, new QMP interfaces should use '-', not '_'.
Indeed! I think this warrants a respin.
>> +#
>> +# @file: #optional colon (:) separated list of pathnames to load and
>> +# concatenate as table data. The resultant binary blob is expected to
>> +# have an ACPI table header. At least one file is required. This
>> field
>> +# excludes @data.
>> +#
>
> Ewwww. This should be '*file' : [ 'str' ] (that is, use a JSON array of
> file names, not a single string). If you have to reparse a JSON
> argument to break it into parts, then you are using the wrong interface;
> not to mention that I might (perversely) want to pass in a file name
> that contains a colon as part of its name.
Again (referring back to the links above), the schema here is structured
so that it accepts the same "-acpitable ..." command line options that
used to work before.
The parsing of that option pre-series, in acpi_table_add(), is "XXX
fixme: this function uses obsolete argument parsing interface". Since
I'm reworking that here, I think it's reasonable to fix that up as well.
The choice is between the traditional QemuOpts functions and
OptsVisitor. I'd like to benefit from the range validation built into
the latter, plus I'd like to demonstrate that OptsVisitor is usable.
(BTW OptsVisitor does support repeating option arguments, and it indeed
turns them into JSON lists, but utilizing that here would break the
current '-acpitable ...' format.)
Thanks!
Laszlo
