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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7443 bytes --]

On 04/24/2013 06:47 AM, 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.

Agreed that no HMP counterpart is needed.  However, I don't think we
have quite the right interface yet.

> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> CC: Osier Yang <jyang@redhat.com>
> CC: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json   |   29 +++++++++++++++++++++++++++++
>  qmp-commands.hx    |   40 ++++++++++++++++++++++++++++++++++++++++
>  util/qemu-config.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..aeab057 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3505,3 +3505,32 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +##
> +# @ConfigSchemaInfo:
> +#
> +# Configration schema information.
> +#
> +# @option: option name
> +#
> +# @params: parameters strList of one option

Why just a strList?  That only tells me option names.  But we already
know so much more than that - we know the type and the help string.

> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'params': ['str']} }

I'd rather see an array of structs, more closely mirroring what
include/qemu/option.h gives us:

# JSON representation of values of QEMUOptionParType, may grow in future
{ 'enum': 'ConfigParamType',
  'data': [ 'flag', 'number', 'size', 'string' ] }

# JSON representation of QEMUOptionParameter, may grow in future
# @help is optional if no text was present
{ 'type': 'ConfigParamInfo',
  'data': { 'name': 'str', 'type': 'ConfigParamType', '*help':'str' } }

# Each command line option, and its list of parameters
{ 'type': 'ConfigSchemaInfo',
  'data': { 'option':'str', 'params': ['ConfigParamInfo'] } }

And that means we no longer have ['str'], which bypasses the need for
your patch 1/2.

> +
> +##
> +# @query-config-schema
> +#
> +# Query configuration schema information of options
> +#
> +# @option: #optional option name
> +#
> +# Returns: returns @ConfigSchemaInfo if option is assigned, returns
> +#          @ConfigSchemaInfo list if no option is assigned, returns an error
> +#          QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.

That didn't read well.  Also, QERR_INVALID_OPTION_GROUP is a generic
error; we don't mention any other QERR_* names in qapi-schema.json.  How
about:

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']}

This part looks good.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..c6399be 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2414,6 +2414,46 @@ 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 option is assigned, return
> +configuration schema list of all options if no option is assigned. return
> +an error QERR_INVALID_OPTION_GROUP if assigned option doesn't exist.

Again, QERR_INVALID_OPTION_GROUP is not a defined error (it is shorthand
for a specific message for a GenericError class).

> +
> +- "option": option name
> +- "params": parameters string list of one option
> +
> +Example:
> +
> +-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}}
> +<- {
> +    "return": [
> +        {
> +            "params": [
> +                "strict",
> +                "reboot-timeout",
> +                "splash-time",
> +                "splash",
> +                "menu",
> +                "once",
> +                "order"
> +            ],
> +            "option": "boot-opts"
> +        }
> +    ]
> +  }

Back to my desire for more structure, I'd like to see:


-> {"execute": "query-config-schema", "arguments" : {"option": "boot-opts"}}
<- {
    "return": [
        {
            "params": [
                {
                    "name": "strict",
                    "type": "string"
                },
                {
                    "name": "reboot-timeout",
                    "type": "string"
                },
                ...
            ],
            "option": "boot-opts"
        }
     ]
  }

Another more interesting example, showing why the "type" and optional
"help" fields are useful, assuming
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04810.html goes in:

-> {"execute": "query-config-schema", "arguments" : {"option": "drive"}}
<- {
    "return": [
        {
            "params": [
                {
                    "name": "bus",
                    "type": "number",
                    "help": "bus number"
                },
                {
                    "name": "if",
                    "type": "string",
                    "help": "interface (ide, scsi, sd, mtd, floppy,
pflash, virtio)"
                },
                ...
            ],
            "option": "drive"
        }
     ]
  }


>  
> +ConfigSchemaInfoList *qmp_query_config_schema(bool has_option,
> +                                              const char *option, Error **errp)
> +{
> +    ConfigSchemaInfoList *conf_list = NULL, *entry;
> +    ConfigSchemaInfo *info;
> +    strList *str_list = NULL, *str_entry;
> +    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))) {
> +            info = g_malloc0(sizeof(*info));

This part of the iteration looks fine.

> +            info->option = g_strdup(vm_config_groups[i]->name);
> +            str_list = NULL;
> +
> +            for (j = 0; vm_config_groups[i]->desc[j].name != NULL; j++) {
> +                str_entry = g_malloc0(sizeof(*str_entry));
> +                str_entry->value = g_strdup(vm_config_groups[i]->desc[j].name);
> +                str_entry->next = str_list;
> +                str_list = str_entry;
> +            }

Here, you'd need to allocate a bit more structure, to match the JSON I
proposed above.

> +
> +            info->params = str_list;
> +            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);
> +    }
> +
> +    return conf_list;
> +}
> +
>  QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
>  {
>      return find_list(vm_config_groups, group, errp);
> 

Looking forward to v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]