| Message ID | 1363821803-3380-5-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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...
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
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
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
"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
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' }}
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- qapi-schema.json | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 58 insertions(+), 0 deletions(-)