From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37781) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVOPP-0007T9-Gl for qemu-devel@nongnu.org; Thu, 25 Apr 2013 11:48:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVOPN-0006P2-Oh for qemu-devel@nongnu.org; Thu, 25 Apr 2013 11:48:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44890) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVOPN-0006Op-Gw for qemu-devel@nongnu.org; Thu, 25 Apr 2013 11:48:29 -0400 Message-ID: <51795040.20402@redhat.com> Date: Thu, 25 Apr 2013 23:48:16 +0800 From: Osier Yang MIME-Version: 1.0 References: <1366883435-4993-1-git-send-email-akong@redhat.com> <20130425092050.41428940@redhat.com> In-Reply-To: <20130425092050.41428940@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5] monitor: introduce query-command-line-options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@us.ibm.com, Amos Kong , qemu-devel@nongnu.org On 25/04/13 21:20, Luiz Capitulino wrote: > On Thu, 25 Apr 2013 17:50:35 +0800 > Amos Kong wrote: > >> Libvirt has no way to probe if an option or property is supported, >> This patch introduces a new qmp command to query command line >> option information. hmp command isn't added because it's not needed. >> >> Signed-off-by: Amos Kong >> CC: Luiz Capitulino >> CC: Osier Yang >> CC: Anthony Liguori > This version looks good to me, but it has two small problems. > > First, some options like -drive, -net, -netdev and -device have an empty > QemuOptsList, meaning that option validation is not done upfront. Not sure if there is work of libvirt which is blocked by -net, -netdev, and -device, but I have one which needs to probe the properties of -drive. https://bugzilla.redhat.com/show_bug.cgi?id=821045 > > The other problem is that we're returning some options which seem to be > created at run-time and I honestly have never heard of them, like tmpdev > and acpi. > > I think there are two possible fixes for both issues: > > 1. Do nothing, accept patch as is and fix things in 1.6 > > 2. Don't return an option when 'desc' is NULL > > I'm willing to do 1, as those options don't return any options anyway and > I don't believe any client will mess with them (worst case the client will pass > an option qemu doesn't accept and will refuse to run). 1 sounds better, it doesn't hurt for libvirt to recongnize the empty result, and it may even better because at least we known the option is supported, even the result is empty. For the "-drive" properties probing, not sure if we can wait till 1.6. > > I have one more comment below, but case we accept to do 1 I can fix it myself. > >> --- >> V3: fix json schema and comments (Eric) >> V4: fix descriptions, rename command, check enum type, cleanup >> (Luiz, Eric) >> V5: fix typo, cleanup (Osier) >> --- >> qapi-schema.json | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qmp-commands.hx | 47 ++++++++++++++++++++++++++++++++++++++ >> util/qemu-config.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 180 insertions(+) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 751d3c2..10affbd 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -3505,3 +3505,69 @@ >> '*asl_compiler_rev': 'uint32', >> '*file': 'str', >> '*data': 'str' }} >> + >> +## >> +# @CommandLineParameterType: >> +# >> +# Possible types for an option parameter. >> +# >> +# @string: accepts a character string >> +# >> +# @boolean: accepts "on" or "off" >> +# >> +# @number: accepts a number >> +# >> +# @size: accepts a number followed by an optional postfix (K)ilo, >> +# (M)ega, (G)iga, (T)era >> +# >> +# Since 1.5 >> +## >> +{ 'enum': 'CommandLineParameterType', >> + 'data': ['string', 'boolean', 'number', 'size'] } >> + >> +## >> +# @CommandLineParameterInfo: >> +# >> +# Details about a single parameter of a command line option. >> +# >> +# @name: parameter name >> +# >> +# @type: parameter @CommandLineParameterType >> +# >> +# @help: #optional human readable text string, not suitable for parsing. >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'CommandLineParameterInfo', >> + 'data': { 'name': 'str', >> + 'type': 'CommandLineParameterType', >> + '*help': 'str' } } >> + >> +## >> +# @CommandLineOptionInfo: >> +# >> +# Details about a command line option, including its list of parameters details >> +# >> +# @option: option name >> +# >> +# @parameters: an array of @CommandLineParameterInfo >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'CommandLineOptionInfo', >> + 'data': { 'option': 'str', 'parameters': ['CommandLineParameterInfo'] } } >> + >> +## >> +# @query-command-line-options: >> +# >> +# Query command line option schema. >> +# >> +# @option: #optional option name >> +# >> +# Returns: list of @CommandLineOptionInfo for all options (or for the given >> +# @option). Returns an error if the given @option doesn't exist. >> +# >> +# Since 1.5 >> +## >> +{'command': 'query-command-line-options', 'data': { '*option': 'str' }, > I'm not a huge fan of option being optional, I prefer dropping it and making > the command simpler. But I won't refuse the patch because of that. > >> + 'returns': ['CommandLineOptionInfo'] } >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 4d65422..31bb360 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -2416,6 +2416,53 @@ EQMP >> }, >> >> SQMP >> +query-command-line-options >> +-------------------------- >> + >> +Show command line option schema. >> + >> +Return a json-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 array entry contains the following: >> + >> +- "option": option name (json-string) >> +- "parameters": a json-array describes all parameters of the option: >> + - "name": parameter name (json-string) >> + - "type": parameter type (one of 'string', boolean', 'number', >> + or 'size') >> + - "help": human readable description of the parameter >> + (json-string, optional) >> + >> +Example: >> + >> +-> { "execute": "query-command-line-options", "arguments": { "option": "option-rom" } } >> +<- { "return": [ >> + { >> + "parameters": [ >> + { >> + "name": "romfile", >> + "type": "string" >> + }, >> + { >> + "name": "bootindex", >> + "type": "number" >> + } >> + ], >> + "option": "option-rom" >> + } >> + ] >> + } >> + >> +EQMP >> + >> + { >> + .name = "query-command-line-options", >> + .args_type = "option:s?", >> + .mhandler.cmd_new = qmp_marshal_input_query_command_line_options, >> + }, >> + >> +SQMP >> query-migrate >> ------------- >> >> diff --git a/util/qemu-config.c b/util/qemu-config.c >> index 01ca890..7d3874f 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,72 @@ QemuOptsList *qemu_find_opts(const char *group) >> return ret; >> } >> >> +static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc) >> +{ >> + CommandLineParameterInfoList *param_list = NULL, *entry; >> + CommandLineParameterInfo *info; >> + int i; >> + >> + for (i = 0; desc[i].name != NULL; i++) { >> + info = g_malloc0(sizeof(*info)); >> + info->name = g_strdup(desc[i].name); >> + >> + switch (desc[i].type) { >> + case QEMU_OPT_STRING: >> + info->type = COMMAND_LINE_PARAMETER_TYPE_STRING; >> + break; >> + case QEMU_OPT_BOOL: >> + info->type = COMMAND_LINE_PARAMETER_TYPE_BOOLEAN; >> + break; >> + case QEMU_OPT_NUMBER: >> + info->type = COMMAND_LINE_PARAMETER_TYPE_NUMBER; >> + break; >> + case QEMU_OPT_SIZE: >> + info->type = COMMAND_LINE_PARAMETER_TYPE_SIZE; >> + break; >> + } >> + >> + if (desc[i].help) { >> + info->has_help = true; >> + info->help = g_strdup(desc[i].help); >> + } >> + >> + entry = g_malloc0(sizeof(*entry)); >> + entry->value = info; >> + entry->next = param_list; >> + param_list = entry; >> + } >> + >> + return param_list; >> +} >> + >> +CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, >> + const char *option, >> + Error **errp) >> +{ >> + CommandLineOptionInfoList *conf_list = NULL, *entry; >> + CommandLineOptionInfo *info; >> + int i; >> + >> + for (i = 0; vm_config_groups[i] != NULL; i++) { >> + if (!has_option || !strcmp(option, vm_config_groups[i]->name)) { >> + info = g_malloc0(sizeof(*info)); >> + info->option = g_strdup(vm_config_groups[i]->name); >> + info->parameters = query_option_descs(vm_config_groups[i]->desc); >> + 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); > You should be using error_setg() like: > > error_setg(errp, "invalid option name: %s", option); > > But I can fix that myself in qmp branch. > >> + } >> + >> + return conf_list; >> +} >> + >> QemuOptsList *qemu_find_opts_err(const char *group, Error **errp) >> { >> return find_list(vm_config_groups, group, errp);