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

[Qemu-devel] [PATCH v2 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.

V2

o Rename option_is_enabled() to option_value_matches() [Michael]
o Improve guest-shutdown and guest-suspend-* docs [Michael]

 qapi-schema-guest.json   |   56 +++++++++++++++++++++++++++++-----------------
 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, 66 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..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

[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 |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index d7a073e..82f901a 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -131,11 +131,15 @@
 #
 # @mode: #optional "halt", "powerdown" (default), or "reboot"
 #
-# Returns: Nothing on success
+# This command does NOT return a response on success. Success condition
+# is indicated by the VM exiting with a zero exit status or, when
+# running with --no-shutdown, by issuing the query-status QMP command
+# to confirm the VM status is "shutdown".
 #
 # 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 |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 82f901a..64db215 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -363,17 +363,21 @@
 # For the best results it's strongly recommended to have the pm-utils
 # package installed in the guest.
 #
-# Returns: nothing on success
+# This command does NOT return a response on success. There is a high chance
+# the command succeeded if the VM exits with a zero exit status or, when
+# running with --no-shutdown, by issuing the query-status QMP command to
+# to confirm the VM status is "shutdown". However, the VM could also exit
+# (or set its status to "shutdown") 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 |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 64db215..c0f46ba 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -395,17 +395,21 @@
 # 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
+# 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. Issue the query-status QMP command to confirm the VM status is
+#      "suspended"
+#
+# 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 |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index c0f46ba..1c949ff 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -422,17 +422,21 @@
 # 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
+# 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. Issue the query-status QMP command to confirm the VM status is
+#      "suspended"
+#
+# 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 v2 0/6]: qemu-ga: no success response for certain commands

[Michael Roth]

On Tue, May 08, 2012 at 02:24:43PM -0300, Luiz Capitulino wrote:
> 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.

Looks good. Tested the affected RPCs and everything seems to work as expected.

Applied to qemu-ga queue.

> 
> V2
> 
> o Rename option_is_enabled() to option_value_matches() [Michael]
> o Improve guest-shutdown and guest-suspend-* docs [Michael]
> 
>  qapi-schema-guest.json   |   56 +++++++++++++++++++++++++++++-----------------
>  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, 66 insertions(+), 28 deletions(-)
> 

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

[Luiz Capitulino]

On Wed, 9 May 2012 15:03:35 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Tue, May 08, 2012 at 02:24:43PM -0300, Luiz Capitulino wrote:
> > 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.
> 
> Looks good. Tested the affected RPCs and everything seems to work as expected.
> 
> Applied to qemu-ga queue.

Thanks, I'm a bit late on the series to make guest-shutdown and guest-suspend-*
synchronous, will try to post it tomorrow.