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

[Osier Yang <jyang@redhat.com>]

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)
> +{
> +    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