diff mbox

[1/6] qapi: add support for command options

Message ID 1336162822-13161-2-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino May 4, 2012, 8:20 p.m. UTC 
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(-)

Comments

Michael Roth May 7, 2012, 4:05 p.m. UTC | #1 
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
>
Luiz Capitulino May 8, 2012, 1:11 p.m. UTC | #2 
On Mon, 7 May 2012 11:05:36 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> 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.

Agreed.

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

I like option_value_matches(), will address this and the other comments and
resend.

Btw, I expect this series and my next one (which makes guest-shutdown and
guest-suspend-* synchronous) to go through your tree. Also note that they're
1.1 material.
diff mbox

Patch

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'
+
         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)