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

[Luiz Capitulino <lcapitulino@redhat.com>]

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);