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.
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.
> +#
> +# @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);
