Patchwork monitor: introduce query-config-schema command

login
register
mail settings
Submitter Amos Kong
Date April 24, 2013, 5:33 p.m.
Message ID <1366824804-24532-1-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/239285/
State New
Headers show

Comments

Amos Kong - April 24, 2013, 5:33 p.m.
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)

Signed-off-by: Amos Kong <akong@redhat.com>
CC: Osier Yang <jyang@redhat.com>
CC: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 qapi-schema.json   | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx    | 43 ++++++++++++++++++++++++++++++++++++
 util/qemu-config.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)
Amos Kong - April 24, 2013, 5:36 p.m.
On Thu, Apr 25, 2013 at 01:33:24AM +0800, Amos Kong 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)

Fix the subject: it's the v2, Please help to review, thanks.

---
              Amos.
Luiz Capitulino - April 24, 2013, 6:20 p.m.
On Thu, 25 Apr 2013 01:33:24 +0800
Amos Kong <akong@redhat.com> 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.

More comments below.

> Signed-off-by: Amos Kong <akong@redhat.com>
> CC: Osier Yang <jyang@redhat.com>
> CC: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  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
> +#
> +# @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.

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

> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigParamInfo',
> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> +
> +##
> +# @ConfigSchemaInfo:
> +#
> +# Each command line option, and its list of parameters
> +#
> +# @option: option name
> +#
> +# @params: a list of parameters of one option
> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo',
> +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> +
> +##
> +# @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?

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

> + '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
> +- "name": parameter name
> +- "type": parameter type
> +- "help": parameter help message
> +
> +Example:
> +
> +-> {"execute": "query-config-schema", "arguments" : {"option": "option-rom"}}
> +<- {
> +    "return": [
> +        {
> +            "params": [
> +                {
> +                    "name": "romfile",
> +                    "type": "flag"
> +                },
> +                {
> +                    "name": "bootindex",
> +                    "type": "size"
> +                }
> +            ],
> +            "option": "option-rom"
> +        }
> +    ]
> +  }
> +
> +EQMP
> +
> +    {
> +        .name       = "query-config-schema",
> +        .args_type  = "option:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_query_config_schema,
> +    },
>  
>  SQMP
>  query-migrate
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 01ca890..6d93642 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -5,6 +5,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
> +#include "qmp-commands.h"
>  
>  static QemuOptsList *vm_config_groups[32];
>  
> @@ -37,6 +38,56 @@ QemuOptsList *qemu_find_opts(const char *group)
>      return ret;
>  }
>  
> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> +                                              const char *option, Error **errp)
> +{
> +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
> +    ConfigSchemaInfo *schema_info;
> +    ConfigParamInfoList *param_list = NULL, *param_entry;
> +    ConfigParamInfo *param_info;
> +    int entries, i, j;
> +
> +    entries = ARRAY_SIZE(vm_config_groups);
> +
> +    for (i = 0; i < entries; i++) {

Can't you loop until vm_config_groups[i] is NULL?

> +        if (vm_config_groups[i] != NULL &&
> +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> +            schema_info = g_malloc0(sizeof(*schema_info));
> +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> +            param_list = NULL;
> +
> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> +                param_info =  g_malloc0(sizeof(*param_info));
> +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
> +                param_info->type = vm_config_groups[i]->desc[j].type;

That's a bug. This would only work if QemuOptType and ConfigParamType elements
ordering matched, but even then it's a bad idea to do that.

Suggest doing the type match manually via a switch().

> +
> +                if (vm_config_groups[i]->desc[j].help) {
> +                    param_info->has_help = true;
> +                    param_info->help = g_strdup(
> +                    vm_config_groups[i]->desc[j].help);
> +                }
> +
> +                param_entry = g_malloc0(sizeof(*param_entry));
> +                param_entry->value = param_info;
> +                param_entry->next = param_list;
> +                param_list = param_entry;
> +            }
> +
> +            schema_info->params = param_list;
> +            conf_entry = g_malloc0(sizeof(*conf_entry));
> +            conf_entry->value = schema_info;
> +            conf_entry->next = conf_list;
> +            conf_list = conf_entry;
> +        }
> +    }
> +
> +    if (conf_list == NULL) {
> +        error_set(errp, QERR_INVALID_OPTION_GROUP, option);
> +    }
> +
> +    return conf_list;
> +}
> +
>  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
>  {
>      return find_list(vm_config_groups, group, errp);
Eric Blake - April 24, 2013, 7:39 p.m.
On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 01:33:24 +0800
> Amos Kong <akong@redhat.com> 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 <akong@redhat.com>
>> CC: Osier Yang <jyang@redhat.com>
>> CC: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---

...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 - April 24, 2013, 11:55 p.m.
On 04/24/2013 01:39 PM, Eric Blake wrote:
> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
>> On Thu, 25 Apr 2013 01:33:24 +0800
>> Amos Kong <akong@redhat.com> 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.
>>>
>>> +##
>>> +# @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).

That said, while the public document need not refer to the C api, you
SHOULD patch include/qemu/option.h to document that any additions to the
C API should be reflected back into the public QMP type name.
Amos Kong - April 25, 2013, 1:14 a.m.
On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
> On Thu, 25 Apr 2013 01:33:24 +0800
> Amos Kong <akong@redhat.com> 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.

I didn't count RFC patch, I will use v4 for next version.
Thanks for the review.
 
> More comments below.
> 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > CC: Osier Yang <jyang@redhat.com>
> > CC: Anthony Liguori <aliguori@us.ibm.com>
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  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
> > +#
> > +# @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.

Ok.

btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
with 'bool' type in qapi-schema.json
 
> > +#
> > +# @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
> > +#
> > +# @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'
> 
> > +#
> > +# Since 1.5
> > +##
> > +{ 'type': 'ConfigParamInfo',
> > +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> > +
> > +##
> > +# @ConfigSchemaInfo:
> > +#
> > +# Each command line option, and its list of parameters
> > +#
> > +# @option: option name
> > +#
> > +# @params: a list of parameters of one option
> > +#
> > +# Since 1.5
> > +##
> > +{ 'type': 'ConfigSchemaInfo',
> > +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> > +
> > +##
> > +# @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?

'query-command-line-options' is clearer.
 
> > +#
> > +# 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.

For the human, if they don't know the detail name of one option, they just
list all the options, then find the useful one.

Not sure the use-case of full list for libvirt.  Osier?
 
  
....
> > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> > +                                              const char *option, Error **errp)
> > +{
> > +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
> > +    ConfigSchemaInfo *schema_info;
> > +    ConfigParamInfoList *param_list = NULL, *param_entry;
> > +    ConfigParamInfo *param_info;
> > +    int entries, i, j;
> > +
> > +    entries = ARRAY_SIZE(vm_config_groups);
> > +
> > +    for (i = 0; i < entries; i++) {
> 
> Can't you loop until vm_config_groups[i] is NULL?

Fixed.
 
> > +        if (vm_config_groups[i] != NULL &&
> > +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> > +            schema_info = g_malloc0(sizeof(*schema_info));
> > +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> > +            param_list = NULL;
> > +
> > +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> > +                param_info =  g_malloc0(sizeof(*param_info));
> > +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);

> > +                param_info->type = vm_config_groups[i]->desc[j].type;
>
> That's a bug. This would only work if QemuOptType and ConfigParamType elements
> ordering matched, but even then it's a bad idea to do that.
> 
> Suggest doing the type match manually via a switch().

Right! the order doesn't match here.

                     switch (vm_config_groups[i]->desc[j].type) {
                     case QEMU_OPT_STRING:
                         param_info->type = CONFIG_PARAM_TYPE_STRING;
                         break;
                     case QEMU_OPT_BOOL:
                         param_info->type = CONFIG_PARAM_TYPE_FLAG;  
                         break;
                     case QEMU_OPT_NUMBER:
                         param_info->type = CONFIG_PARAM_TYPE_NUMBER;               
                         break;                                                              
                     case QEMU_OPT_SIZE:                                                     
                         param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
                         break;
                     }     

I think we don't need default here, until some add new items in enum
QemuOptType without update this code.
Luiz Capitulino - April 25, 2013, 1:35 a.m.
On Thu, 25 Apr 2013 09:14:51 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
> > On Thu, 25 Apr 2013 01:33:24 +0800
> > Amos Kong <akong@redhat.com> 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.
> 
> I didn't count RFC patch, I will use v4 for next version.
> Thanks for the review.
>  
> > More comments below.
> > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > CC: Osier Yang <jyang@redhat.com>
> > > CC: Anthony Liguori <aliguori@us.ibm.com>
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > >  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
> > > +#
> > > +# @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.
> 
> Ok.
> 
> btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
> with 'bool' type in qapi-schema.json

I don't think so because this won't cause a real conflict, as the enum name
will always have the CONFIG_PARAM_TYPE prefix. But I 'suggested' boolean
anyway.

>  
> > > +#
> > > +# @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
> > > +#
> > > +# @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'
> > 
> > > +#
> > > +# Since 1.5
> > > +##
> > > +{ 'type': 'ConfigParamInfo',
> > > +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
> > > +
> > > +##
> > > +# @ConfigSchemaInfo:
> > > +#
> > > +# Each command line option, and its list of parameters
> > > +#
> > > +# @option: option name
> > > +#
> > > +# @params: a list of parameters of one option
> > > +#
> > > +# Since 1.5
> > > +##
> > > +{ 'type': 'ConfigSchemaInfo',
> > > +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
> > > +
> > > +##
> > > +# @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?
> 
> 'query-command-line-options' is clearer.
>  
> > > +#
> > > +# 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.
> 
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.

QMP is not concerned with human users, we can always use tools like qmp-shell
to give this feature to humans.

> Not sure the use-case of full list for libvirt.  Osier?
>  
>   
> ....
> > > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> > > +                                              const char *option, Error **errp)
> > > +{
> > > +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
> > > +    ConfigSchemaInfo *schema_info;
> > > +    ConfigParamInfoList *param_list = NULL, *param_entry;
> > > +    ConfigParamInfo *param_info;
> > > +    int entries, i, j;
> > > +
> > > +    entries = ARRAY_SIZE(vm_config_groups);
> > > +
> > > +    for (i = 0; i < entries; i++) {
> > 
> > Can't you loop until vm_config_groups[i] is NULL?
> 
> Fixed.
>  
> > > +        if (vm_config_groups[i] != NULL &&
> > > +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
> > > +            schema_info = g_malloc0(sizeof(*schema_info));
> > > +            schema_info->option = g_strdup(vm_config_groups[i]->name);
> > > +            param_list = NULL;
> > > +
> > > +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> > > +                param_info =  g_malloc0(sizeof(*param_info));
> > > +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
> 
> > > +                param_info->type = vm_config_groups[i]->desc[j].type;
> >
> > That's a bug. This would only work if QemuOptType and ConfigParamType elements
> > ordering matched, but even then it's a bad idea to do that.
> > 
> > Suggest doing the type match manually via a switch().
> 
> Right! the order doesn't match here.
> 
>                      switch (vm_config_groups[i]->desc[j].type) {
>                      case QEMU_OPT_STRING:
>                          param_info->type = CONFIG_PARAM_TYPE_STRING;
>                          break;
>                      case QEMU_OPT_BOOL:
>                          param_info->type = CONFIG_PARAM_TYPE_FLAG;  
>                          break;
>                      case QEMU_OPT_NUMBER:
>                          param_info->type = CONFIG_PARAM_TYPE_NUMBER;               
>                          break;                                                              
>                      case QEMU_OPT_SIZE:                                                     
>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
>                          break;
>                      }     

Looks good.

> I think we don't need default here, until some add new items in enum
> QemuOptType without update this code.

Maybe we can have:

	default:
	 abort();

So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
type.
Eric Blake - April 25, 2013, 1:43 a.m.
On 04/24/2013 07:14 PM, Amos Kong wrote:
>>> +##
>>> +{'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.
> 
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.
> 
> Not sure the use-case of full list for libvirt.  Osier?

I think I already answered that - having the whole list (single QMP
call) is more efficient than calling multiple times per command we are
interested in.  Also, most other query-* commands do NOT filter; having
the 'option' argument introduces filtering.  It's up to Luiz whether we
leave it as is, or omit '*option' for now and save filtering for later,
but I'd rather not have mandatory 'option' as then you have to know in
advance which command line options you care about.
Eric Blake - April 25, 2013, 1:44 a.m.
On 04/24/2013 07:35 PM, Luiz Capitulino wrote:

>>                      case QEMU_OPT_SIZE:                                                     
>>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
>>                          break;
>>                      }     
> 
> Looks good.
> 
>> I think we don't need default here, until some add new items in enum
>> QemuOptType without update this code.
> 
> Maybe we can have:
> 
> 	default:
> 	 abort();
> 
> So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> type.

gcc is able to warn about missed options if you omit a default case (and
if the switch was on an actual enum type); but does not warn about
missed options if you do have a default case.  Libvirt has actually been
switching to default-free coding styles on any switch statement where we
expect an enum will grow in the future, for that reason.
Amos Kong - April 25, 2013, 2:03 a.m.
On Wed, Apr 24, 2013 at 07:44:37PM -0600, Eric Blake wrote:
> On 04/24/2013 07:35 PM, Luiz Capitulino wrote:
> 
> >>                      case QEMU_OPT_SIZE:                                                     
> >>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
> >>                          break;
> >>                      }     
> > 
> > Looks good.
> > 
> >> I think we don't need default here, until some add new items in enum
> >> QemuOptType without update this code.
> > 
> > Maybe we can have:
> > 
> > 	default:
> > 	 abort();
> > 
> > So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> > type.
> 
> gcc is able to warn about missed options if you omit a default case (and
> if the switch was on an actual enum type); but does not warn about
> missed options if you do have a default case.  Libvirt has actually been
> switching to default-free coding styles on any switch statement where we
> expect an enum will grow in the future, for that reason.

util/qemu-config.c: In function ‘qmp_query_command_line_options’:
util/qemu-config.c:60:17: error: enumeration value ‘QEMU_OPT_NEW’ not
handled in switch [-Werror=switch]
cc1: all warnings being treated as errors

Good! no need default case here.
Luiz Capitulino - April 25, 2013, 2:14 a.m.
On Wed, 24 Apr 2013 19:44:37 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 04/24/2013 07:35 PM, Luiz Capitulino wrote:
> 
> >>                      case QEMU_OPT_SIZE:                                                     
> >>                          param_info->type = CONFIG_PARAM_TYPE_SIZE;                          
> >>                          break;
> >>                      }     
> > 
> > Looks good.
> > 
> >> I think we don't need default here, until some add new items in enum
> >> QemuOptType without update this code.
> > 
> > Maybe we can have:
> > 
> > 	default:
> > 	 abort();
> > 
> > So that we catch new QEmuOpts types not accompanied by a new ConfigParamType
> > type.
> 
> gcc is able to warn about missed options if you omit a default case (and
> if the switch was on an actual enum type); but does not warn about
> missed options if you do have a default case.  Libvirt has actually been
> switching to default-free coding styles on any switch statement where we
> expect an enum will grow in the future, for that reason.

Ah, didn't know that. Fine with me then.
Osier Yang - April 25, 2013, 3:38 a.m.
On 25/04/13 09:14, Amos Kong wrote:
> On Wed, Apr 24, 2013 at 02:20:20PM -0400, Luiz Capitulino wrote:
>> On Thu, 25 Apr 2013 01:33:24 +0800
>> Amos Kong <akong@redhat.com> 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.
> I didn't count RFC patch, I will use v4 for next version.
> Thanks for the review.
>   
>> More comments below.
>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> CC: Osier Yang <jyang@redhat.com>
>>> CC: Anthony Liguori <aliguori@us.ibm.com>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> ---
>>>   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
>>> +#
>>> +# @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.
> Ok.
>
> btw, using 'bool' matches with 'enum QemuOptType', but it might confuse
> with 'bool' type in qapi-schema.json
>   
>>> +#
>>> +# @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
>>> +#
>>> +# @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'
>>
>>> +#
>>> +# Since 1.5
>>> +##
>>> +{ 'type': 'ConfigParamInfo',
>>> +  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
>>> +
>>> +##
>>> +# @ConfigSchemaInfo:
>>> +#
>>> +# Each command line option, and its list of parameters
>>> +#
>>> +# @option: option name
>>> +#
>>> +# @params: a list of parameters of one option
>>> +#
>>> +# Since 1.5
>>> +##
>>> +{ 'type': 'ConfigSchemaInfo',
>>> +  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
>>> +
>>> +##
>>> +# @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?
> 'query-command-line-options' is clearer.
>   
>>> +#
>>> +# 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.
> For the human, if they don't know the detail name of one option, they just
> list all the options, then find the useful one.
>
> Not sure the use-case of full list for libvirt.  Osier?

One of the use is to query what options qemu support. Changing the
"option" to be mandatory is not nice? any other way for libvirt to query
what options qemu support?

>   
>    
> ....
>>> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
>>> +                                              const char *option, Error **errp)
>>> +{
>>> +    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
>>> +    ConfigSchemaInfo *schema_info;
>>> +    ConfigParamInfoList *param_list = NULL, *param_entry;
>>> +    ConfigParamInfo *param_info;
>>> +    int entries, i, j;
>>> +
>>> +    entries = ARRAY_SIZE(vm_config_groups);
>>> +
>>> +    for (i = 0; i < entries; i++) {
>> Can't you loop until vm_config_groups[i] is NULL?
> Fixed.
>   
>>> +        if (vm_config_groups[i] != NULL &&
>>> +            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
>>> +            schema_info = g_malloc0(sizeof(*schema_info));
>>> +            schema_info->option = g_strdup(vm_config_groups[i]->name);
>>> +            param_list = NULL;
>>> +
>>> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
>>> +                param_info =  g_malloc0(sizeof(*param_info));
>>> +                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
>>> +                param_info->type = vm_config_groups[i]->desc[j].type;
>> That's a bug. This would only work if QemuOptType and ConfigParamType elements
>> ordering matched, but even then it's a bad idea to do that.
>>
>> Suggest doing the type match manually via a switch().
> Right! the order doesn't match here.
>
>                       switch (vm_config_groups[i]->desc[j].type) {
>                       case QEMU_OPT_STRING:
>                           param_info->type = CONFIG_PARAM_TYPE_STRING;
>                           break;
>                       case QEMU_OPT_BOOL:
>                           param_info->type = CONFIG_PARAM_TYPE_FLAG;
>                           break;
>                       case QEMU_OPT_NUMBER:
>                           param_info->type = CONFIG_PARAM_TYPE_NUMBER;
>                           break;
>                       case QEMU_OPT_SIZE:
>                           param_info->type = CONFIG_PARAM_TYPE_SIZE;
>                           break;
>                       }
>
> I think we don't need default here, until some add new items in enum
> QemuOptType without update this code.
>
>

gcc will give warnings if the new enum member is added but not handled
in the switch. From this p.o.v, not using the default is good.
Amos Kong - April 25, 2013, 3:52 a.m.
On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> > On Thu, 25 Apr 2013 01:33:24 +0800
> > Amos Kong <akong@redhat.com> 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)

...

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


I'm wrong. If it's omitted in cmdline, we will give it a default value.

example:
   enable_mlock = qemu_opt_get_bool(opts, "mlock", true);

another example:
   -boot strict=on

   bool boot_strict; (false by default)

   strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
   the logical default parameter is "off".

This kind of default info is only added in help descriptions right
now, we can add a new item 'default_value' to option.h:QemuOptDesc &
qapi-schema.json:CommandLineParameterInfo in future?

I guess the default value is useful for libvirt.

However, we live in different timezone, so I will post my latest patch
to maillist (no change about default value).

Agree with other comments, thanks.


                Amos.
Osier Yang - April 25, 2013, 4:27 a.m.
On 25/04/13 11:52, Amos Kong wrote:
> On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
>> On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
>>> On Thu, 25 Apr 2013 01:33:24 +0800
>>> Amos Kong <akong@redhat.com> 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)
> ...
>
>>>> +#
>>>> +# @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?
>
> I'm wrong. If it's omitted in cmdline, we will give it a default value.
>
> example:
>     enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
>
> another example:
>     -boot strict=on
>
>     bool boot_strict; (false by default)
>
>     strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
>     the logical default parameter is "off".

I didn't look through all the threads, not sure if it's already 
clarified, but
is the "flags" indicates whether the option is enabled or disabled by 
default?

If so, is it possible to have another "name" instead of "flag"? Or please
add documentation to tell more about the meaning.

>
> This kind of default info is only added in help descriptions right
> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
> qapi-schema.json:CommandLineParameterInfo in future?

Sounds good to add it in the struct instead and dump as a JSON parameter.

>
> I guess the default value is useful for libvirt.

Yeah, sure, it's nice if libvirt could known if the device is enabled or 
disabled
by default.

Osier
Amos Kong - April 25, 2013, 5:09 a.m.
On Thu, Apr 25, 2013 at 12:27:16PM +0800, Osier Yang wrote:
> On 25/04/13 11:52, Amos Kong wrote:
> >On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> >>On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> >>>On Thu, 25 Apr 2013 01:33:24 +0800
> >>>Amos Kong <akong@redhat.com> 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)
> >...
> >
> >>>>+#
> >>>>+# @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?
> >
> >I'm wrong. If it's omitted in cmdline, we will give it a default value.
> >
> >example:
> >    enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
> >
> >another example:
> >    -boot strict=on
> >
> >    bool boot_strict; (false by default)
> >
> >    strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
> >    the logical default parameter is "off".
> 
> I didn't look through all the threads, not sure if it's already
> clarified, but
> is the "flags" indicates whether the option is enabled or disabled
> by default?

No, it used for used to enable/disable sth, not to save default value
 
> If so, is it possible to have another "name" instead of "flag"? Or please
> add documentation to tell more about the meaning.
Luiz Capitulino - April 25, 2013, 12:36 p.m.
On Thu, 25 Apr 2013 11:52:58 +0800
Amos Kong <akong@redhat.com> wrote:

> On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote:
> > On 04/24/2013 12:20 PM, Luiz Capitulino wrote:
> > > On Thu, 25 Apr 2013 01:33:24 +0800
> > > Amos Kong <akong@redhat.com> 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)
> 
> ...
> 
> > >> +#
> > >> +# @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?
> 
> 
> I'm wrong. If it's omitted in cmdline, we will give it a default value.
> 
> example:
>    enable_mlock = qemu_opt_get_bool(opts, "mlock", true);
> 
> another example:
>    -boot strict=on
> 
>    bool boot_strict; (false by default)
> 
>    strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING'
>    the logical default parameter is "off".
> 
> This kind of default info is only added in help descriptions right
> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
> qapi-schema.json:CommandLineParameterInfo in future?

Yes. I don't think we'll have enough time to fix this now.

> 
> I guess the default value is useful for libvirt.
> 
> However, we live in different timezone, so I will post my latest patch
> to maillist (no change about default value).
> 
> Agree with other comments, thanks.
> 
> 
>                 Amos.
>
Eric Blake - April 25, 2013, 1:30 p.m.
On 04/25/2013 06:36 AM, Luiz Capitulino wrote:

>>> One other concern - you document that if a flag parameter is omitted,
>>> then it defaults to 1.  Is that really true?
>>
>>
>> I'm wrong. If it's omitted in cmdline, we will give it a default value.
>>

>> This kind of default info is only added in help descriptions right
>> now, we can add a new item 'default_value' to option.h:QemuOptDesc &
>> qapi-schema.json:CommandLineParameterInfo in future?
> 
> Yes. I don't think we'll have enough time to fix this now.

Agreed - we've added additional information in the QMP types returned by
query-* in the past, and it's no problem if we don't expose default
values via QMP until improving the C code to track default values in
1.6.  For 1.5, let's just focus on exposing the information we have
readily available.

Patch

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
+#
+# @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)
+#
+# @number: Simple number
+#
+# @size: The value is converted to an integer. Suffixes for kilobytes etc
+#
+# @string: Character string
+#
+# Since 1.5
+##
+{ 'enum': 'ConfigParamType',
+  'data': [ 'flag', 'number', 'size', 'string' ] }
+
+##
+# @ConfigParamInfo:
+#
+# JSON representation of QEMUOptionParameter, may grow in future
+#
+# @name: parameter name
+#
+# @type: parameter type
+#
+# @help is optional if no text was present
+#
+# Since 1.5
+##
+{ 'type': 'ConfigParamInfo',
+  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }
+
+##
+# @ConfigSchemaInfo:
+#
+# Each command line option, and its list of parameters
+#
+# @option: option name
+#
+# @params: a list of parameters of one option
+#
+# Since 1.5
+##
+{ 'type': 'ConfigSchemaInfo',
+  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }
+
+##
+# @query-config-schema:
+#
+# 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'},
+ '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
+- "name": parameter name
+- "type": parameter type
+- "help": parameter help message
+
+Example:
+
+-> {"execute": "query-config-schema", "arguments" : {"option": "option-rom"}}
+<- {
+    "return": [
+        {
+            "params": [
+                {
+                    "name": "romfile",
+                    "type": "flag"
+                },
+                {
+                    "name": "bootindex",
+                    "type": "size"
+                }
+            ],
+            "option": "option-rom"
+        }
+    ]
+  }
+
+EQMP
+
+    {
+        .name       = "query-config-schema",
+        .args_type  = "option:s?",
+        .mhandler.cmd_new = qmp_marshal_input_query_config_schema,
+    },
 
 SQMP
 query-migrate
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 01ca890..6d93642 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -5,6 +5,7 @@ 
 #include "qapi/qmp/qerror.h"
 #include "hw/qdev.h"
 #include "qapi/error.h"
+#include "qmp-commands.h"
 
 static QemuOptsList *vm_config_groups[32];
 
@@ -37,6 +38,56 @@  QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
+ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
+                                              const char *option, Error **errp)
+{
+    ConfigSchemaInfoList *conf_list = NULL, *conf_entry;
+    ConfigSchemaInfo *schema_info;
+    ConfigParamInfoList *param_list = NULL, *param_entry;
+    ConfigParamInfo *param_info;
+    int entries, i, j;
+
+    entries = ARRAY_SIZE(vm_config_groups);
+
+    for (i = 0; i < entries; i++) {
+        if (vm_config_groups[i] != NULL &&
+            (!has_option || !strcmp(option, vm_config_groups[i]->name))) {
+            schema_info = g_malloc0(sizeof(*schema_info));
+            schema_info->option = g_strdup(vm_config_groups[i]->name);
+            param_list = NULL;
+
+            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
+                param_info =  g_malloc0(sizeof(*param_info));
+                param_info->name = g_strdup(vm_config_groups[i]->desc[j].name);
+                param_info->type = vm_config_groups[i]->desc[j].type;
+
+                if (vm_config_groups[i]->desc[j].help) {
+                    param_info->has_help = true;
+                    param_info->help = g_strdup(
+                    vm_config_groups[i]->desc[j].help);
+                }
+
+                param_entry = g_malloc0(sizeof(*param_entry));
+                param_entry->value = param_info;
+                param_entry->next = param_list;
+                param_list = param_entry;
+            }
+
+            schema_info->params = param_list;
+            conf_entry = g_malloc0(sizeof(*conf_entry));
+            conf_entry->value = schema_info;
+            conf_entry->next = conf_list;
+            conf_list = conf_entry;
+        }
+    }
+
+    if (conf_list == NULL) {
+        error_set(errp, QERR_INVALID_OPTION_GROUP, option);
+    }
+
+    return conf_list;
+}
+
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
 {
     return find_list(vm_config_groups, group, errp);