On Thu, 25 Apr 2013 09:14:51 +0800
Amos Kong <ak...@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 <ak...@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 <ak...@redhat.com>
> > > CC: Osier Yang <jy...@redhat.com>
> > > CC: Anthony Liguori <aligu...@us.ibm.com>
> > > Signed-off-by: Amos Kong <ak...@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.
