[RFC] introduce a general query-config cmd (was: [Qemu PATCH v2] add a boot option to do strict boot)

On Tue, Jan 22, 2013 at 12:26:03PM -0600, Anthony Liguori wrote:
> Eric Blake <eblake@redhat.com> writes:
> > On 01/22/2013 08:52 AM, Amos Kong wrote:
> >>>>
> >>>> Libvirt will need to expose an attribute that lets the user control
> >>>> whether to use this new option; how do we probe via QMP whether the
> >>>> new
> >>>> -boot strict=on command-line option is available?
> >>>
> >>> Hi all,
> >>>
> >>> How about add new info/query command?
> >>>
> >>> (hmp) info strict-boot
> >>>       on
> >>>
> >>> (qmp) {"execute": "query-strict-boot"}
> >>>       {"return": {"state": true}}
> >> 
> >> It might be not a good solution, I already updated qemu-options.hx,
> >> we can check help message to know if this new option is added or not.
> >
> > Having libvirt probe the -help output is out of the question.  We
> > already declared that for qemu 1.3 and beyond, ALL command line behavior
> > must ALSO be probe-able via QMP.  I think Anthony had a trick for
> > testing for existence of various command line options without needing to
> > add a new query-strict-boot command, but I don't remember what that
> > trick was.
> 
> We need a generic query-config-schema command.


Hi all,

The config info is included in qemu-options.def, current
help() defines QEMU_OPTIONS_GENERATE_HELP, and includes
qemu-options-wrapper.h,  DEF macro will output the help 
message of options to the stdio.

I tried to add two branches in qemu-options-wrapper.h, which
are used to generate two string arraies (one for option name,
one for help message)

Thy help message is too long, it's failed to output all message to
hmp monitor, always got a segfault, the max limitation maybe reached.
It's more clear to only output help message of one option,

eg: {"execute": "query-config", "arguments" : {"name": "boot"}}

{"return": {"config": "-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"}}

But info command of hmp doesn't support parameter
  (eg: (hmp) info config boot)


Q1) Is my patch ok for resolve the issue in last email (libvirt can check
the help message of each option)?

Q2) Can we only added a command for qmp monitor?

Q3) Is it ok to output all help message for hmp (info config)?

Q4) query-config or query-config-schema ?

Amos Kong <akong@redhat.com> writes:

[...]
> But info command of hmp doesn't support parameter
>   (eg: (hmp) info config boot)

It does since Wenchao Xia's recent improvements (commit 84c44613).  For
an example how to use them, check out his "hmp: add function
hmp_info_snapshots()" (on list, not yet committed).

[More questions, left to more competent folks...]

On Wed, Jan 23, 2013 at 06:41:44PM +0800, Amos Kong wrote:
> On Tue, Jan 22, 2013 at 12:26:03PM -0600, Anthony Liguori wrote:
> > Eric Blake <eblake@redhat.com> writes:
> > > On 01/22/2013 08:52 AM, Amos Kong wrote:
> > >>>>
> > >>>> Libvirt will need to expose an attribute that lets the user control
> > >>>> whether to use this new option; how do we probe via QMP whether the
> > >>>> new
> > >>>> -boot strict=on command-line option is available?
> > >>>
> > >>> Hi all,
> > >>>
> > >>> How about add new info/query command?
> > >>>
> > >>> (hmp) info strict-boot
> > >>>       on
> > >>>
> > >>> (qmp) {"execute": "query-strict-boot"}
> > >>>       {"return": {"state": true}}
> > >> 
> > >> It might be not a good solution, I already updated qemu-options.hx,
> > >> we can check help message to know if this new option is added or not.
> > >
> > > Having libvirt probe the -help output is out of the question.  We
> > > already declared that for qemu 1.3 and beyond, ALL command line behavior
> > > must ALSO be probe-able via QMP.  I think Anthony had a trick for
> > > testing for existence of various command line options without needing to
> > > add a new query-strict-boot command, but I don't remember what that
> > > trick was.
> > 
> > We need a generic query-config-schema command.
> 
> 
> Hi all,
> 
> The config info is included in qemu-options.def, current
> help() defines QEMU_OPTIONS_GENERATE_HELP, and includes
> qemu-options-wrapper.h,  DEF macro will output the help 
> message of options to the stdio.
> 
> I tried to add two branches in qemu-options-wrapper.h, which
> are used to generate two string arraies (one for option name,
> one for help message)
> 
> Thy help message is too long, it's failed to output all message to
> hmp monitor, always got a segfault, the max limitation maybe reached.
> It's more clear to only output help message of one option,
> 
> eg: {"execute": "query-config", "arguments" : {"name": "boot"}}
> 
> {"return": {"config": "-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"}}
> 
> But info command of hmp doesn't support parameter
>   (eg: (hmp) info config boot)
 
Hi Anthony,

As we talked in IRC, you will send a patch to resolve the query issue.
Can you post patch out when you have time? I will review it.

Thanks, Amos
 
> Q1) Is my patch ok for resolve the issue in last email (libvirt can check
> the help message of each option)?
> 
> Q2) Can we only added a command for qmp monitor?
> 
> Q3) Is it ok to output all help message for hmp (info config)?
> 
> Q4) query-config or query-config-schema ?
> 
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..13d1840 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1552,6 +1552,8 @@ show the vnc server status
>  show the current VM name
>  @item info uuid
>  show the current VM UUID
> +@item info config
> +show config
>  @item info cpustats
>  show CPU statistics
>  @item info usernet
> diff --git a/hmp.c b/hmp.c
> index 9e9e624..c0d84d1 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -98,6 +98,17 @@ void hmp_info_uuid(Monitor *mon)
>      qapi_free_UuidInfo(info);
>  }
>  
> +void hmp_info_config(Monitor *mon)
> +{
> +    ConfigInfo *info;
> +
> +    /* Fixed ME: hmp info command doesn't support parameter */
> +
> +    info = qmp_query_config("boot", NULL);
> +    monitor_printf(mon, "%s\n", info->config);
> +    qapi_free_ConfigInfo(info);
> +}
> +
>  void hmp_info_chardev(Monitor *mon)
>  {
>      ChardevInfoList *char_info, *info;
> diff --git a/hmp.h b/hmp.h
> index 21f3e05..f217a8c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -23,6 +23,7 @@ void hmp_info_version(Monitor *mon);
>  void hmp_info_kvm(Monitor *mon);
>  void hmp_info_status(Monitor *mon);
>  void hmp_info_uuid(Monitor *mon);
> +void hmp_info_config(Monitor *mon);
>  void hmp_info_chardev(Monitor *mon);
>  void hmp_info_mice(Monitor *mon);
>  void hmp_info_migrate(Monitor *mon);
> diff --git a/monitor.c b/monitor.c
> index 9cf419b..6f331fa 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2661,6 +2661,13 @@ static mon_cmd_t info_cmds[] = {
>          .help       = "show the current VM UUID",
>          .mhandler.info = hmp_info_uuid,
>      },
> +    {
> +        .name       = "config",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the config",
> +        .mhandler.info = hmp_info_config,
> +    },
>  #if defined(TARGET_PPC)
>      {
>          .name       = "cpustats",
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5dfa052..8c46d57 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3017,3 +3017,23 @@
>  # Since: 1.3.0
>  ##
>  { 'command': 'nbd-server-stop' }
> +
> +##
> +# @ConfigInfo:
> +#
> +# Commandline configration information.
> +#
> +##
> +{ 'type': 'ConfigInfo', 'data': {'config': 'str'} }
> +
> +##
> +# @query-config
> +#
> +# Query configuration information of one option
> +#
> +# @name: option name
> +#
> +# Returns: configuration information.
> +#
> +##
> +{'command': 'query-config', 'data': {'name': 'str'}, 'returns': 'ConfigInfo'}
> diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h
> index 13bfea0..97b44fb 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 5c692d0..ed42525 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2339,7 +2339,30 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
>      },
> +SQMP
> +query-config
> +------------
> +
> +Show config.
> +
> +- "Config": config
> +
> +Example:
> +-> {"execute": "query-config", "arguments" : {"name": "boot"}}
> +<- {"return": {"config": "-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"}}
> +
> +EQMP
>  
> +    {
> +        .name       = "query-config",
> +        .args_type  = "name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_query_config,
> +    },
>  SQMP
>  query-migrate
>  -------------
> diff --git a/qmp.c b/qmp.c
> index 55b056b..6a3a13a 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -24,6 +24,7 @@
>  #include "hw/qdev.h"
>  #include "sysemu/blockdev.h"
>  #include "qom/qom-qobject.h"
> +#include "qemu-options.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -78,6 +79,31 @@ UuidInfo *qmp_query_uuid(Error **errp)
>      return info;
>  }
>  
> +ConfigInfo *qmp_query_config(const char *name, Error **errp)
> +{
> +    ConfigInfo *info = g_malloc0(sizeof(*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 (!strcmp(name, optionstr[i])) {
> +            info->config = g_strdup(configstr[i]);
> +            break;
> +        }
> +    }
> +
> +    return info;
> +}
> +
>  void qmp_quit(Error **err)
>  {
>      no_shutdown = 0;

On Mon, Jan 28, 2013 at 11:19:34AM +0800, Amos Kong wrote:
> On Wed, Jan 23, 2013 at 06:41:44PM +0800, Amos Kong wrote:
> > On Tue, Jan 22, 2013 at 12:26:03PM -0600, Anthony Liguori wrote:
> > > Eric Blake <eblake@redhat.com> writes:
> > > > On 01/22/2013 08:52 AM, Amos Kong wrote:
> > > >>>>
> > > >>>> Libvirt will need to expose an attribute that lets the user control
> > > >>>> whether to use this new option; how do we probe via QMP whether the
> > > >>>> new
> > > >>>> -boot strict=on command-line option is available?
> > > >>>
> > > >>> Hi all,
> > > >>>
> > > >>> How about add new info/query command?
> > > >>>
> > > >>> (hmp) info strict-boot
> > > >>>       on
> > > >>>
> > > >>> (qmp) {"execute": "query-strict-boot"}
> > > >>>       {"return": {"state": true}}
> > > >> 
> > > >> It might be not a good solution, I already updated qemu-options.hx,
> > > >> we can check help message to know if this new option is added or not.
> > > >
> > > > Having libvirt probe the -help output is out of the question.  We
> > > > already declared that for qemu 1.3 and beyond, ALL command line behavior
> > > > must ALSO be probe-able via QMP.  I think Anthony had a trick for
> > > > testing for existence of various command line options without needing to
> > > > add a new query-strict-boot command, but I don't remember what that
> > > > trick was.
> > > 
> > > We need a generic query-config-schema command.
> > 
> > 
> > Hi all,
> > 
> > The config info is included in qemu-options.def, current
> > help() defines QEMU_OPTIONS_GENERATE_HELP, and includes
> > qemu-options-wrapper.h,  DEF macro will output the help 
> > message of options to the stdio.
> > 
> > I tried to add two branches in qemu-options-wrapper.h, which
> > are used to generate two string arraies (one for option name,
> > one for help message)
> > 
> > Thy help message is too long, it's failed to output all message to
> > hmp monitor, always got a segfault, the max limitation maybe reached.
> > It's more clear to only output help message of one option,
> > 
> > eg: {"execute": "query-config", "arguments" : {"name": "boot"}}
> > 
> > {"return": {"config": "-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"}}
> > 
> > But info command of hmp doesn't support parameter
> >   (eg: (hmp) info config boot)
>  
> Hi Anthony,
> 
> As we talked in IRC, you will send a patch to resolve the query issue.
> Can you post patch out when you have time? I will review it.

Anthony, ping :)

[1] http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg01259.html
[2] http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05167.html
 
> Thanks, Amos
>  
> > Q1) Is my patch ok for resolve the issue in last email (libvirt can check
> > the help message of each option)?
> > 
> > Q2) Can we only added a command for qmp monitor?
> > 
> > Q3) Is it ok to output all help message for hmp (info config)?
> > 
> > Q4) query-config or query-config-schema ?
> > 
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 010b8c9..13d1840 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1552,6 +1552,8 @@ show the vnc server status
> >  show the current VM name
> >  @item info uuid
> >  show the current VM UUID
> > +@item info config
> > +show config
> >  @item info cpustats
> >  show CPU statistics
> >  @item info usernet
> > diff --git a/hmp.c b/hmp.c
> > index 9e9e624..c0d84d1 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -98,6 +98,17 @@ void hmp_info_uuid(Monitor *mon)
> >      qapi_free_UuidInfo(info);
> >  }
> >  
> > +void hmp_info_config(Monitor *mon)
> > +{
> > +    ConfigInfo *info;
> > +
> > +    /* Fixed ME: hmp info command doesn't support parameter */
> > +
> > +    info = qmp_query_config("boot", NULL);
> > +    monitor_printf(mon, "%s\n", info->config);
> > +    qapi_free_ConfigInfo(info);
> > +}
> > +
> >  void hmp_info_chardev(Monitor *mon)
> >  {
> >      ChardevInfoList *char_info, *info;
> > diff --git a/hmp.h b/hmp.h
> > index 21f3e05..f217a8c 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -23,6 +23,7 @@ void hmp_info_version(Monitor *mon);
> >  void hmp_info_kvm(Monitor *mon);
> >  void hmp_info_status(Monitor *mon);
> >  void hmp_info_uuid(Monitor *mon);
> > +void hmp_info_config(Monitor *mon);
> >  void hmp_info_chardev(Monitor *mon);
> >  void hmp_info_mice(Monitor *mon);
> >  void hmp_info_migrate(Monitor *mon);
> > diff --git a/monitor.c b/monitor.c
> > index 9cf419b..6f331fa 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2661,6 +2661,13 @@ static mon_cmd_t info_cmds[] = {
> >          .help       = "show the current VM UUID",
> >          .mhandler.info = hmp_info_uuid,
> >      },
> > +    {
> > +        .name       = "config",
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "show the config",
> > +        .mhandler.info = hmp_info_config,
> > +    },
> >  #if defined(TARGET_PPC)
> >      {
> >          .name       = "cpustats",
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5dfa052..8c46d57 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3017,3 +3017,23 @@
> >  # Since: 1.3.0
> >  ##
> >  { 'command': 'nbd-server-stop' }
> > +
> > +##
> > +# @ConfigInfo:
> > +#
> > +# Commandline configration information.
> > +#
> > +##
> > +{ 'type': 'ConfigInfo', 'data': {'config': 'str'} }
> > +
> > +##
> > +# @query-config
> > +#
> > +# Query configuration information of one option
> > +#
> > +# @name: option name
> > +#
> > +# Returns: configuration information.
> > +#
> > +##
> > +{'command': 'query-config', 'data': {'name': 'str'}, 'returns': 'ConfigInfo'}
> > diff --git a/qemu-options-wrapper.h b/qemu-options-wrapper.h
> > index 13bfea0..97b44fb 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 5c692d0..ed42525 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -2339,7 +2339,30 @@ EQMP
> >          .args_type  = "",
> >          .mhandler.cmd_new = qmp_marshal_input_query_uuid,
> >      },
> > +SQMP
> > +query-config
> > +------------
> > +
> > +Show config.
> > +
> > +- "Config": config
> > +
> > +Example:
> > +-> {"execute": "query-config", "arguments" : {"name": "boot"}}
> > +<- {"return": {"config": "-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"}}
> > +
> > +EQMP
> >  
> > +    {
> > +        .name       = "query-config",
> > +        .args_type  = "name:s",
> > +        .mhandler.cmd_new = qmp_marshal_input_query_config,
> > +    },
> >  SQMP
> >  query-migrate
> >  -------------
> > diff --git a/qmp.c b/qmp.c
> > index 55b056b..6a3a13a 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/qdev.h"
> >  #include "sysemu/blockdev.h"
> >  #include "qom/qom-qobject.h"
> > +#include "qemu-options.h"
> >  
> >  NameInfo *qmp_query_name(Error **errp)
> >  {
> > @@ -78,6 +79,31 @@ UuidInfo *qmp_query_uuid(Error **errp)
> >      return info;
> >  }
> >  
> > +ConfigInfo *qmp_query_config(const char *name, Error **errp)
> > +{
> > +    ConfigInfo *info = g_malloc0(sizeof(*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 (!strcmp(name, optionstr[i])) {
> > +            info->config = g_strdup(configstr[i]);
> > +            break;
> > +        }
> > +    }
> > +
> > +    return info;
> > +}
> > +
> >  void qmp_quit(Error **err)
> >  {
> >      no_shutdown = 0;