From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52611) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UTBbv-0000MG-1g for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:44:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UTBbs-0005jX-It for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:44:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31470) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UTBbs-0005jN-Bg for qemu-devel@nongnu.org; Fri, 19 Apr 2013 09:44:16 -0400 Message-ID: <51714A28.2090301@redhat.com> Date: Fri, 19 Apr 2013 21:44:08 +0800 From: Osier Yang MIME-Version: 1.0 References: <1366365123-5412-1-git-send-email-akong@redhat.com> <51713241.70403@redhat.com> In-Reply-To: <51713241.70403@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: aliguori@us.ibm.com, lcapitulino@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org On 19/04/13 20:02, Osier Yang wrote: > On 19/04/13 17:52, Amos Kong wrote: >> Libvirt doesn't have a stable way to know option support > > Actually no way now. > > Libvirt swtiched to use qmp to collect the qemu capabilities > for qemu newer than 1.2.0, thus there is no way to probe either > if a option or a property is supported or not by QEMU. Any > support for the new options/properties in libvirt are blocked. > > Except we go back to probe using the help output, but it's not > right way to go. > > Consider to change the sentence to: > > Libvirt has no way to probe if an option or property is supported, > >> detail. This patch introdued a new qmp command to query > > s/introdued/introduces/ > >> configuration schema information. hmp command isn't added. > > Not quite sure if QEMU wants the hmp command, though I think no, > per all the monitor commands are converted (or going to be converted) > to qmp. Anyway, libvirt won't want it. > > >> Signed-off-by: Amos Kong >> CC: Osier Yang >> CC: Anthony Liguori >> --- >> Reposted this patch: >> https://www.redhat.com/archives/libvir-list/2013-January/msg01656.html >> --- >> qapi-schema.json | 26 ++++++++++++++++++++++++++ >> qemu-options-wrapper.h | 16 ++++++++++++++++ >> qmp-commands.hx | 32 ++++++++++++++++++++++++++++++++ >> qmp.c | 36 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 110 insertions(+), 0 deletions(-) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 751d3c2..f781372 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -3505,3 +3505,29 @@ >> '*asl_compiler_rev': 'uint32', >> '*file': 'str', >> '*data': 'str' }} >> + >> +## >> +# @ConfigSchemaInfo: >> +# >> +# Configration schema information. >> +# >> +# @option: option name >> +# >> +# @config-schema: configuration schema string of one option > > Since if no option name is specified, all the config schema is > returned, you > might want to change this comment a bit. > >> +# >> +# Since 1.5 >> +## >> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', >> 'config-schema': 'str'} } >> + >> +## >> +# @query-config-schema >> +# >> +# Query configuration schema information of options > > And this. > >> +# >> +# @option: #optional option name >> +# >> +# Returns: @ConfigSchemaInfo list >> +# >> +# Since 1.5 >> +## >> +{'command': 'query-config-schema', 'data': {'*option': 'str'}, >> 'returns': ['ConfigSchemaInfo']} >> diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h >> index 13bfea0..6449268 100644 >> --- a/qemu-options-wrapper.h >> +++ b/qemu-options-wrapper.h >> @@ -18,6 +18,22 @@ >> #define DEFHEADING(text) ARCHHEADING(text, QEMU_ARCH_ALL) >> +#elif defined(QEMU_OPTIONS_GENERATE_CONFIG) >> + >> +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \ >> + opt_help, >> + >> +#define DEFHEADING(text) >> +#define ARCHHEADING(text, arch_mask) >> + >> +#elif defined(QEMU_OPTIONS_GENERATE_NAME) > > How about QEMU_OPTIONS_GENERATE_OPTION_NAME? > >> + >> +#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \ >> + option, >> + >> +#define DEFHEADING(text) >> +#define ARCHHEADING(text, arch_mask) >> + >> #elif defined(QEMU_OPTIONS_GENERATE_OPTIONS) >> #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \ >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 4d65422..1bb691e 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -2414,7 +2414,39 @@ 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 no option is assigned, >> +will return configuration schema of all options. >> + >> +- "option": option name >> +- "config-schema": configuration schema string of one option > > And this. > >> + >> +Example: >> +-> {"execute": "query-config-schema", "arguments" : {"option": "boot"}} >> +<- {"return": [ >> + {"config-schema": "-boot >> [order=drives][,once=drives][,menu=on|off]\n >> + >> [,splash=sp_name][,splash-time=sp_time][,reboot-timeout=rb_time][,strict=on|off]\n >> + 'drives': floppy (a), hard disk (c), CD-ROM (d), network (n)\n >> + 'sp_name': the file's name that would be passed to bios as logo >> picture, if menu=on\n >> + 'sp_time': the period that splash picture last if menu=on, unit >> is ms\n >> + 'rb_timeout': the timeout before guest reboot when boot failed, >> unit is ms\n", >> + "option": "boot" >> + } >> + ] >> +} >> + >> +EQMP >> + { >> + .name = "query-config-schema", >> + .args_type = "option:s?", >> + .mhandler.cmd_new = qmp_marshal_input_query_config_schema, >> + }, > > Need one blank line. > >> SQMP >> query-migrate >> ------------- >> diff --git a/qmp.c b/qmp.c >> index ed6c7ef..a3f7cc9 100644 >> --- a/qmp.c >> +++ b/qmp.c >> @@ -24,6 +24,9 @@ >> #include "hw/qdev.h" >> #include "sysemu/blockdev.h" >> #include "qom/qom-qobject.h" >> +#include "qemu-options.h" >> +#include "net/net.h" >> +#include "exec/gdbstub.h" >> NameInfo *qmp_query_name(Error **errp) >> { >> @@ -78,6 +81,39 @@ UuidInfo *qmp_query_uuid(Error **errp) >> return info; >> } >> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option, >> + const char *option, Error **errp) Indention problem. >> +{ >> + ConfigSchemaInfoList *conf_list = NULL, *entry; >> + ConfigSchemaInfo *info; >> + >> + char const *optionstr[] = { > > s/optionstr/options/, ? > >> +#define QEMU_OPTIONS_GENERATE_NAME >> +#include "qemu-options-wrapper.h" >> + }; >> + >> + char const *configstr[] = { > > s/configstr/configs/, ? > >> +#define QEMU_OPTIONS_GENERATE_CONFIG >> +#include "qemu-options-wrapper.h" >> + }; >> + >> + int i; >> + for (i = 0; i < sizeof(optionstr) / sizeof(char *); i++) { >> + if (!has_option || !strcmp(option, optionstr[i])) { > > How about an invalid option name is passed? E.g. > > {"execute": "query-config-schema", "arguments" : {"option": "foo"}} > > Should we have an QMP error instead of sliently ignoring it with nothing > returned? > > How about changing the logic to: > > bool found = false; > > if (has_option) { > for (i = 0; i < sizeof(options) / sizeof(char *); i++) { > if (!strcmp(option, option[i])) { > found = true; > break; > } > } > > if (found) { > /* Return the found option schema */ > } else { > /* QMP error */ > } > } else { > /* Return the schema of all options */ > } > >> + info = g_malloc0(sizeof(*info)); >> + info->option = g_strdup(option); >> + info->config_schema = g_strdup(configstr[i]); >> + >> + entry = g_malloc0(sizeof(*entry)); >> + entry->value = info; >> + entry->next = conf_list; >> + conf_list = entry; > > No macros or helpers to manage the list? > > Osier >