Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema

[Osier Yang <jyang@redhat.com>]

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 <akong@redhat.com>
>> CC: Osier Yang <jyang@redhat.com>
>> CC: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> 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
>