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

[Amos Kong]

On Wed, Apr 24, 2013 at 02:20:20PM -0400, 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.

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 <address@hidden>
> > CC: Osier Yang <address@hidden>
> > CC: Anthony Liguori <address@hidden>
> > Signed-off-by: Amos Kong <address@hidden>
> > ---
> >  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.


-- 
                        Amos.