* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 9:52 [Qemu-devel] [PATCH] monitor: introduce query-config-schema Amos Kong
@ 2013-04-19 12:02 ` Osier Yang
2013-04-19 12:22 ` Eric Blake
2013-04-19 13:44 ` Osier Yang
2013-04-19 12:39 ` Eric Blake
2013-04-19 15:21 ` Paolo Bonzini
2 siblings, 2 replies; 15+ messages in thread
From: Osier Yang @ 2013-04-19 12:02 UTC (permalink / raw)
To: Amos Kong; +Cc: mdroth, aliguori, qemu-devel, lcapitulino
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 12:02 ` Osier Yang
@ 2013-04-19 12:22 ` Eric Blake
2013-04-19 13:44 ` Osier Yang
1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2013-04-19 12:22 UTC (permalink / raw)
To: Osier Yang; +Cc: lcapitulino, aliguori, Amos Kong, mdroth, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]
On 04/19/2013 06:02 AM, 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.
>
>> +
>> +##
>> +# @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.
No, this part is correct. This is an intermediate type for one option,
then the QMP command returns an array of these types.
>
>> +#
>> +# Since 1.5
>> +##
>> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str',
>> 'config-schema': 'str'} }
>> +
>> +##
>> +# @query-config-schema
>> +#
>> +# Query configuration schema information of options
>
> And this.
But here, it might make sense to call out that the default is all
options, but specifying a name limits to one option.
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 12:02 ` Osier Yang
2013-04-19 12:22 ` Eric Blake
@ 2013-04-19 13:44 ` Osier Yang
1 sibling, 0 replies; 15+ messages in thread
From: Osier Yang @ 2013-04-19 13:44 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, lcapitulino, mdroth, qemu-devel
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 9:52 [Qemu-devel] [PATCH] monitor: introduce query-config-schema Amos Kong
2013-04-19 12:02 ` Osier Yang
@ 2013-04-19 12:39 ` Eric Blake
2013-04-19 13:50 ` Osier Yang
2013-04-19 15:21 ` Paolo Bonzini
2 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2013-04-19 12:39 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, lcapitulino, qemu-devel, jyang, mdroth
[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]
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 <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(-)
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 12:39 ` Eric Blake
@ 2013-04-19 13:50 ` Osier Yang
0 siblings, 0 replies; 15+ messages in thread
From: Osier Yang @ 2013-04-19 13:50 UTC (permalink / raw)
To: Eric Blake; +Cc: lcapitulino, aliguori, Amos Kong, qemu-devel, mdroth
On 19/04/13 20:39, Eric Blake wrote:
> 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 <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(-)
> 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.
Agreed. Parsing with the string is not a happy work. Though the existing
code in libvirt can be reused.
> 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).
I'm wondering if can have a JSON dictionary from the beginning.
>
> 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?
It's required for building, see the two #includes of qemu-options-wrapper.h
Osier
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 9:52 [Qemu-devel] [PATCH] monitor: introduce query-config-schema Amos Kong
2013-04-19 12:02 ` Osier Yang
2013-04-19 12:39 ` Eric Blake
@ 2013-04-19 15:21 ` Paolo Bonzini
2013-04-22 11:48 ` Amos Kong
2 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:21 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, lcapitulino, qemu-devel, jyang, mdroth
Il 19/04/2013 11:52, Amos Kong ha scritto:
> 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.
Can you introspect QemuOpts instead? All new options are added there.
Paolo
> 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 1.5
> +##
> +{ 'type': 'ConfigSchemaInfo', 'data': {'option': 'str', 'config-schema': 'str'} }
> +
> +##
> +# @query-config-schema
> +#
> +# Query configuration schema information of options
> +#
> +# @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)
> +
> +#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
> +
> +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,
> + },
> 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[] = {
> +#define QEMU_OPTIONS_GENERATE_NAME
> +#include "qemu-options-wrapper.h"
> + };
> +
> + char const *configstr[] = {
> +#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])) {
> + 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;
> + }
> + }
> +
> + return conf_list;
> +}
> +
> void qmp_quit(Error **err)
> {
> no_shutdown = 0;
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-19 15:21 ` Paolo Bonzini
@ 2013-04-22 11:48 ` Amos Kong
2013-04-22 12:07 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Amos Kong @ 2013-04-22 11:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: mdroth, aliguori, qemu-devel, jyang, lcapitulino
On Fri, Apr 19, 2013 at 05:21:37PM +0200, Paolo Bonzini wrote:
> Il 19/04/2013 11:52, Amos Kong ha scritto:
> > 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.
>
> Can you introspect QemuOpts instead? All new options are added there.
It would be exact to use QemuOpts. I tried to output the vm_config_groups[]
in qemu-config.c, but it seems not enough. (desc list of -netdev, -drive,
-device are all empty)
Is there a better way to go through _all_ the QemuOpts?
Amos.
name: drive
name: chardev
\ desc->name: backend
\ desc->name: path
\ desc->name: host
\ desc->name: port
\ desc->name: localaddr
\ desc->name: localport
\ desc->name: to
\ desc->name: ipv4
\ desc->name: ipv6
\ desc->name: wait
\ desc->name: server
\ desc->name: delay
\ desc->name: telnet
\ desc->name: width
\ desc->name: height
\ desc->name: cols
\ desc->name: rows
\ desc->name: mux
\ desc->name: signal
\ desc->name: name
\ desc->name: debug
\ desc->name: size
name: device
name: netdev
name: net
name: rtc
\ desc->name: base
\ desc->name: clock
\ desc->name: driftfix
name: global
\ desc->name: driver
\ desc->name: property
\ desc->name: value
name: mon
\ desc->name: mode
\ desc->name: chardev
\ desc->name: default
\ desc->name: pretty
name: trace
\ desc->name: events
\ desc->name: file
name: option-rom
\ desc->name: bootindex
\ desc->name: romfile
name: machine
\ desc->name: type
\ desc->name: accel
\ desc->name: kernel_irqchip
\ desc->name: kvm_shadow_mem
\ desc->name: kernel
\ desc->name: initrd
\ desc->name: append
\ desc->name: dtb
\ desc->name: dumpdtb
\ desc->name: phandle_start
\ desc->name: dt_compatible
\ desc->name: dump-guest-core
\ desc->name: mem-merge
\ desc->name: usb
name: boot-opts
\ desc->name: order
\ desc->name: once
\ desc->name: menu
\ desc->name: splash
\ desc->name: splash-time
\ desc->name: reboot-timeout
\ desc->name: strict
name: sandbox
\ desc->name: enable
name: add-fd
\ desc->name: fd
\ desc->name: set
\ desc->name: opaque
name: object
name: tpmdev
\ desc->name: type
\ desc->name: cancel-path
\ desc->name: path
name: acpi
name: fsdev
\ desc->name: fsdriver
\ desc->name: path
\ desc->name: security_model
\ desc->name: writeout
\ desc->name: readonly
\ desc->name: socket
\ desc->name: sock_fd
name: virtfs
\ desc->name: fsdriver
\ desc->name: path
\ desc->name: mount_tag
\ desc->name: security_model
\ desc->name: writeout
\ desc->name: readonly
\ desc->name: socket
\ desc->name: sock_fd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-22 11:48 ` Amos Kong
@ 2013-04-22 12:07 ` Paolo Bonzini
2013-04-22 15:00 ` Eric Blake
0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2013-04-22 12:07 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, lcapitulino, mdroth, jyang, qemu-devel
Il 22/04/2013 13:48, Amos Kong ha scritto:
>>> > > 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.
>> >
>> > Can you introspect QemuOpts instead? All new options are added there.
>
> It would be exact to use QemuOpts. I tried to output the vm_config_groups[]
> in qemu-config.c, but it seems not enough. (desc list of -netdev, -drive,
> -device are all empty)
That's expected because they are parsed otherwise, depending on the
backend type. -chardev is currently working but it's an implementation
detail.
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-22 12:07 ` Paolo Bonzini
@ 2013-04-22 15:00 ` Eric Blake
2013-04-22 15:16 ` Paolo Bonzini
2013-04-23 13:20 ` Luiz Capitulino
0 siblings, 2 replies; 15+ messages in thread
From: Eric Blake @ 2013-04-22 15:00 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, mdroth, jyang, lcapitulino, Amos Kong
[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]
On 04/22/2013 06:07 AM, Paolo Bonzini wrote:
> Il 22/04/2013 13:48, Amos Kong ha scritto:
>>>>>> 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.
>>>>
>>>> Can you introspect QemuOpts instead? All new options are added there.
>>
>> It would be exact to use QemuOpts. I tried to output the vm_config_groups[]
>> in qemu-config.c, but it seems not enough. (desc list of -netdev, -drive,
>> -device are all empty)
>
> That's expected because they are parsed otherwise, depending on the
> backend type. -chardev is currently working but it's an implementation
> detail.
Libvirt cares most about newly added options, which should use qemuOpts
all the way. We can understand that legacy options like -netdev might
not yet use qemuOpts, but they are also legacy options, and therefore
libvirt can already assume they exist since at least qemu 1.3 (when
libvirt switched over to QMP probing). If we later add a new feature to
-netdev, we should also convert -netdev to qemuOpts at that time, so
that libvirt would know whether the new feature is available.
At any rate, we really DO want introspection, and having it in 1.5 is a
worthwhile goal. Even if the introspection turns up empty on legacy
options, having it for the sake of new options is worth the effort.
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-22 15:00 ` Eric Blake
@ 2013-04-22 15:16 ` Paolo Bonzini
2013-04-23 2:40 ` Amos Kong
2013-04-23 13:20 ` Luiz Capitulino
1 sibling, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2013-04-22 15:16 UTC (permalink / raw)
To: Eric Blake; +Cc: aliguori, qemu-devel, mdroth, jyang, lcapitulino, Amos Kong
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 22/04/2013 17:00, Eric Blake ha scritto:
> On 04/22/2013 06:07 AM, Paolo Bonzini wrote:
>> Il 22/04/2013 13:48, Amos Kong ha scritto:
>>>>>>> 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.
>>>>>
>>>>> Can you introspect QemuOpts instead? All new options are
>>>>> added there.
>>>
>>> It would be exact to use QemuOpts. I tried to output the
>>> vm_config_groups[] in qemu-config.c, but it seems not enough.
>>> (desc list of -netdev, -drive, -device are all empty)
>>
>> That's expected because they are parsed otherwise, depending on
>> the backend type. -chardev is currently working but it's an
>> implementation detail.
>
> Libvirt cares most about newly added options, which should use
> qemuOpts all the way. We can understand that legacy options like
> -netdev might not yet use qemuOpts, but they are also legacy
> options, and therefore libvirt can already assume they exist since
> at least qemu 1.3 (when libvirt switched over to QMP probing). If
> we later add a new feature to -netdev, we should also convert
> -netdev to qemuOpts at that time, so that libvirt would know
> whether the new feature is available.
- -netdev is not a legacy option. -netdev/-drive/-device do use
QemuOpts, but not for validation. They create an object, and let the
object parse the option.
They are more complex than the other option, and need a different kind
of introspection (on the properties of a class, or something like that).
Paolo
> At any rate, we really DO want introspection, and having it in 1.5
> is a worthwhile goal. Even if the introspection turns up empty on
> legacy options, having it for the sake of new options is worth the
> effort.
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRdVQyAAoJEBvWZb6bTYbyopcP/jE5TX/NziMeXmXmSS+GFn4G
4SH+4XckjZZPe2ZnpshvkWi1OrbsK0CKw9nk4xcZRFmUFnIZ4T1J2VBXodnATvKZ
+r6yqvOCuoleWQBlnN8OJm/I5kil5UkztiUSsDbgYyP2D4pr3Z7+uGX7ju/4oGCK
qEASGYRsGFItvKkjfUDL2UjBv3dnDerWSPj9IsD/sFajGqcBrvfbGK8YeOG7YvQF
Tinv5dhHg9dpnTJ/fwmw0xr3BmgzLf4fT16I73ErG1INOBjWSUPkQ0h8UeydJEJq
nvpj3/zlqqJdSNxZXU1FRP+stQN2hHDZsTXhKuY+2kbDFRqpwB8WG94TEbOdx6gr
fyNHfueWIByylmQNgbvBCyR2hVI+RipgGHfQ6slTcMMu2pZpZ1m9vWfZF8bvAS+p
NL4+Rf+Ic4uMKZnS6GvD15px0ugGPIcDdwX7YgVqjNMIRZhKFiOf9HYnUfJstQpN
WrEpcnyE4p0JzkO27Otezoz+RTpJ8HaKHvnsbM49BDlWDMme7jKveymWnCyeJxib
g7Hz7V7M76LK0Mlcn66PYctF6JVZP25hC3p3poYbm2F9Duvwg78+b53O6k1XN0sP
/kuZfYWrWRr6sx+/oN6HWMP5q60jRVYKUOYcNOriWS+6Yi8xohFvqQVu4qmycXOG
3HBqamagi+JNMiiO9cx/
=4p7B
-----END PGP SIGNATURE-----
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-22 15:16 ` Paolo Bonzini
@ 2013-04-23 2:40 ` Amos Kong
0 siblings, 0 replies; 15+ messages in thread
From: Amos Kong @ 2013-04-23 2:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, mdroth, jyang, lcapitulino
On Mon, Apr 22, 2013 at 05:16:02PM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Il 22/04/2013 17:00, Eric Blake ha scritto:
> > On 04/22/2013 06:07 AM, Paolo Bonzini wrote:
> >> Il 22/04/2013 13:48, Amos Kong ha scritto:
> >>>>>>> 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.
> >>>>>
> >>>>> Can you introspect QemuOpts instead? All new options are
> >>>>> added there.
> >>>
> >>> It would be exact to use QemuOpts. I tried to output the
> >>> vm_config_groups[] in qemu-config.c, but it seems not enough.
> >>> (desc list of -netdev, -drive, -device are all empty)
> >>
> >> That's expected because they are parsed otherwise, depending on
> >> the backend type. -chardev is currently working but it's an
> >> implementation detail.
> >
> > Libvirt cares most about newly added options, which should use
> > qemuOpts all the way. We can understand that legacy options like
> > -netdev might not yet use qemuOpts, but they are also legacy
> > options, and therefore libvirt can already assume they exist since
> > at least qemu 1.3 (when libvirt switched over to QMP probing). If
> > we later add a new feature to -netdev, we should also convert
> > -netdev to qemuOpts at that time, so that libvirt would know
> > whether the new feature is available.
>
> - -netdev is not a legacy option. -netdev/-drive/-device do use
> QemuOpts, but not for validation. They create an object, and let the
> object parse the option.
>
> They are more complex than the other option, and need a different kind
> of introspection (on the properties of a class, or something like that).
'-netdev fds=...' was added after supported multiqueue, but we can't
check it from the output of vm_config_groups[]
Do we need to process all non-legacy options for validation first?
then add the query command.
> Paolo
>
> > At any rate, we really DO want introspection, and having it in 1.5
> > is a worthwhile goal. Even if the introspection turns up empty on
> > legacy options, having it for the sake of new options is worth the
> > effort.
Ok, I will work on another patch to output vm_config_groups[] with
clear JSON structure. Thanks
--
Amos.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-22 15:00 ` Eric Blake
2013-04-22 15:16 ` Paolo Bonzini
@ 2013-04-23 13:20 ` Luiz Capitulino
2013-04-23 15:32 ` Eric Blake
1 sibling, 1 reply; 15+ messages in thread
From: Luiz Capitulino @ 2013-04-23 13:20 UTC (permalink / raw)
To: Eric Blake; +Cc: aliguori, mdroth, qemu-devel, jyang, Paolo Bonzini, Amos Kong
On Mon, 22 Apr 2013 09:00:05 -0600
Eric Blake <eblake@redhat.com> wrote:
> At any rate, we really DO want introspection, and having it in 1.5 is a
> worthwhile goal. Even if the introspection turns up empty on legacy
> options, having it for the sake of new options is worth the effort.
Agreed. But as you said in another email, we need a JSON representation
for this command. Dumping all options as strings is just like using -help.
Something else that has occurred to me is that, do we want this to be
a QMP command or do we want this to be a command-line option? If we do this
as a QMP command then libvirt would have to actually start QEMU just to
query supported options.
Another interesting point is about full schema introspection. This patch
only supports command-line options. It would be desirable to be able to
introspect all QAPI types and QMP commands, although I'm aware that this
might be a lot of work to do for 1.5.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-23 13:20 ` Luiz Capitulino
@ 2013-04-23 15:32 ` Eric Blake
2013-04-23 16:55 ` Luiz Capitulino
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2013-04-23 15:32 UTC (permalink / raw)
To: Luiz Capitulino
Cc: aliguori, mdroth, qemu-devel, jyang, Paolo Bonzini, Amos Kong
[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]
On 04/23/2013 07:20 AM, Luiz Capitulino wrote:
> On Mon, 22 Apr 2013 09:00:05 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
>> At any rate, we really DO want introspection, and having it in 1.5 is a
>> worthwhile goal. Even if the introspection turns up empty on legacy
>> options, having it for the sake of new options is worth the effort.
>
> Agreed. But as you said in another email, we need a JSON representation
> for this command. Dumping all options as strings is just like using -help.
>
> Something else that has occurred to me is that, do we want this to be
> a QMP command or do we want this to be a command-line option? If we do this
> as a QMP command then libvirt would have to actually start QEMU just to
> query supported options.
A command-line option might (or might not) be useful to humans, but a
QMP command is what libvirt wants. Libvirt already starts a QMP session
for lots of other queries (not the least of which is the
'query-commands' query which would let us know whether to even expect
the new QMP command for command-line introspection to work). Adding one
more QMP query to the mix called by libvirt is easy (no additional
processes, and the response already goes through the JSON parser that
handles all QMP responses); adding a new command line option is more
expensive (we would have to start qemu once to use the command line
option, then again for the QMP queries; also, it's more coding effort to
wire up libvirt to pass the stdout from the command line call into the
JSON parser that was normally expecting data from a Unix socket).
>
> Another interesting point is about full schema introspection. This patch
> only supports command-line options. It would be desirable to be able to
> introspect all QAPI types and QMP commands, although I'm aware that this
> might be a lot of work to do for 1.5.
Based on the call today, getting introspection for command line
additions in time for 1.5, and saving full QMP introspection until 1.6,
would be reasonable; libvirt's immediate goal is to determine when it is
safe to use new command line options while doing its one-shot QMP
probing of a qemu binary.
--
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: introduce query-config-schema
2013-04-23 15:32 ` Eric Blake
@ 2013-04-23 16:55 ` Luiz Capitulino
0 siblings, 0 replies; 15+ messages in thread
From: Luiz Capitulino @ 2013-04-23 16:55 UTC (permalink / raw)
To: Eric Blake; +Cc: aliguori, mdroth, qemu-devel, jyang, Paolo Bonzini, Amos Kong
On Tue, 23 Apr 2013 09:32:31 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 04/23/2013 07:20 AM, Luiz Capitulino wrote:
> > On Mon, 22 Apr 2013 09:00:05 -0600
> > Eric Blake <eblake@redhat.com> wrote:
> >
> >> At any rate, we really DO want introspection, and having it in 1.5 is a
> >> worthwhile goal. Even if the introspection turns up empty on legacy
> >> options, having it for the sake of new options is worth the effort.
> >
> > Agreed. But as you said in another email, we need a JSON representation
> > for this command. Dumping all options as strings is just like using -help.
> >
> > Something else that has occurred to me is that, do we want this to be
> > a QMP command or do we want this to be a command-line option? If we do this
> > as a QMP command then libvirt would have to actually start QEMU just to
> > query supported options.
>
> A command-line option might (or might not) be useful to humans, but a
> QMP command is what libvirt wants. Libvirt already starts a QMP session
> for lots of other queries (not the least of which is the
> 'query-commands' query which would let us know whether to even expect
> the new QMP command for command-line introspection to work). Adding one
> more QMP query to the mix called by libvirt is easy (no additional
> processes, and the response already goes through the JSON parser that
> handles all QMP responses);
That's right.
> adding a new command line option is more
> expensive (we would have to start qemu once to use the command line
> option, then again for the QMP queries; also, it's more coding effort to
> wire up libvirt to pass the stdout from the command line call into the
> JSON parser that was normally expecting data from a Unix socket).
>
> >
> > Another interesting point is about full schema introspection. This patch
> > only supports command-line options. It would be desirable to be able to
> > introspect all QAPI types and QMP commands, although I'm aware that this
> > might be a lot of work to do for 1.5.
>
> Based on the call today, getting introspection for command line
> additions in time for 1.5, and saving full QMP introspection until 1.6,
> would be reasonable; libvirt's immediate goal is to determine when it is
> safe to use new command line options while doing its one-shot QMP
> probing of a qemu binary.
Yes. All my points have been clarified in today's meeting.
^ permalink raw reply [flat|nested] 15+ messages in thread