Patchwork [04/11] qapi schema: add AcpiTableOptions

login
register
mail settings
Submitter Laszlo Ersek
Date March 20, 2013, 11:23 p.m.
Message ID <1363821803-3380-5-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/229513/
State New
Headers show

Comments

Laszlo Ersek - March 20, 2013, 11:23 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi-schema.json |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)
Eric Blake - March 20, 2013, 11:45 p.m.
On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qapi-schema.json |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++

No counterpart change to qmp-commands.hx showing a valid usage?

>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fdaa9da..aae6767 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3442,3 +3442,61 @@
>  # Since: 1.5
>  ##
>  { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> +
> +##
> +# @AcpiTableOptions
> +#
> +# Specify an ACPI table on the command line to load.
> +#

> +#
> +# @oem_id: #optional OEM identifier (6 bytes)

s/oem_id/oem-id/

In general, new QMP interfaces should use '-', not '_'.

> +#
> +# @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.

> +# @data: #optional colon (:) separated list of pathnames to load and
> +#        concatenate as table data. The resultant binary blob must not have an
> +#        ACPI table header. At least one file is required. This field excludes
> +#        @file.

Again, JSON array, not flat string.
Laszlo Ersek - March 21, 2013, 12:31 a.m.
On 03/21/13 00:45, Eric Blake wrote:
> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@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
Laszlo Ersek - March 21, 2013, 10:41 a.m.
On 03/21/13 01:31, Laszlo Ersek wrote:
> On 03/21/13 00:45, Eric Blake wrote:
>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:

>>> +#
>>> +# @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.

Actually it doesn't, apologies -- I got confused for a minute. Again,
since I aim to match the existing option format, I must keep the same
spelling.

Thanks,
Laszlo
Paolo Bonzini - March 21, 2013, 11:51 a.m.
Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
> On 03/21/13 01:31, Laszlo Ersek wrote:
>> On 03/21/13 00:45, Eric Blake wrote:
>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> 
>>>> +#
>>>> +# @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.
> 
> Actually it doesn't, apologies -- I got confused for a minute. Again,
> since I aim to match the existing option format, I must keep the same
> spelling.

We should make a list of places where we have mixed conventions, accept
both, and mass-convert to dash...

Paolo
Michael S. Tsirkin - March 21, 2013, 12:36 p.m.
On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
> > On 03/21/13 01:31, Laszlo Ersek wrote:
> >> On 03/21/13 00:45, Eric Blake wrote:
> >>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> > 
> >>>> +#
> >>>> +# @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.
> > 
> > Actually it doesn't, apologies -- I got confused for a minute. Again,
> > since I aim to match the existing option format, I must keep the same
> > spelling.
> 
> We should make a list of places where we have mixed conventions, accept
> both, and mass-convert to dash...
> 
> Paolo

Anthony used to nack all mass conversions since they mess up the git
history. Since the movement of headers went in, I gather this position
has been relaxed...
Laszlo Ersek - March 21, 2013, 12:42 p.m.
On 03/21/13 13:36, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>
>>>>>> +#
>>>>>> +# @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.
>>>
>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>> since I aim to match the existing option format, I must keep the same
>>> spelling.
>>
>> We should make a list of places where we have mixed conventions, accept
>> both, and mass-convert to dash...
>>
>> Paolo
> 
> Anthony used to nack all mass conversions since they mess up the git
> history. Since the movement of headers went in, I gather this position
> has been relaxed...
> 

Paolo, what are you suggesting precisely?

Thanks
Laszlo
Paolo Bonzini - March 21, 2013, 12:44 p.m.
Il 21/03/2013 13:42, Laszlo Ersek ha scritto:
> On 03/21/13 13:36, Michael S. Tsirkin wrote:
>> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>>
>>>>>>> +#
>>>>>>> +# @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.
>>>>
>>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>>> since I aim to match the existing option format, I must keep the same
>>>> spelling.
>>>
>>> We should make a list of places where we have mixed conventions, accept
>>> both, and mass-convert to dash...
>>>
>>> Paolo
>>
>> Anthony used to nack all mass conversions since they mess up the git
>> history. Since the movement of headers went in, I gather this position
>> has been relaxed...
>>
> 
> Paolo, what are you suggesting precisely?

Using something like

int strcmp_dash(const char *a, const char *b)
{
    char p, q;
    for (;; a++, b++) {
        p = *a == '_' ? '-' : *a;
        q = *b == '_' ? '-' : *b;
        a++, b++;
    } while (p == q && p != 0);
    return p - q;
}

in QemuOpts, the monitor, etc.  The mass conversion isn't really
necessary, just for cleanliness.

Paolo
Anthony Liguori - April 3, 2013, 7:59 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 21/03/2013 13:42, Laszlo Ersek ha scritto:
>> On 03/21/13 13:36, Michael S. Tsirkin wrote:
>>> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>>>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>>>
>>>>>>>> +#
>>>>>>>> +# @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.
>>>>>
>>>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>>>> since I aim to match the existing option format, I must keep the same
>>>>> spelling.
>>>>
>>>> We should make a list of places where we have mixed conventions, accept
>>>> both, and mass-convert to dash...
>>>>
>>>> Paolo
>>>
>>> Anthony used to nack all mass conversions since they mess up the git
>>> history. Since the movement of headers went in, I gather this position
>>> has been relaxed...
>>>
>> 
>> Paolo, what are you suggesting precisely?
>
> Using something like
>
> int strcmp_dash(const char *a, const char *b)
> {
>     char p, q;
>     for (;; a++, b++) {
>         p = *a == '_' ? '-' : *a;
>         q = *b == '_' ? '-' : *b;
>         a++, b++;
>     } while (p == q && p != 0);
>     return p - q;
> }
>
> in QemuOpts, the monitor, etc.  The mass conversion isn't really
> necessary, just for cleanliness.

I had thought we already did this for qemu-option.c but it doesn't
appear that we do...

Regards,

Anthony Liguori

>
> Paolo
Anthony Liguori - April 3, 2013, 8:01 p.m.
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>> > On 03/21/13 01:31, Laszlo Ersek wrote:
>> >> On 03/21/13 00:45, Eric Blake wrote:
>> >>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> > 
>> >>>> +#
>> >>>> +# @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.
>> > 
>> > Actually it doesn't, apologies -- I got confused for a minute. Again,
>> > since I aim to match the existing option format, I must keep the same
>> > spelling.
>> 
>> We should make a list of places where we have mixed conventions, accept
>> both, and mass-convert to dash...
>> 
>> Paolo
>
> Anthony used to nack all mass conversions since they mess up the git
> history. Since the movement of headers went in, I gather this position
> has been relaxed...

It's all about the value-to-cost ratio.  Running indent to remove
invisible space at the end of lines or sed'ing a way a '_t' just because
people like to quote standards too much offers very little value at a
high cost.

Significantly improving the code layout OTOH adds a lot of value and
justifies the impact.

Regards,

Anthony Liguori

>
> -- 
> MST

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index fdaa9da..aae6767 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3442,3 +3442,61 @@ 
 # Since: 1.5
 ##
 { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
+
+##
+# @AcpiTableOptions
+#
+# Specify an ACPI table on the command line to load.
+#
+# At most one of @file and @data can be specified. The list of files specified
+# by any one of them is loaded and concatenated in order. If both are omitted,
+# @data is implied.
+#
+# Other fields / optargs can be used to override fields of the generic ACPI
+# table header; refer to the ACPI specification 5.0, section 5.2.6 System
+# Description Table Header. If a header field is not overridden, then the
+# corresponding value from the concatenated blob is used (in case of @file), or
+# it is filled in with a hard-coded value (in case of @data).
+#
+# String fields are copied into the matching ACPI member from lowest address
+# upwards, and silently truncated / NUL-padded to length.
+#
+# @sig: #optional table signature / identifier (4 bytes)
+#
+# @rev: #optional table revision number (dependent on signature, 1 byte)
+#
+# @oem_id: #optional OEM identifier (6 bytes)
+#
+# @oem_table_id: #optional OEM table identifier (8 bytes)
+#
+# @oem_rev: #optional OEM-supplied revision number (4 bytes)
+#
+# @asl_compiler_id: #optional identifier of the utility that created the table
+#                   (4 bytes)
+#
+# @asl_compiler_rev: #optional revision number of the utility that created the
+#                    table (4 bytes)
+#
+# @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.
+#
+# @data: #optional colon (:) separated list of pathnames to load and
+#        concatenate as table data. The resultant binary blob must not have an
+#        ACPI table header. At least one file is required. This field excludes
+#        @file.
+#
+# Since 1.5
+##
+{ 'type': 'AcpiTableOptions',
+  'data': {
+    '*sig':               'str',
+    '*rev':               'uint8',
+    '*oem_id':            'str',
+    '*oem_table_id':      'str',
+    '*oem_rev':           'uint32',
+    '*asl_compiler_id':   'str',
+    '*asl_compiler_rev':  'uint32',
+    '*file':              'str',
+    '*data':              'str' }}