| Message ID | 1366824804-24532-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 25, 2013 at 01:33:24AM +0800, 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. > > V2: fix jaso schema and comments (Eric) Fix the subject: it's the v2, Please help to review, thanks. --- Amos.
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);
On 04/24/2013 12:20 PM, 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. Also, move your changelog below the --- line... > > 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> >> --- ...here. The changelog is essential during review, but adds nothing to the long-term git history (a year from now, we won't care if it took 1 or 20 revisions to get to the version finally committed). http://wiki.qemu.org/Contribute/SubmitAPatch >> 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 I see why you used this text (it's straight copy-and-paste from my email suggestion); but it's rather geared towards someone that cares about the internals of qemu. On the other hand, qapi-schema is the public interface that should tell you how to interact with qemu without having to open include/qemu/option.h or even know about the C type QEMUOptionParType in the first place. Also, we've never mentioned that enums are subject to future growth elsewhere - it's kind of implicit. I think the following would be better text (and sorry for leading you in the wrong direction; in my mail, I was trying to justify WHY I suggested the type, not HOW it should be documented). # Possible types for an option parameter. >> +# >> +# @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. I'm fine with calling the enum value 'boolean' even where the C code called it 'flag'. As long as we have a documented name that describes the semantics of what the parameter will take, libvirt should be able to cope. One other concern - you document that if a flag parameter is omitted, then it defaults to 1. Is that really true? For that matter, does QEMUOptionParameter even have a way of expression which parameters are mandatory vs. optional, and for those which are optional, what the default value is if omitted? If so, then we probably ought to expand the representation of ConfigParamInfo to provide additional fields exposing that information. > >> +# >> +# @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 Again, my fault for trying to point to the C code as my justification, instead of providing a user-facing documentation. I think a better wording would be: # Details about a single parameter of a command-line option. >> +# >> +# @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' Well, with enough punctuation to read well: @help: #optional human readable text string. Not suitable for parsing. > >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'ConfigParamInfo', >> + 'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } } >> + >> +## >> +# @ConfigSchemaInfo: >> +# >> +# Each command line option, and its list of parameters Close. But really, a single ConfigSchemaInfo is only one command line option, not 'each'. I'd use: Details about a command line option, including its list of parameters. >> +# >> +# @option: option name >> +# >> +# @params: a list of parameters of one option No need to abbreviate in QMP; let's call this 'parameters'. >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'ConfigSchemaInfo', >> + 'data': { 'option':'str', 'params': ['ConfigParamInfo'] } } We aren't very consistent on whether to put space after ':' or not; I don't care enough to fix existing uses, but it would look better if you are at least self-consistent within a patch. >> + >> +## >> +# @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? I like query-command-line-options, myself. And if you change that name, you should reflect the naming convention back to the other types: s/ConfigSchemaInfo/CommandLineOptionInfo/, s/ConfigParamInfo/CommandLineParameterInfo/, s/ConfigParamType/CommandLineParameterType/ > >> +# >> +# 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. There's two possibilities: 1. Discovering which options are available. Libvirt wants to do this (a single call returns as much information as possible, and it is mandatory for an option that was added after libvirt was compiled, but where libvirt has ability to dynamically use the results of the introspection to offer the user a chance to use introspected options) - for this one, omitting 'option' is essential. 2. Querying whether a given option is available, and what it supports. Libvirt is less likely to do this (if it needs to learn about 3 different command line options, a single call to get all options and then read through the array is easier to code than making 3 separate QMP calls to query about each individual option); but it makes sense from a usability standpoint of minimizing processing time (making 3 calls that return targeted information passes less data over the wire than making 1 call that returns all information and which the receiver must then sift through). Most existing query-* commands do NOT support filtering, but always return a full array of information. I am not opposed to filtering (I actually think the idea of filtering is quite slick), even if libvirt won't use it; but if you are going to simplify the command, then get rid of 'option' entirely. Do NOT make 'option' mandatory, as then you have hindered the ability to learn which options exist even when you didn't know their name a priori. > >> + '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 s/infomation/information/ >> +- "name": parameter name >> +- "type": parameter type >> +- "help": parameter help message I'd format this to show the levels of structures: Return an array of command line option schema for all options (or for the given option), returning an error if the given option doesn't exist. Each entry contains: - "option": option name (json-string) - "parameters": an array of json-objects, each describing one parameter of the option: - "name": parameter name (json-string) - "type": parameter type (one of 'flag', 'number', 'size', or 'string') - "help": human readable description of the parameter (json-string, optional)
On 04/24/2013 01:39 PM, Eric Blake wrote: > On 04/24/2013 12:20 PM, 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. >>> >>> +## >>> +# @ConfigParamType: >>> +# >>> +# JSON representation of values of QEMUOptionParType, may grow in future > > I see why you used this text (it's straight copy-and-paste from my email > suggestion); but it's rather geared towards someone that cares about the > internals of qemu. On the other hand, qapi-schema is the public > interface that should tell you how to interact with qemu without having > to open include/qemu/option.h or even know about the C type > QEMUOptionParType in the first place. Also, we've never mentioned that > enums are subject to future growth elsewhere - it's kind of implicit. I > think the following would be better text (and sorry for leading you in > the wrong direction; in my mail, I was trying to justify WHY I suggested > the type, not HOW it should be documented). That said, while the public document need not refer to the C api, you SHOULD patch include/qemu/option.h to document that any additions to the C API should be reflected back into the public QMP type name.
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 > > +# > > +# @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. 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; } I think we don't need default here, until some add new items in enum QemuOptType without update this code.
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.
On 04/24/2013 07:14 PM, Amos Kong wrote: >>> +## >>> +{'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. > > Not sure the use-case of full list for libvirt. Osier? I think I already answered that - having the whole list (single QMP call) is more efficient than calling multiple times per command we are interested in. Also, most other query-* commands do NOT filter; having the 'option' argument introduces filtering. It's up to Luiz whether we leave it as is, or omit '*option' for now and save filtering for later, but I'd rather not have mandatory 'option' as then you have to know in advance which command line options you care about.
On 04/24/2013 07:35 PM, Luiz Capitulino wrote: >> 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. gcc is able to warn about missed options if you omit a default case (and if the switch was on an actual enum type); but does not warn about missed options if you do have a default case. Libvirt has actually been switching to default-free coding styles on any switch statement where we expect an enum will grow in the future, for that reason.
On Wed, Apr 24, 2013 at 07:44:37PM -0600, Eric Blake wrote: > On 04/24/2013 07:35 PM, Luiz Capitulino wrote: > > >> 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. > > gcc is able to warn about missed options if you omit a default case (and > if the switch was on an actual enum type); but does not warn about > missed options if you do have a default case. Libvirt has actually been > switching to default-free coding styles on any switch statement where we > expect an enum will grow in the future, for that reason. util/qemu-config.c: In function ‘qmp_query_command_line_options’: util/qemu-config.c:60:17: error: enumeration value ‘QEMU_OPT_NEW’ not handled in switch [-Werror=switch] cc1: all warnings being treated as errors Good! no need default case here.
On Wed, 24 Apr 2013 19:44:37 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/24/2013 07:35 PM, Luiz Capitulino wrote: > > >> 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. > > gcc is able to warn about missed options if you omit a default case (and > if the switch was on an actual enum type); but does not warn about > missed options if you do have a default case. Libvirt has actually been > switching to default-free coding styles on any switch statement where we > expect an enum will grow in the future, for that reason. Ah, didn't know that. Fine with me then.
On 25/04/13 09:14, Amos Kong 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 > >>> +# >>> +# @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. > > Not sure the use-case of full list for libvirt. Osier? One of the use is to query what options qemu support. Changing the "option" to be mandatory is not nice? any other way for libvirt to query what options qemu support? > > > .... >>> +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; > } > > I think we don't need default here, until some add new items in enum > QemuOptType without update this code. > > gcc will give warnings if the new enum member is added but not handled in the switch. From this p.o.v, not using the default is good.
On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote: > On 04/24/2013 12:20 PM, 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) ... > >> +# > >> +# @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. > > I'm fine with calling the enum value 'boolean' even where the C code > called it 'flag'. As long as we have a documented name that describes > the semantics of what the parameter will take, libvirt should be able to > cope. > > One other concern - you document that if a flag parameter is omitted, > then it defaults to 1. Is that really true? I'm wrong. If it's omitted in cmdline, we will give it a default value. example: enable_mlock = qemu_opt_get_bool(opts, "mlock", true); another example: -boot strict=on bool boot_strict; (false by default) strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING' the logical default parameter is "off". This kind of default info is only added in help descriptions right now, we can add a new item 'default_value' to option.h:QemuOptDesc & qapi-schema.json:CommandLineParameterInfo in future? I guess the default value is useful for libvirt. However, we live in different timezone, so I will post my latest patch to maillist (no change about default value). Agree with other comments, thanks. Amos.
On 25/04/13 11:52, Amos Kong wrote: > On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote: >> On 04/24/2013 12:20 PM, 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) > ... > >>>> +# >>>> +# @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. >> I'm fine with calling the enum value 'boolean' even where the C code >> called it 'flag'. As long as we have a documented name that describes >> the semantics of what the parameter will take, libvirt should be able to >> cope. >> >> One other concern - you document that if a flag parameter is omitted, >> then it defaults to 1. Is that really true? > > I'm wrong. If it's omitted in cmdline, we will give it a default value. > > example: > enable_mlock = qemu_opt_get_bool(opts, "mlock", true); > > another example: > -boot strict=on > > bool boot_strict; (false by default) > > strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING' > the logical default parameter is "off". I didn't look through all the threads, not sure if it's already clarified, but is the "flags" indicates whether the option is enabled or disabled by default? If so, is it possible to have another "name" instead of "flag"? Or please add documentation to tell more about the meaning. > > This kind of default info is only added in help descriptions right > now, we can add a new item 'default_value' to option.h:QemuOptDesc & > qapi-schema.json:CommandLineParameterInfo in future? Sounds good to add it in the struct instead and dump as a JSON parameter. > > I guess the default value is useful for libvirt. Yeah, sure, it's nice if libvirt could known if the device is enabled or disabled by default. Osier
On Thu, Apr 25, 2013 at 12:27:16PM +0800, Osier Yang wrote: > On 25/04/13 11:52, Amos Kong wrote: > >On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote: > >>On 04/24/2013 12:20 PM, 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) > >... > > > >>>>+# > >>>>+# @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. > >>I'm fine with calling the enum value 'boolean' even where the C code > >>called it 'flag'. As long as we have a documented name that describes > >>the semantics of what the parameter will take, libvirt should be able to > >>cope. > >> > >>One other concern - you document that if a flag parameter is omitted, > >>then it defaults to 1. Is that really true? > > > >I'm wrong. If it's omitted in cmdline, we will give it a default value. > > > >example: > > enable_mlock = qemu_opt_get_bool(opts, "mlock", true); > > > >another example: > > -boot strict=on > > > > bool boot_strict; (false by default) > > > > strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING' > > the logical default parameter is "off". > > I didn't look through all the threads, not sure if it's already > clarified, but > is the "flags" indicates whether the option is enabled or disabled > by default? No, it used for used to enable/disable sth, not to save default value > If so, is it possible to have another "name" instead of "flag"? Or please > add documentation to tell more about the meaning.
On Thu, 25 Apr 2013 11:52:58 +0800 Amos Kong <akong@redhat.com> wrote: > On Wed, Apr 24, 2013 at 01:39:08PM -0600, Eric Blake wrote: > > On 04/24/2013 12:20 PM, 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) > > ... > > > >> +# > > >> +# @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. > > > > I'm fine with calling the enum value 'boolean' even where the C code > > called it 'flag'. As long as we have a documented name that describes > > the semantics of what the parameter will take, libvirt should be able to > > cope. > > > > One other concern - you document that if a flag parameter is omitted, > > then it defaults to 1. Is that really true? > > > I'm wrong. If it's omitted in cmdline, we will give it a default value. > > example: > enable_mlock = qemu_opt_get_bool(opts, "mlock", true); > > another example: > -boot strict=on > > bool boot_strict; (false by default) > > strict boot is disabled by default, type of strict parameter is 'QEMU_OPT_STRING' > the logical default parameter is "off". > > This kind of default info is only added in help descriptions right > now, we can add a new item 'default_value' to option.h:QemuOptDesc & > qapi-schema.json:CommandLineParameterInfo in future? Yes. I don't think we'll have enough time to fix this now. > > I guess the default value is useful for libvirt. > > However, we live in different timezone, so I will post my latest patch > to maillist (no change about default value). > > Agree with other comments, thanks. > > > Amos. >
On 04/25/2013 06:36 AM, Luiz Capitulino wrote: >>> One other concern - you document that if a flag parameter is omitted, >>> then it defaults to 1. Is that really true? >> >> >> I'm wrong. If it's omitted in cmdline, we will give it a default value. >> >> This kind of default info is only added in help descriptions right >> now, we can add a new item 'default_value' to option.h:QemuOptDesc & >> qapi-schema.json:CommandLineParameterInfo in future? > > Yes. I don't think we'll have enough time to fix this now. Agreed - we've added additional information in the QMP types returned by query-* in the past, and it's no problem if we don't expose default values via QMP until improving the C code to track default values in 1.6. For 1.5, let's just focus on exposing the information we have readily available.
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) +# +# @number: Simple number +# +# @size: The value is converted to an integer. Suffixes for kilobytes etc +# +# @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 +# +# 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: +# +# 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'}, + '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++) { + 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; + + 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);