Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema command

[Eric Blake]

On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 01:33:24 +0800
> Amos Kong <address@hidden> wrote:
> 
>> Libvirt has no way to probe if an option or property is supported,
>> This patch introdues a new qmp command to query configuration schema
>> information. hmp command isn't added because it's not needed.
>>
>> V2: fix jaso schema and comments (Eric)
> 
> I guess this is v3, isn't it? Btw, it's better to start a new thread
> when submitting a new version.

Also, move your changelog below the --- line...

> 
> More comments below.
> 
>> Signed-off-by: Amos Kong <address@hidden>
>> CC: Osier Yang <address@hidden>
>> CC: Anthony Liguori <address@hidden>
>> Signed-off-by: Amos Kong <address@hidden>
>> ---

...here.  The changelog is essential during review, but adds nothing to
the long-term git history (a year from now, we won't care if it took 1
or 20 revisions to get to the version finally committed).

http://wiki.qemu.org/Contribute/SubmitAPatch

>>  qapi-schema.json   | 64 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
>>  util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 158 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 751d3c2..55aee4a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3505,3 +3505,67 @@
>>      '*asl_compiler_rev':  'uint32',
>>      '*file':              'str',
>>      '*data':              'str' }}
>> +
>> +##
>> +# @ConfigParamType:
>> +#
>> +# JSON representation of values of QEMUOptionParType, may grow in future

I see why you used this text (it's straight copy-and-paste from my email
suggestion); but it's rather geared towards someone that cares about the
internals of qemu.  On the other hand, qapi-schema is the public
interface that should tell you how to interact with qemu without having
to open include/qemu/option.h or even know about the C type
QEMUOptionParType in the first place.  Also, we've never mentioned that
enums are subject to future growth elsewhere - it's kind of implicit.  I
think the following would be better text (and sorry for leading  you in
the wrong direction; in my mail, I was trying to justify WHY I suggested
the type, not HOW it should be documented).

# Possible types for an option parameter.

>> +#
>> +# @flag: If no value is given, the flag is set to 1. Otherwise the value 
>> must
>> +#        be "on" (set to 1) or "off" (set to 0)
> 
> Let's call this 'boolean', because it's what it is. Also, I suggest
> 'Accepts "on" or "off"' as the help text.

I'm fine with calling the enum value 'boolean' even where the C code
called it 'flag'.  As long as we have a documented name that describes
the semantics of what the parameter will take, libvirt should be able to
cope.

One other concern - you document that if a flag parameter is omitted,
then it defaults to 1.  Is that really true?  For that matter, does
QEMUOptionParameter even have a way of expression which parameters are
mandatory vs. optional, and for those which are optional, what the
default value is if omitted?  If so, then we probably ought to expand
the representation of ConfigParamInfo to provide additional fields
exposing that information.

> 
>> +#
>> +# @number: Simple number
> 
> Suggest "Accepts a number".
> 
>> +#
>> +# @size: The value is converted to an integer. Suffixes for kilobytes etc
> 
> Suggest "Accepts a number followed by an optional postfix (K)ilo, (M)ega, 
> (G)iga,
> (T)era"
> 
>> +#
>> +# @string: Character string
>> +#
>> +# Since 1.5
>> +##
>> +{ 'enum': 'ConfigParamType',
>> +  'data': [ 'flag', 'number', 'size', 'string' ] }
>> +
>> +##
>> +# @ConfigParamInfo:
>> +#
>> +# JSON representation of QEMUOptionParameter, may grow in future

Again, my fault for trying to point to the C code as my justification,
instead of providing a user-facing documentation.  I think a better
wording would be:

# Details about a single parameter of a command-line option.

>> +#
>> +# @name: parameter name
>> +#
>> +# @type: parameter type
>> +#
>> +# @help is optional if no text was present
> 
> Suggest '@help human readable text string. This text may change is not 
> suitable
> for parsing #optional'

Well, with enough punctuation to read well:

@help: #optional human readable text string. Not suitable for parsing.

> 
>> +#
>> +# Since 1.5
>> +##
>> +{ 'type': 'ConfigParamInfo',
>> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
>> +
>> +##
>> +# @ConfigSchemaInfo:
>> +#
>> +# Each command line option, and its list of parameters

Close.  But really, a single ConfigSchemaInfo is only one command line
option, not 'each'.  I'd use:

Details about a command line option, including its list of parameters.

>> +#
>> +# @option: option name
>> +#
>> +# @params: a list of parameters of one option

No need to abbreviate in QMP; let's call this 'parameters'.

>> +#
>> +# Since 1.5
>> +##
>> +{ 'type': 'ConfigSchemaInfo',
>> +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }

We aren't very consistent on whether to put space after ':' or not; I
don't care enough to fix existing uses, but it would look better if you
are at least self-consistent within a patch.

>> +
>> +##
>> +# @query-config-schema:
> 
> If you allow me the bikeshed, I find query-config-schema too generic,
> what about query-command-line-schema? query-command-line-options?

I like query-command-line-options, myself.  And if you change that name,
you should reflect the naming convention back to the other types:
s/ConfigSchemaInfo/CommandLineOptionInfo/,
s/ConfigParamInfo/CommandLineParameterInfo/,
s/ConfigParamType/CommandLineParameterType/

> 
>> +#
>> +# Query configuration schema information
>> +#
>> +# @option: #optional option name
>> +#
>> +# Returns: list of @ConfigSchemaInfo for all options (or for the given
>> +#          @option).  Returns an error if a given @option doesn't exist.
>> +#
>> +# Since 1.5
>> +##
>> +{'command': 'query-config-schema', 'data': {'*option': 'str'},
> 
> Please, let's not make option optional. It makes the code slightly more
> complex for no good reason.

There's two possibilities:

1. Discovering which options are available.  Libvirt wants to do this (a
single call returns as much information as possible, and it is mandatory
for an option that was added after libvirt was compiled, but where
libvirt has ability to dynamically use the results of the introspection
to offer the user a chance to use introspected options) - for this one,
omitting 'option' is essential.

2. Querying whether a given option is available, and what it supports.
Libvirt is less likely to do this (if it needs to learn about 3
different command line options, a single call to get all options and
then read through the array is easier to code than making 3 separate QMP
calls to query about each individual option); but it makes sense from a
usability standpoint of minimizing processing time (making 3 calls that
return targeted information passes less data over the wire than making 1
call that returns all information and which the receiver must then sift
through).

Most existing query-* commands do NOT support filtering, but always
return a full array of information.  I am not opposed to filtering (I
actually think the idea of filtering is quite slick), even if libvirt
won't use it; but if you are going to simplify the command, then get rid
of 'option' entirely.  Do NOT make 'option' mandatory, as then you have
hindered the ability to learn which options exist even when you didn't
know their name a priori.

> 
>> + 'returns': ['ConfigSchemaInfo']}
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 4d65422..19415e4 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2414,6 +2414,49 @@ EQMP
>>          .args_type  = "",
>>          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
>>      },
>> +SQMP
>> +query-config-schema
>> +------------
>> +
>> +Show configuration schema.
>> +
>> +Return list of configuration schema of all options (or for the given 
>> option),
>> +return an error if given option doesn't exist.
>> +
>> +- "option": option name
>> +- "params": parameters infomation list of one option

s/infomation/information/

>> +- "name": parameter name
>> +- "type": parameter type
>> +- "help": parameter help message

I'd format this to show the levels of structures:

Return an array of command line option schema for all options (or for
the given option), returning an error if the given option doesn't exist.
 Each entry contains:
- "option": option name (json-string)
- "parameters": an array of json-objects, each describing one
                parameter of the option:
    - "name": parameter name (json-string)
    - "type": parameter type (one of 'flag', 'number', 'size',
              or 'string')
    - "help": human readable description of the parameter
              (json-string, optional)

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

signature.asc
Description: OpenPGP digital signature