[Qemu-devel] [PATCH 0/6]: qemu-ga: no success response for certain commands

[Qemu-devel] [PATCH 0/6]: qemu-ga: no success response for certain commands

[Luiz Capitulino]

This series changes qemu-ga to not emit a success response for commands
guest-shutdown and guest-suspend-{ram,disk,hybrid}. More details and the
reason for this change can be found in the following patches.

Also note that this series depends on this series sent by me previously:

    http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00584.html

 qapi-schema-guest.json   |   50 +++++++++++++++++++++++++++-------------------
 qapi/qmp-core.h          |   10 +++++++++-
 qapi/qmp-dispatch.c      |    8 ++++++--
 qapi/qmp-registry.c      |    4 +++-
 qemu-ga.c                |    2 --
 scripts/qapi-commands.py |   14 +++++++++++--
 6 files changed, 60 insertions(+), 28 deletions(-)

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

[Luiz Capitulino]

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

[Qemu-devel] [PATCH 2/6] qemu-ga: don't warn on no command return

[Luiz Capitulino]

This is a valid condition when a command chooses to not emit a
success response.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-ga.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 216be39..3547119 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -488,8 +488,6 @@ static void process_command(GAState *s, QDict *req)
             g_warning("error sending response: %s", strerror(ret));
         }
         qobject_decref(rsp);
-    } else {
-        g_warning("error getting response");
     }
 }
 
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 3/6] qemu-ga: guest-shutdown: don't emit a success response

[Luiz Capitulino]

Today, qemu-ga may not be able to emit a success response when
guest-shutdown completes. This happens because the VM may vanish
before qemu-ga is able to emit a response.

This semantic is a bit confusing, as it's not clear for clients if
they should wait for a response or how they should check for success.

This commit solves that problem by changing guest-shutdown to never
emit a success response and suggests in the documentation what
clients could do to check for success.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema-guest.json |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index d7a073e..0aa9f91 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -131,11 +131,13 @@
 #
 # @mode: #optional "halt", "powerdown" (default), or "reboot"
 #
-# Returns: Nothing on success
+# Returns: This command does NOT return a response on success. Success
+# condition is indicated by the VM exiting with a zero exit status.
 #
 # Since: 0.15.0
 ##
-{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
+{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' },
+  'success-response': 'no' }
 
 ##
 # @guest-file-open:
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 4/6] qemu-ga: guest-suspend-disk: don't emit a success response

[Luiz Capitulino]

Today, qemu-ga may not be able to emit a success response when
guest-suspend-disk completes. This happens because the VM may
vanish before qemu-ga is able to emit a response.

This semantic is a bit confusing, as it's not clear for clients if
they should wait for a response or how they should check for success.

This commit solves that problem by changing guest-suspend-disk to
never emit a success response and suggests in the documentation
what clients could do to check for success.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema-guest.json |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 0aa9f91..8ea7805 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -361,17 +361,19 @@
 # For the best results it's strongly recommended to have the pm-utils
 # package installed in the guest.
 #
-# Returns: nothing on success
+# Returns: This command does NOT return a response on success. There's a
+# high chance the command succeeded if the VM exits with a zero exit status,
+# but the VM could also exit due to other reasons.
+#
+# The following errors may be returned:
 #          If suspend to disk is not supported, Unsupported
 #
-# Notes: o This is an asynchronous request. There's no guarantee a response
-#          will be sent
-#        o It's strongly recommended to issue the guest-sync command before
-#          sending commands when the guest resumes
+# Notes: It's strongly recommended to issue the guest-sync command before
+#        sending commands when the guest resumes
 #
 # Since: 1.1
 ##
-{ 'command': 'guest-suspend-disk' }
+{ 'command': 'guest-suspend-disk', 'success-response': 'no' }
 
 ##
 # @guest-suspend-ram
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 5/6] qemu-ga: guest-suspend-ram: don't emit a success response

[Luiz Capitulino]

Today, qemu-ga may not be able to emit a success response when
guest-suspend-ram completes. This happens because the VM may
suspend before qemu-ga is able to emit a response.

This semantic is a bit confusing, as it's not clear for clients if
they should wait for a response or how they should check for success.

This commit solves that problem by changing guest-suspend-ram to
never emit a success response and suggests in the documentation
what clients should do to check for success.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema-guest.json |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 8ea7805..f5f6f26 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -391,17 +391,20 @@
 # command.  Thus, it's *required* to query QEMU for the presence of the
 # 'system_wakeup' command before issuing guest-suspend-ram.
 #
-# Returns: nothing on success
+# Returns: This command does NOT return a response on success. There are
+# two options to check for success:
+#   1. Wait for the SUSPEND QMP event from QEMU
+#   2. Check for the suspended RunState in QEMU, by issuing query-status
+#
+# The following errors may be returned:
 #          If suspend to ram is not supported, Unsupported
 #
-# Notes: o This is an asynchronous request. There's no guarantee a response
-#          will be sent
-#        o It's strongly recommended to issue the guest-sync command before
-#          sending commands when the guest resumes
+# Notes: It's strongly recommended to issue the guest-sync command before
+#        sending commands when the guest resumes
 #
 # Since: 1.1
 ##
-{ 'command': 'guest-suspend-ram' }
+{ 'command': 'guest-suspend-ram', 'success-response': 'no' }
 
 ##
 # @guest-suspend-hybrid
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 6/6] qemu-ga: guest-suspend-hybrid: don't emit a success response

[Luiz Capitulino]

Today, qemu-ga may not be able to emit a success response when
guest-suspend-hybrid completes. This happens because the VM may
suspend before qemu-ga is able to emit a response.

This semantic is a bit confusing, as it's not clear for clients if
they should wait for a response or how they should check for success.

This commit solves that problem by changing guest-suspend-hybrid to
never emit a success response and suggests in the documentation
what clients should do to check for success.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema-guest.json |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index f5f6f26..b3ccd66 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -417,17 +417,20 @@
 # command.  Thus, it's *required* to query QEMU for the presence of the
 # 'system_wakeup' command before issuing guest-suspend-hybrid.
 #
-# Returns: nothing on success
+# Returns: This command does NOT return a response on success. There are
+# two options to check for success:
+#   1. Wait for the SUSPEND QMP event from QEMU
+#   2. Check for the suspended RunState in QEMU, by issuing query-status
+#
+# The following errors may be returned:
 #          If hybrid suspend is not supported, Unsupported
 #
-# Notes: o This is an asynchronous request. There's no guarantee a response
-#          will be sent
-#        o It's strongly recommended to issue the guest-sync command before
-#          sending commands when the guest resumes
+# Notes: It's strongly recommended to issue the guest-sync command before
+#        sending commands when the guest resumes
 #
 # Since: 1.1
 ##
-{ 'command': 'guest-suspend-hybrid' }
+{ 'command': 'guest-suspend-hybrid', 'success-response': 'no' }
 
 ##
 # @GuestIpAddressType:
-- 
1.7.9.2.384.g4a92a

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

[Michael Roth]

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
> 

Re: [Qemu-devel] [PATCH 2/6] qemu-ga: don't warn on no command return

[Michael Roth]

On Fri, May 04, 2012 at 05:20:18PM -0300, Luiz Capitulino wrote:
> This is a valid condition when a command chooses to not emit a
> success response.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

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

> ---
>  qemu-ga.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 216be39..3547119 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -488,8 +488,6 @@ static void process_command(GAState *s, QDict *req)
>              g_warning("error sending response: %s", strerror(ret));
>          }
>          qobject_decref(rsp);
> -    } else {
> -        g_warning("error getting response");
>      }
>  }
> 
> -- 
> 1.7.9.2.384.g4a92a
> 

Re: [Qemu-devel] [PATCH 3/6] qemu-ga: guest-shutdown: don't emit a success response

[Michael Roth]

On Fri, May 04, 2012 at 05:20:19PM -0300, Luiz Capitulino wrote:
> Today, qemu-ga may not be able to emit a success response when
> guest-shutdown completes. This happens because the VM may vanish
> before qemu-ga is able to emit a response.
> 
> This semantic is a bit confusing, as it's not clear for clients if
> they should wait for a response or how they should check for success.
> 
> This commit solves that problem by changing guest-shutdown to never
> emit a success response and suggests in the documentation what
> clients could do to check for success.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qapi-schema-guest.json |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index d7a073e..0aa9f91 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -131,11 +131,13 @@
>  #
>  # @mode: #optional "halt", "powerdown" (default), or "reboot"
>  #
> -# Returns: Nothing on success
> +# Returns: This command does NOT return a response on success. Success
> +# condition is indicated by the VM exiting with a zero exit status.

Would also include something like:

'or, when running with --no-shutdown, by running the query-status QMP
command to confirm the status/RunState is "shutdown"'

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

>  #
>  # Since: 0.15.0
>  ##
> -{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
> +{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' },
> +  'success-response': 'no' }
> 
>  ##
>  # @guest-file-open:
> -- 
> 1.7.9.2.384.g4a92a
> 

Re: [Qemu-devel] [PATCH 4/6] qemu-ga: guest-suspend-disk: don't emit a success response

[Michael Roth]

On Fri, May 04, 2012 at 05:20:20PM -0300, Luiz Capitulino wrote:
> Today, qemu-ga may not be able to emit a success response when
> guest-suspend-disk completes. This happens because the VM may
> vanish before qemu-ga is able to emit a response.
> 
> This semantic is a bit confusing, as it's not clear for clients if
> they should wait for a response or how they should check for success.
> 
> This commit solves that problem by changing guest-suspend-disk to
> never emit a success response and suggests in the documentation
> what clients could do to check for success.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qapi-schema-guest.json |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 0aa9f91..8ea7805 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -361,17 +361,19 @@
>  # For the best results it's strongly recommended to have the pm-utils
>  # package installed in the guest.
>  #
> -# Returns: nothing on success
> +# Returns: This command does NOT return a response on success. There's a
> +# high chance the command succeeded if the VM exits with a zero exit status,
> +# but the VM could also exit due to other reasons.

Similar suggestion as previous patch (adding a note about query-status)

> +#
> +# The following errors may be returned:
>  #          If suspend to disk is not supported, Unsupported
>  #
> -# Notes: o This is an asynchronous request. There's no guarantee a response
> -#          will be sent
> -#        o It's strongly recommended to issue the guest-sync command before
> -#          sending commands when the guest resumes
> +# Notes: It's strongly recommended to issue the guest-sync command before
> +#        sending commands when the guest resumes
>  #
>  # Since: 1.1
>  ##
> -{ 'command': 'guest-suspend-disk' }
> +{ 'command': 'guest-suspend-disk', 'success-response': 'no' }
> 
>  ##
>  # @guest-suspend-ram
> -- 
> 1.7.9.2.384.g4a92a
> 

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

[Luiz Capitulino]

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.

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

[Luiz Capitulino]

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..9eed40e 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -291,14 +291,24 @@ out:
 
     return ret
 
+def option_value_matches(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_value_matches('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)
-- 
1.7.9.2.384.g4a92a