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

[Eric Blake]

On 04/19/2013 03:52 AM, Amos Kong wrote:
> Libvirt doesn't have a stable way to know option support
> detail. This patch introdued a new qmp command to query
> configuration schema information. hmp command isn't added.

Agreed; HMP is not needed: by the time you can connect to a human
monitor, you've already invoked the command line, and humans can read
-help.  It's just libvirt (and other management apps) that need a
machine-parseable alternative to -help.

> 
> Signed-off-by: Amos Kong <address@hidden>
> CC: Osier Yang <address@hidden>
> CC: Anthony Liguori <address@hidden>
> ---
> 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(-)

In addition to Osier's review,

> +##
> +# @ConfigSchemaInfo:
> +#
> +# Configration schema information.
> +#
> +# @option: option name
> +#
> +# @config-schema: configuration schema string of one option

Any more details about the typical contents of this string?  Is it
supposed to be machine-parseable, or is it merely a human-readable
replay of -help text where the only machine-parseable aspect is whether
a particular @option name exists?

> +#
> +# Since 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'config-schema': 
> 'str'} }
> +
> +##
> +# @query-config-schema
> +#
> +# Query configuration schema information of options
> +#
> +# @option: #optional option name

I don't know that may existing query-* commands support optional
arguments for filtering; but it seems like a nice enough thing to provide.

> +#
> +# Returns: @ConfigSchemaInfo list
> +#
> +# Since 1.5
> +##
> +{'command': 'query-config-schema', 'data': {'*option': 'str'}, 'returns': 
> ['ConfigSchemaInfo']}

Long line; it might be worth wrapping this similar to what other
commands have done.

> +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
> +
> +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",

Ah, the example makes it clear - this is human-readable text from -help
output, and not something that is further machine-parseable.  Which
means that if we have an existing command line version in one qemu
release, and the next qemu release adds more features to that option, we
have no reliable way (other than string-scraping) to detect that new
feature of the existing option.  Can we do any better?

I was expecting more of a JSON representation of the
QEMUOptionParameter[] used to parse a given option - THAT would be
machine-parseable, and would also let us know when an option has learned
more arguments in a newer qemu version.  But it is also something that
can be added later (we've already decided that query-* commands can add
output fields to their dictionary as needed).

The name 'config-schema' sounds like it is machine-parseable; can we use
a more descriptive name such as 'help-text' that makes it obvious what
you are really providing?

> +++ 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"

What does net/net.h have to do with anything?

Overall, looks quite useful, and it is worth getting into 1.5.

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

signature.asc
Description: OpenPGP digital signature