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

[Luiz Capitulino <lcapitulino@redhat.com>]

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.