Re: [Qemu-devel] [PATCH 1/6] qapi: add support for command options

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Fri, May 04, 2012 at 05:20:17PM -0300, Luiz Capitulino wrote:
> Options allow for changes in commands behavior. This commit introduces
> the QCO_NO_SUCCESS_RESP option, which causes a command to not emit a
> success response.
> 
> This is needed by commands such as qemu-ga's guest-shutdown, which
> may not be able to complete before the VM vanishes. In this case, it's
> useful and simpler not to bother sending a success response.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qapi/qmp-core.h          |   10 +++++++++-
>  qapi/qmp-dispatch.c      |    8 ++++++--
>  qapi/qmp-registry.c      |    4 +++-
>  scripts/qapi-commands.py |   14 ++++++++++++--
>  4 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
> index 431ddbb..b0f64ba 100644
> --- a/qapi/qmp-core.h
> +++ b/qapi/qmp-core.h
> @@ -25,16 +25,24 @@ typedef enum QmpCommandType
>      QCT_NORMAL,
>  } QmpCommandType;
> 
> +typedef enum QmpCommandOptions
> +{
> +    QCO_NO_OPTIONS = 0x0,
> +    QCO_NO_SUCCESS_RESP = 0x1,
> +} QmpCommandOptions;
> +
>  typedef struct QmpCommand
>  {
>      const char *name;
>      QmpCommandType type;
>      QmpCommandFunc *fn;
> +    QmpCommandOptions options;
>      QTAILQ_ENTRY(QmpCommand) node;
>      bool enabled;
>  } QmpCommand;
> 
> -void qmp_register_command(const char *name, QmpCommandFunc *fn);
> +void qmp_register_command(const char *name, QmpCommandFunc *fn,
> +                          QmpCommandOptions options);
>  QmpCommand *qmp_find_command(const char *name);
>  QObject *qmp_dispatch(QObject *request);
>  void qmp_disable_command(const char *name);
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 43f640a..122c1a2 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -94,8 +94,12 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
>      switch (cmd->type) {
>      case QCT_NORMAL:
>          cmd->fn(args, &ret, errp);
> -        if (!error_is_set(errp) && ret == NULL) {
> -            ret = QOBJECT(qdict_new());
> +        if (!error_is_set(errp)) {
> +            if (cmd->options & QCO_NO_SUCCESS_RESP) {
> +                g_assert(!ret);
> +            } else if (!ret) {
> +                ret = QOBJECT(qdict_new());
> +            }
>          }
>          break;
>      }
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index 43d5cde..5414613 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -17,7 +17,8 @@
>  static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands =
>      QTAILQ_HEAD_INITIALIZER(qmp_commands);
> 
> -void qmp_register_command(const char *name, QmpCommandFunc *fn)
> +void qmp_register_command(const char *name, QmpCommandFunc *fn,
> +                          QmpCommandOptions options)
>  {
>      QmpCommand *cmd = g_malloc0(sizeof(*cmd));
> 
> @@ -25,6 +26,7 @@ void qmp_register_command(const char *name, QmpCommandFunc *fn)
>      cmd->type = QCT_NORMAL;
>      cmd->fn = fn;
>      cmd->enabled = true;
> +    cmd->options = options;
>      QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node);
>  }
> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 0b4f0a0..e746333 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -291,14 +291,24 @@ out:
> 
>      return ret
> 
> +def option_is_enabled(opt, val, cmd):
> +    if opt in cmd and cmd[opt] == val:
> +        return True
> +    return False
> +
>  def gen_registry(commands):
>      registry=""
>      push_indent()
>      for cmd in commands:
> +        options = 'QCO_NO_OPTIONS'
> +        if option_is_enabled('success-response', 'no', cmd):
> +            options = 'QCO_NO_SUCCESS_RESP'
> +

Hate to hold things up for a nit, but the naming of option_is_enabled()
is just plain wrong here. option_is_enabled() is returning true for a
case where we're actually checking for an option being disabled. I'm
guessing it looks this way because we're trying to determine if the
internal QCO_NO_SUCCESS_RESP option is enabled, but option_is_enabled()
only has knowledge of the QAPI directive so I think that's backwards.

option_value_matches() would indicate the purpose better, or
option_is_disabled() and then moving the "no"/"false"/"disabled"
matching into it.

Patch looks good otherwise.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

>          registry += mcgen('''
> -qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s);
> +qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s);
>  ''',
> -                     name=cmd['command'], c_name=c_fun(cmd['command']))
> +                     name=cmd['command'], c_name=c_fun(cmd['command']),
> +                     opts=options)
>      pop_indent()
>      ret = mcgen('''
>  static void qmp_init_marshal(void)
> -- 
> 1.7.9.2.384.g4a92a
>