| Message ID | 1366807646-8473-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/24/2013 06:47 AM, Amos Kong 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. Agreed that no HMP counterpart is needed. However, I don't think we have quite the right interface yet. > > Signed-off-by: Amos Kong <akong@redhat.com> > CC: Osier Yang <jyang@redhat.com> > CC: Anthony Liguori <aliguori@us.ibm.com> > --- > qapi-schema.json | 29 +++++++++++++++++++++++++++++ > qmp-commands.hx | 40 ++++++++++++++++++++++++++++++++++++++++ > util/qemu-config.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 109 insertions(+), 0 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 751d3c2..aeab057 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3505,3 +3505,32 @@ > '*asl_compiler_rev': 'uint32', > '*file': 'str', > '*data': 'str' }} > + > +## > +# @ConfigSchemaInfo: > +# > +# Configration schema information. > +# > +# @option: option name > +# > +# @params: parameters strList of one option Why just a strList? That only tells me option names. But we already know so much more than that - we know the type and the help string. > +# > +# Since 1.5 > +## > +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} } I'd rather see an array of structs, more closely mirroring what include/qemu/option.h gives us: # JSON representation of values of QEMUOptionParType, may grow in future { 'enum': 'ConfigParamType', 'data': [ 'flag', 'number', 'size', 'string' ] } # JSON representation of QEMUOptionParameter, may grow in future # @help is optional if no text was present { 'type': 'ConfigParamInfo', 'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } } # Each command line option, and its list of parameters { 'type': 'ConfigSchemaInfo', 'data': { 'option':'str', 'params': ['ConfigParamInfo'] } } And that means we no longer have ['str'], which bypasses the need for your patch 1/2. > + > +## > +# @query-config-schema > +# > +# Query configuration schema information of options > +# > +# @option: #optional option name > +# > +# Returns: returns @ConfigSchemaInfo if option is assigned, returns > +# @ConfigSchemaInfo list if no option is assigned, returns an error > +# QERR_INVALID_OPTION_GROUP if assigned option doesn't exist. That didn't read well. Also, QERR_INVALID_OPTION_GROUP is a generic error; we don't mention any other QERR_* names in qapi-schema.json. How about: 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'}, > + 'returns': ['ConfigSchemaInfo']} This part looks good. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 4d65422..c6399be 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2414,6 +2414,46 @@ EQMP > .args_type = "", > .mhandler.cmd_new = qmp_marshal_input_query_uuid, > }, > +SQMP > +query-config-schema > +------------ > + > +Show configuration schema. > + > +Return configuration schema of one option if option is assigned, return > +configuration schema list of all options if no option is assigned. return > +an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist. Again, QERR_INVALID_OPTION_GROUP is not a defined error (it is shorthand for a specific message for a GenericError class). > + > +- "option": option name > +- "params": parameters string list of one option > + > +Example: > + > +-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}} > +<- { > + "return": [ > + { > + "params": [ > + "strict", > + "reboot-timeout", > + "splash-time", > + "splash", > + "menu", > + "once", > + "order" > + ], > + "option": "boot-opts" > + } > + ] > + } Back to my desire for more structure, I'd like to see: -> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}} <- { "return": [ { "params": [ { "name": "strict", "type": "string" }, { "name": "reboot-timeout", "type": "string" }, ... ], "option": "boot-opts" } ] } Another more interesting example, showing why the "type" and optional "help" fields are useful, assuming https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04810.html goes in: -> {"execute": "query-config-schema", "arguments" : {"option": "drive"}} <- { "return": [ { "params": [ { "name": "bus", "type": "number", "help": "bus number" }, { "name": "if", "type": "string", "help": "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)" }, ... ], "option": "drive" } ] } > > +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option, > + const char *option, Error **errp) > +{ > + ConfigSchemaInfoList *conf_list = NULL, *entry; > + ConfigSchemaInfo *info; > + strList *str_list = NULL, *str_entry; > + int entries, i, j; > + > + entries = ARRAY_SIZE(vm_config_groups); > + > + for (i = 0; i < entries; i++) { > + if (vm_config_groups[i] != NULL && > + (!has_option || !strcmp(option, vm_config_groups[i]->name))) { > + info = g_malloc0(sizeof(*info)); This part of the iteration looks fine. > + info->option = g_strdup(vm_config_groups[i]->name); > + str_list = NULL; > + > + for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) { > + str_entry = g_malloc0(sizeof(*str_entry)); > + str_entry->value = g_strdup(vm_config_groups[i]->desc[j].name); > + str_entry->next = str_list; > + str_list = str_entry; > + } Here, you'd need to allocate a bit more structure, to match the JSON I proposed above. > + > + info->params = str_list; > + entry = g_malloc0(sizeof(*entry)); > + entry->value = info; > + entry->next = conf_list; > + conf_list = 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); > Looking forward to v2.
Eric Blake <eblake@redhat.com> writes: > On 04/24/2013 06:47 AM, Amos Kong 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. > > Agreed that no HMP counterpart is needed. However, I don't think we > have quite the right interface yet. > >> >> Signed-off-by: Amos Kong <akong@redhat.com> >> CC: Osier Yang <jyang@redhat.com> >> CC: Anthony Liguori <aliguori@us.ibm.com> >> --- >> qapi-schema.json | 29 +++++++++++++++++++++++++++++ >> qmp-commands.hx | 40 ++++++++++++++++++++++++++++++++++++++++ >> util/qemu-config.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 109 insertions(+), 0 deletions(-) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 751d3c2..aeab057 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -3505,3 +3505,32 @@ >> '*asl_compiler_rev': 'uint32', >> '*file': 'str', >> '*data': 'str' }} >> + >> +## >> +# @ConfigSchemaInfo: >> +# >> +# Configration schema information. >> +# >> +# @option: option name >> +# >> +# @params: parameters strList of one option > > Why just a strList? That only tells me option names. But we already > know so much more than that - we know the type and the help string. > >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} } > > I'd rather see an array of structs, more closely mirroring what > include/qemu/option.h gives us: > > # JSON representation of values of QEMUOptionParType, may grow in future > { 'enum': 'ConfigParamType', > 'data': [ 'flag', 'number', 'size', 'string' ] } > > # JSON representation of QEMUOptionParameter, may grow in future > # @help is optional if no text was present > { 'type': 'ConfigParamInfo', > 'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } } > > # Each command line option, and its list of parameters > { 'type': 'ConfigSchemaInfo', > 'data': { 'option':'str', 'params': ['ConfigParamInfo'] } } > > And that means we no longer have ['str'], which bypasses the need for > your patch 1/2. Strong Ack on this schema. Regards, Anthony Liguori
diff --git a/qapi-schema.json b/qapi-schema.json index 751d3c2..aeab057 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3505,3 +3505,32 @@ '*asl_compiler_rev': 'uint32', '*file': 'str', '*data': 'str' }} + +## +# @ConfigSchemaInfo: +# +# Configration schema information. +# +# @option: option name +# +# @params: parameters strList of one option +# +# Since 1.5 +## +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} } + +## +# @query-config-schema +# +# Query configuration schema information of options +# +# @option: #optional option name +# +# Returns: returns @ConfigSchemaInfo if option is assigned, returns +# @ConfigSchemaInfo list if no option is assigned, returns an error +# QERR_INVALID_OPTION_GROUP if assigned option doesn't exist. +# +# Since 1.5 +## +{'command': 'query-config-schema', 'data': {'*option': 'str'}, + 'returns': ['ConfigSchemaInfo']} diff --git a/qmp-commands.hx b/qmp-commands.hx index 4d65422..c6399be 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2414,6 +2414,46 @@ EQMP .args_type = "", .mhandler.cmd_new = qmp_marshal_input_query_uuid, }, +SQMP +query-config-schema +------------ + +Show configuration schema. + +Return configuration schema of one option if option is assigned, return +configuration schema list of all options if no option is assigned. return +an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist. + +- "option": option name +- "params": parameters string list of one option + +Example: + +-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}} +<- { + "return": [ + { + "params": [ + "strict", + "reboot-timeout", + "splash-time", + "splash", + "menu", + "once", + "order" + ], + "option": "boot-opts" + } + ] + } + +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..e8b4466 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,45 @@ 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, *entry; + ConfigSchemaInfo *info; + strList *str_list = NULL, *str_entry; + int entries, i, j; + + entries = ARRAY_SIZE(vm_config_groups); + + for (i = 0; i < entries; i++) { + if (vm_config_groups[i] != NULL && + (!has_option || !strcmp(option, vm_config_groups[i]->name))) { + info = g_malloc0(sizeof(*info)); + info->option = g_strdup(vm_config_groups[i]->name); + str_list = NULL; + + for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) { + str_entry = g_malloc0(sizeof(*str_entry)); + str_entry->value = g_strdup(vm_config_groups[i]->desc[j].name); + str_entry->next = str_list; + str_list = str_entry; + } + + info->params = str_list; + entry = g_malloc0(sizeof(*entry)); + entry->value = info; + entry->next = conf_list; + conf_list = 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);
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. Signed-off-by: Amos Kong <akong@redhat.com> CC: Osier Yang <jyang@redhat.com> CC: Anthony Liguori <aliguori@us.ibm.com> --- qapi-schema.json | 29 +++++++++++++++++++++++++++++ qmp-commands.hx | 40 ++++++++++++++++++++++++++++++++++++++++ util/qemu-config.c | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 0 deletions(-)