[Qemu-devel] [PULL] qemu-ga fixes for 1.1

[Qemu-devel] [PULL] qemu-ga fixes for 1.1

[Michael Roth]

The following changes since commit 76ee152a86d5f2533443ce4d2be6fe253cfb3c45:

  Update version to 1.1.0-rc2 (2012-05-14 17:56:50 -0500)

are available in the git repository at:
  git://github.com/mdroth/qemu.git qga-pull-5-15-12

The bulk of these patches finish out the transition to making
guest-suspend*/guest-shutdown forgo sending a response on success, and making
them synchronous on the guest side such that errors can be be reported back to
the host.

There are also a mix of odd fixes and a version bump for qemu-ga.

Luiz Capitulino (11):
      qapi: add support for command options
      qemu-ga: don't warn on no command return
      qemu-ga: guest-shutdown: don't emit a success response
      qemu-ga: guest-suspend-disk: don't emit a success response
      qemu-ga: guest-suspend-ram: don't emit a success response
      qemu-ga: guest-suspend-hybrid: don't emit a success response
      qemu-ga: make reopen_fd_to_null() public
      qemu-ga: become_daemon(): reopen standard fds to /dev/null
      qemu-ga: guest-suspend: make the API synchronous
      qemu-ga: guest-shutdown: become synchronous
      qemu-ga: guest-shutdown: use only async-signal-safe functions

Michael Roth (2):
      qemu-ga: fix segv after failure to open log file
      qemu-ga: align versioning with QEMU_VERSION

 qapi-schema-guest.json   |   59 +++++++++------
 qapi/qmp-core.h          |   10 ++-
 qapi/qmp-dispatch.c      |    8 ++-
 qapi/qmp-registry.c      |    4 +-
 qemu-ga.c                |   49 ++++++------
 qga/commands-posix.c     |  185 +++++++++++++++++++---------------------------
 qga/commands.c           |    2 +-
 qga/guest-agent-core.h   |    5 +-
 scripts/qapi-commands.py |   14 +++-
 9 files changed, 174 insertions(+), 162 deletions(-)

[Qemu-devel] [PATCH 01/13] qapi: add support for command options

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.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.4.1

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

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 680997e..cf61cb9 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.4.1

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

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 692b570..1dd3454 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.4.1

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

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 1dd3454..88b165c 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.4.1

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

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 88b165c..50e2815 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.4.1

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

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 50e2815..7c6cb21 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.4.1

[Qemu-devel] [PATCH 07/13] qemu-ga: make reopen_fd_to_null() public

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

The next commit wants to use it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c              |   17 +++++++++++++++++
 qga/commands-posix.c   |   19 -------------------
 qga/guest-agent-core.h |    4 ++++
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index cf61cb9..8d53e04 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -140,6 +140,23 @@ static gboolean register_signal_handlers(void)
 
     return true;
 }
+
+/* TODO: use this in place of all post-fork() fclose(std*) callers */
+void reopen_fd_to_null(int fd)
+{
+    int nullfd;
+
+    nullfd = open("/dev/null", O_RDWR);
+    if (nullfd < 0) {
+        return;
+    }
+
+    dup2(nullfd, fd);
+
+    if (nullfd != fd) {
+        close(nullfd);
+    }
+}
 #endif
 
 static void usage(const char *cmd)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e448431..adb9b3d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -34,25 +34,6 @@
 #endif
 #endif
 
-#if defined(__linux__)
-/* TODO: use this in place of all post-fork() fclose(std*) callers */
-static void reopen_fd_to_null(int fd)
-{
-    int nullfd;
-
-    nullfd = open("/dev/null", O_RDWR);
-    if (nullfd < 0) {
-        return;
-    }
-
-    dup2(nullfd, fd);
-
-    if (nullfd != fd) {
-        close(nullfd);
-    }
-}
-#endif /* defined(__linux__) */
-
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     int ret;
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index bbb8b9b..6dba104 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -35,3 +35,7 @@ void ga_set_response_delimited(GAState *s);
 bool ga_is_frozen(GAState *s);
 void ga_set_frozen(GAState *s);
 void ga_unset_frozen(GAState *s);
+
+#ifndef _WIN32
+void reopen_fd_to_null(int fd);
+#endif
-- 
1.7.4.1

[Qemu-devel] [PATCH 08/13] qemu-ga: become_daemon(): reopen standard fds to /dev/null

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

This fixes a bug where qemu-ga doesn't suspend the guest because it
fails to detect suspend support even when the guest does support
suspend. This happens because of the way qemu-ga fds are managed in
daemon mode.

When starting qemu-ga with --daemon, become_daemon() will close all
standard fds. This will cause qemu-ga to end up with the following
fds (if started with 'qemu-ga --daemon'):

    0 -> /dev/vport0p1
    3 -> /run/qemu-ga.pid

Then a guest-suspend-* function is issued. They call bios_supports_mode(),
which will call pipe(), and qemu-ga's fd will be:

    0 -> /dev/vport0p1
    1 -> pipe:[16247]
    2 -> pipe:[16247]
    3 -> /run/qemu-ga.pid

bios_supports_mode() forks off a child and blocks waiting for the child
to write something to the pipe. The child, however, closes its reading
end of the pipe _and_ reopen all standard fds to /dev/null. This will
cause the child's fds to be:

    0 -> /dev/null
    1 -> /dev/null
    2 -> /dev/null
    3 -> /run/qemu-ga.pid

In other words, the child's writing end of the pipe is now /dev/null.
It writes there and exits. The parent process (blocked on read()) will
get an EOF and interpret this as "something unexpected happened in
the child, let's assume the guest doesn't support suspend". And suspend
will fail.

To solve this problem we have to reopen standard fds to /dev/null
in become_daemon(), instead of closing them.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 8d53e04..6e7caed 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -445,9 +445,9 @@ static void become_daemon(const char *pidfile)
         goto fail;
     }
 
-    close(STDIN_FILENO);
-    close(STDOUT_FILENO);
-    close(STDERR_FILENO);
+    reopen_fd_to_null(STDIN_FILENO);
+    reopen_fd_to_null(STDOUT_FILENO);
+    reopen_fd_to_null(STDERR_FILENO);
     return;
 
 fail:
-- 
1.7.4.1

[Qemu-devel] [PATCH 09/13] qemu-ga: guest-suspend: make the API synchronous

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

Currently, qemu-ga has a SIGCHLD handler that automatically reaps terminated
children processes. The idea is to avoid having qemu-ga commands blocked
waiting for children to terminate.

That approach has two problems:

 1. qemu-ga is unable to detect errors in the child, meaning that qemu-ga
    returns success even if the child fails to perform its task

 2. if a command does depend on the child exit status, the command has to
    play tricks to bypass the automatic reaper

Case 2 impacts the guest-suspend-* API, because it has to execute an external
program to check for suspend support. Today, to bypass the automatic reaper,
suspend code has to double fork and pass exit status information through a
pipe. Besides being complex, this is prone to race condition bugs. Indeed,
the current code does have such bugs.

Making the guest-suspend-* API synchronous (ie. by dropping the SIGCHLD
handler and calling waitpid() from commands) is a much simpler approach,
which fixes current race conditions bugs and enables commands to detect
errors in the child.

This commit does just that. There's a side effect though, guest-shutdown
will generate zombies if shutting down fails. This will be fixed by the
next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c            |   17 +------
 qga/commands-posix.c |  128 +++++++++++++++++++++-----------------------------
 2 files changed, 55 insertions(+), 90 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 6e7caed..3a88333 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -104,16 +104,9 @@ static void quit_handler(int sig)
 }
 
 #ifndef _WIN32
-/* reap _all_ terminated children */
-static void child_handler(int sig)
-{
-    int status;
-    while (waitpid(-1, &status, WNOHANG) > 0) /* NOTHING */;
-}
-
 static gboolean register_signal_handlers(void)
 {
-    struct sigaction sigact, sigact_chld;
+    struct sigaction sigact;
     int ret;
 
     memset(&sigact, 0, sizeof(struct sigaction));
@@ -130,14 +123,6 @@ static gboolean register_signal_handlers(void)
         return false;
     }
 
-    memset(&sigact_chld, 0, sizeof(struct sigaction));
-    sigact_chld.sa_handler = child_handler;
-    sigact_chld.sa_flags = SA_NOCLDSTOP;
-    ret = sigaction(SIGCHLD, &sigact_chld, NULL);
-    if (ret == -1) {
-        g_error("error configuring signal handler: %s", strerror(errno));
-    }
-
     return true;
 }
 
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index adb9b3d..76c8235 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -512,117 +512,88 @@ static void guest_fsfreeze_cleanup(void)
 #define SUSPEND_SUPPORTED 0
 #define SUSPEND_NOT_SUPPORTED 1
 
-/**
- * This function forks twice and the information about the mode support
- * status is passed to the qemu-ga process via a pipe.
- *
- * This approach allows us to keep the way we reap terminated children
- * in qemu-ga quite simple.
- */
 static void bios_supports_mode(const char *pmutils_bin, const char *pmutils_arg,
                                const char *sysfile_str, Error **err)
 {
-    pid_t pid;
-    ssize_t ret;
     char *pmutils_path;
-    int status, pipefds[2];
-
-    if (pipe(pipefds) < 0) {
-        error_set(err, QERR_UNDEFINED_ERROR);
-        return;
-    }
+    pid_t pid, rpid;
+    int status;
 
     pmutils_path = g_find_program_in_path(pmutils_bin);
 
     pid = fork();
     if (!pid) {
-        struct sigaction act;
-
-        memset(&act, 0, sizeof(act));
-        act.sa_handler = SIG_DFL;
-        sigaction(SIGCHLD, &act, NULL);
+        char buf[32]; /* hopefully big enough */
+        ssize_t ret;
+        int fd;
 
         setsid();
-        close(pipefds[0]);
         reopen_fd_to_null(0);
         reopen_fd_to_null(1);
         reopen_fd_to_null(2);
 
-        pid = fork();
-        if (!pid) {
-            int fd;
-            char buf[32]; /* hopefully big enough */
-
-            if (pmutils_path) {
-                execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
-            }
-
-            /*
-             * If we get here either pm-utils is not installed or execle() has
-             * failed. Let's try the manual method if the caller wants it.
-             */
-
-            if (!sysfile_str) {
-                _exit(SUSPEND_NOT_SUPPORTED);
-            }
-
-            fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
-            if (fd < 0) {
-                _exit(SUSPEND_NOT_SUPPORTED);
-            }
+        if (pmutils_path) {
+            execle(pmutils_path, pmutils_bin, pmutils_arg, NULL, environ);
+        }
 
-            ret = read(fd, buf, sizeof(buf)-1);
-            if (ret <= 0) {
-                _exit(SUSPEND_NOT_SUPPORTED);
-            }
-            buf[ret] = '\0';
+        /*
+         * If we get here either pm-utils is not installed or execle() has
+         * failed. Let's try the manual method if the caller wants it.
+         */
 
-            if (strstr(buf, sysfile_str)) {
-                _exit(SUSPEND_SUPPORTED);
-            }
+        if (!sysfile_str) {
+            _exit(SUSPEND_NOT_SUPPORTED);
+        }
 
+        fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
+        if (fd < 0) {
             _exit(SUSPEND_NOT_SUPPORTED);
         }
 
-        if (pid > 0) {
-            wait(&status);
-        } else {
-            status = SUSPEND_NOT_SUPPORTED;
+        ret = read(fd, buf, sizeof(buf)-1);
+        if (ret <= 0) {
+            _exit(SUSPEND_NOT_SUPPORTED);
         }
+        buf[ret] = '\0';
 
-        ret = write(pipefds[1], &status, sizeof(status));
-        if (ret != sizeof(status)) {
-            _exit(EXIT_FAILURE);
+        if (strstr(buf, sysfile_str)) {
+            _exit(SUSPEND_SUPPORTED);
         }
 
-        _exit(EXIT_SUCCESS);
+        _exit(SUSPEND_NOT_SUPPORTED);
     }
 
-    close(pipefds[1]);
     g_free(pmutils_path);
 
     if (pid < 0) {
-        error_set(err, QERR_UNDEFINED_ERROR);
-        goto out;
-    }
-
-    ret = read(pipefds[0], &status, sizeof(status));
-    if (ret == sizeof(status) && WIFEXITED(status) &&
-        WEXITSTATUS(status) == SUSPEND_SUPPORTED) {
-            goto out;
+        goto undef_err;
+    }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status)) {
+        switch (WEXITSTATUS(status)) {
+        case SUSPEND_SUPPORTED:
+            return;
+        case SUSPEND_NOT_SUPPORTED:
+            error_set(err, QERR_UNSUPPORTED);
+            return;
+        default:
+            goto undef_err;
+        }
     }
 
-    error_set(err, QERR_UNSUPPORTED);
-
-out:
-    close(pipefds[0]);
+undef_err:
+    error_set(err, QERR_UNDEFINED_ERROR);
 }
 
 static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
                           Error **err)
 {
-    pid_t pid;
     char *pmutils_path;
+    pid_t rpid, pid;
+    int status;
 
     pmutils_path = g_find_program_in_path(pmutils_bin);
 
@@ -664,9 +635,18 @@ static void guest_suspend(const char *pmutils_bin, const char *sysfile_str,
     g_free(pmutils_path);
 
     if (pid < 0) {
-        error_set(err, QERR_UNDEFINED_ERROR);
+        goto exit_err;
+    }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
         return;
     }
+
+exit_err:
+    error_set(err, QERR_UNDEFINED_ERROR);
 }
 
 void qmp_guest_suspend_disk(Error **err)
-- 
1.7.4.1

[Qemu-devel] [PATCH 10/13] qemu-ga: guest-shutdown: become synchronous

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

Last commit dropped qemu-ga's SIGCHLD handler, used to automatically
reap terminated children processes. This introduced a bug to
qmp_guest_shutdown(): it will generate zombies.

This problem probably doesn't matter in the success case, as the VM
will shutdown anyway, but let's do the right thing and reap the
created process. This ultimately means that guest-shutdown is now a
synchronous command.

An interesting side effect is that guest-shutdown is now able to
report an error to the client if shutting down fails.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 76c8235..68da71f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -36,8 +36,9 @@
 
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
-    int ret;
     const char *shutdown_flag;
+    int ret, status;
+    pid_t rpid, pid;
 
     slog("guest-shutdown called, mode: %s", mode);
     if (!has_mode || strcmp(mode, "powerdown") == 0) {
@@ -52,8 +53,8 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
         return;
     }
 
-    ret = fork();
-    if (ret == 0) {
+    pid = fork();
+    if (pid == 0) {
         /* child, start the shutdown */
         setsid();
         fclose(stdin);
@@ -66,9 +67,19 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
             slog("guest-shutdown failed: %s", strerror(errno));
         }
         exit(!!ret);
-    } else if (ret < 0) {
-        error_set(err, QERR_UNDEFINED_ERROR);
+    } else if (pid < 0) {
+        goto exit_err;
     }
+
+    do {
+        rpid = waitpid(pid, &status, 0);
+    } while (rpid == -1 && errno == EINTR);
+    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
+        return;
+    }
+
+exit_err:
+    error_set(err, QERR_UNDEFINED_ERROR);
 }
 
 typedef struct GuestFileHandle {
-- 
1.7.4.1

[Qemu-devel] [PATCH 11/13] qemu-ga: guest-shutdown: use only async-signal-safe functions

[Michael Roth]

From: Luiz Capitulino <lcapitulino@redhat.com>

POSIX mandates[1] that a child process of a multi-thread program uses
only async-signal-safe functions before exec(). We consider qemu-ga
to be multi-thread, because it uses glib.

However, qmp_guest_shutdown() uses functions that are not
async-signal-safe. Fix it the following way:

- fclose() -> reopen_fd_to_null()
- execl() -> execle()
- exit() -> _exit()
- drop slog() usage (which is not safe)

  [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json |    3 +--
 qga/commands-posix.c   |   17 +++++++----------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 7c6cb21..d4055d2 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -126,8 +126,7 @@
 # @guest-shutdown:
 #
 # Initiate guest-activated shutdown. Note: this is an asynchronous
-# shutdown request, with no guaruntee of successful shutdown. Errors
-# will be logged to guest's syslog.
+# shutdown request, with no guarantee of successful shutdown.
 #
 # @mode: #optional "halt", "powerdown" (default), or "reboot"
 #
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 68da71f..7664be1 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -37,8 +37,8 @@
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     const char *shutdown_flag;
-    int ret, status;
     pid_t rpid, pid;
+    int status;
 
     slog("guest-shutdown called, mode: %s", mode);
     if (!has_mode || strcmp(mode, "powerdown") == 0) {
@@ -57,16 +57,13 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
     if (pid == 0) {
         /* child, start the shutdown */
         setsid();
-        fclose(stdin);
-        fclose(stdout);
-        fclose(stderr);
-
-        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
-                    "hypervisor initiated shutdown", (char*)NULL);
-        if (ret) {
-            slog("guest-shutdown failed: %s", strerror(errno));
-        }
-        exit(!!ret);
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execle("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
+               "hypervisor initiated shutdown", (char*)NULL, environ);
+        _exit(EXIT_FAILURE);
     } else if (pid < 0) {
         goto exit_err;
     }
-- 
1.7.4.1

[Qemu-devel] [PATCH 12/13] qemu-ga: fix segv after failure to open log file

[Michael Roth]

Currently, if we fail to open the specified log file (generally due to a
permissions issue), we'll assign NULL to the logfile handle (stderr,
initially) used by the logging routines, which can cause a segfault to
occur when we attempt to report the error before exiting.

Instead, only re-assign if the open() was successful.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 3a88333..42e271f 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -836,12 +836,13 @@ int main(int argc, char **argv)
             become_daemon(pid_filepath);
         }
         if (log_filepath) {
-            s->log_file = fopen(log_filepath, "a");
-            if (!s->log_file) {
+            FILE *log_file = fopen(log_filepath, "a");
+            if (!log_file) {
                 g_critical("unable to open specified log file: %s",
                            strerror(errno));
                 goto out_bad;
             }
+            s->log_file = log_file;
         }
     }
 
-- 
1.7.4.1

[Qemu-devel] [PATCH 13/13] qemu-ga: align versioning with QEMU_VERSION

[Michael Roth]

Previously qemu-ga version was defined seperately. Since it is aligned
with QEMU releases, use QEMU_VERSION instead. This also implies the
version bump for 1.1[-rcN] release of qemu-ga.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-ga.c              |    4 ++--
 qga/commands.c         |    2 +-
 qga/guest-agent-core.h |    1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 42e271f..8199da7 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -169,7 +169,7 @@ static void usage(const char *cmd)
 "  -h, --help        display this help and exit\n"
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
-    , cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
+    , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
     QGA_STATEDIR_DEFAULT);
 }
 
@@ -729,7 +729,7 @@ int main(int argc, char **argv)
             log_level = G_LOG_LEVEL_MASK;
             break;
         case 'V':
-            printf("QEMU Guest Agent %s\n", QGA_VERSION);
+            printf("QEMU Guest Agent %s\n", QEMU_VERSION);
             return 0;
         case 'd':
             daemonize = 1;
diff --git a/qga/commands.c b/qga/commands.c
index 5bcceaa..46b0b08 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -52,7 +52,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
     GuestAgentCommandInfoList *cmd_info_list;
     char **cmd_list_head, **cmd_list;
 
-    info->version = g_strdup(QGA_VERSION);
+    info->version = g_strdup(QEMU_VERSION);
 
     cmd_list_head = cmd_list = qmp_get_command_list();
     if (*cmd_list_head == NULL) {
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index 6dba104..49a7abe 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -13,7 +13,6 @@
 #include "qapi/qmp-core.h"
 #include "qemu-common.h"
 
-#define QGA_VERSION "1.0"
 #define QGA_READ_COUNT_DEFAULT 4096
 
 typedef struct GAState GAState;
-- 
1.7.4.1

Re: [Qemu-devel] [PULL] qemu-ga fixes for 1.1

[Luiz Capitulino]

On Tue, 15 May 2012 09:48:46 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> The following changes since commit 76ee152a86d5f2533443ce4d2be6fe253cfb3c45:
> 
>   Update version to 1.1.0-rc2 (2012-05-14 17:56:50 -0500)
> 
> are available in the git repository at:
>   git://github.com/mdroth/qemu.git qga-pull-5-15-12
> 
> The bulk of these patches finish out the transition to making
> guest-suspend*/guest-shutdown forgo sending a response on success, and making
> them synchronous on the guest side such that errors can be be reported back to
> the host.

ping?