[Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Luiz Capitulino]

The main motivation for this series is fixing two possible race conditions
in the guest-suspend-* API due to the complexity that arose from the way
we handle terminated children processes today. Full details in the first
patch.

This series applies on top of my two other qemu-ga series submitted
previously:

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

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

 qapi-schema-guest.json   |   56 ++++++++++------
 qapi/qmp-core.h          |   10 ++-
 qapi/qmp-dispatch.c      |    8 ++-
 qapi/qmp-registry.c      |    4 +-
 qemu-ga.c                |   40 ++++++------
 qga/commands-posix.c     |  162 ++++++++++++++++++----------------------------
 qga/guest-agent-core.h   |    4 ++
 scripts/qapi-commands.py |   14 +++-
 8 files changed, 154 insertions(+), 144 deletions(-)

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

[Luiz Capitulino]

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>
---
 qemu-ga.c            |   17 +------
 qga/commands-posix.c |  124 ++++++++++++++++++++------------------------------
 2 files changed, 51 insertions(+), 90 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 822f527..c0ca8b2 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 2ce9b82..a61f9e0 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -512,117 +512,86 @@ 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;
+    }
+
+    rpid = waitpid(pid, &status, 0);
+    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 +633,16 @@ 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;
+    }
+
+    rpid = waitpid(pid, &status, 0);
+    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.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 1/2] qemu-ga: guest-suspend: make the API synchronous

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On 05/11/2012 01:19 PM, Luiz Capitulino wrote:
> 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.
> 

> +
> +    rpid = waitpid(pid, &status, 0);

You need to worry about signals interrupting the wait.  Something like:

while ((rpid = waitpid(pid, &status, 0) == -1 && errno == EINTR) {
}


> @@ -664,9 +633,16 @@ 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;
> +    }
> +
> +    rpid = waitpid(pid, &status, 0);
> +    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
>          return;

And again.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

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

[Luiz Capitulino]

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>
---
 qga/commands-posix.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index a61f9e0..f1b19af 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,17 @@ 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;
     }
+
+    rpid = waitpid(pid, &status, 0);
+    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {
+        return;
+    }
+
+exit_err:
+    error_set(err, QERR_UNDEFINED_ERROR);
 }
 
 typedef struct GuestFileHandle {
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 2/2] qemu-ga: guest-shutdown: become synchronous

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On 05/11/2012 01:19 PM, Luiz Capitulino wrote:
> 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>
> ---
>  qga/commands-posix.c |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 

>          exit(!!ret);
> -    } else if (ret < 0) {
> -        error_set(err, QERR_UNDEFINED_ERROR);
> +    } else if (pid < 0) {
> +        goto exit_err;
>      }
> +
> +    rpid = waitpid(pid, &status, 0);
> +    if (rpid == pid && WIFEXITED(status) && !WEXITSTATUS(status)) {

And another case where waitpid should be used in a loop.

Other than that, I'm in favor of this series.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Michael Roth]

Hi Luiz,

I have these patches applied to qga branch with Eric's suggested fix-ups included. I've attached them for your review. If all looks good I'll send a PULL some time today with what's hopefully all the pending fixes for qemu-ga 1.1

[Qemu-devel] [PATCH 1/2] 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>
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 822f527..c0ca8b2 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 2ce9b82..cb97646 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

Re: [Qemu-devel] [PATCH 1/2] qemu-ga: guest-suspend: make the API synchronous

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On 05/14/2012 08:56 AM, Michael Roth wrote:
> 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.
> 

> 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>
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

[Qemu-devel] [PATCH 2/2] 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>
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 cb97646..9a59276 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

Re: [Qemu-devel] [PATCH 2/2] qemu-ga: guest-shutdown: become synchronous

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On 05/14/2012 08:56 AM, Michael Roth wrote:
> 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>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Luiz Capitulino]

On Mon, 14 May 2012 09:56:30 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Hi Luiz,
> 
> I have these patches applied to qga branch with Eric's suggested fix-ups included. I've attached them for your review. If all looks good I'll send a PULL some time today with what's hopefully all the pending fixes for qemu-ga 1.1

Yes, looks good. I was finishing to work on a similar version that moves
the loop to a function called ga_waitpid(), it's only missing some testing. But
you can merge this version if you're hurrying.

Btw, I have another fix that fixes guest-shutdown to use async-safe-signal
functions.

What's the best way to go? Do you want me to send just the guest-shutdown
fix or should I post my v2 of this series too (that uses ga_waitpid())?

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Michael Roth]

On Mon, May 14, 2012 at 01:39:50PM -0300, Luiz Capitulino wrote:
> On Mon, 14 May 2012 09:56:30 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Hi Luiz,
> > 
> > I have these patches applied to qga branch with Eric's suggested fix-ups included. I've attached them for your review. If all looks good I'll send a PULL some time today with what's hopefully all the pending fixes for qemu-ga 1.1
> 
> Yes, looks good. I was finishing to work on a similar version that moves
> the loop to a function called ga_waitpid(), it's only missing some testing. But
> you can merge this version if you're hurrying.
> 
> Btw, I have another fix that fixes guest-shutdown to use async-safe-signal
> functions.
> 
> What's the best way to go? Do you want me to send just the guest-shutdown
> fix or should I post my v2 of this series too (that uses ga_waitpid())?
> 

Hmm, since Eric has already reviewed the quick fix let's just save ga_waitpid()
for another day.

Will keep an eye out for guest-shutdown fixes. I have a patch that
switches it to using reopen_to_null() btw, but if that's already part of
your patch I'll drop it in favor of yours:

https://github.com/mdroth/qemu/commits/qga

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Luiz Capitulino]

On Mon, 14 May 2012 12:19:04 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Mon, May 14, 2012 at 01:39:50PM -0300, Luiz Capitulino wrote:
> > On Mon, 14 May 2012 09:56:30 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > Hi Luiz,
> > > 
> > > I have these patches applied to qga branch with Eric's suggested fix-ups included. I've attached them for your review. If all looks good I'll send a PULL some time today with what's hopefully all the pending fixes for qemu-ga 1.1
> > 
> > Yes, looks good. I was finishing to work on a similar version that moves
> > the loop to a function called ga_waitpid(), it's only missing some testing. But
> > you can merge this version if you're hurrying.
> > 
> > Btw, I have another fix that fixes guest-shutdown to use async-safe-signal
> > functions.
> > 
> > What's the best way to go? Do you want me to send just the guest-shutdown
> > fix or should I post my v2 of this series too (that uses ga_waitpid())?
> > 
> 
> Hmm, since Eric has already reviewed the quick fix let's just save ga_waitpid()
> for another day.

OK.

> Will keep an eye out for guest-shutdown fixes. I have a patch that
> switches it to using reopen_to_null() btw, but if that's already part of
> your patch I'll drop it in favor of yours:
> 
> https://github.com/mdroth/qemu/commits/qga

Yes, it's. Besides using reopen_to_null() we also have to switch to
execle() and _exit(). I'll give it a quick test and post it shortly.

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Michal Privoznik]

On 11.05.2012 21:19, Luiz Capitulino wrote:
> The main motivation for this series is fixing two possible race conditions
> in the guest-suspend-* API due to the complexity that arose from the way
> we handle terminated children processes today. Full details in the first
> patch.
> 
> This series applies on top of my two other qemu-ga series submitted
> previously:
> 
>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00999.html
> 
>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01507.html
> 
>  qapi-schema-guest.json   |   56 ++++++++++------
>  qapi/qmp-core.h          |   10 ++-
>  qapi/qmp-dispatch.c      |    8 ++-
>  qapi/qmp-registry.c      |    4 +-
>  qemu-ga.c                |   40 ++++++------
>  qga/commands-posix.c     |  162 ++++++++++++++++++----------------------------
>  qga/guest-agent-core.h   |    4 ++
>  scripts/qapi-commands.py |   14 +++-
>  8 files changed, 154 insertions(+), 144 deletions(-)
> 

Okay, this is definitely an enhancement and fix of bogus implementation.
One thing that I'd like to ask is - how can user distinguish between
these implementations. I am asking basically from libvirt POV.
Because if I assume I am dealing with the previous implementation and
thus waiting for the {'return':{}} before I can return form an API, but
the GA actually uses the new implementation I will block endlessly.

Thanks,

Michal

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Luiz Capitulino]

On Mon, 14 May 2012 19:06:28 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 11.05.2012 21:19, Luiz Capitulino wrote:
> > The main motivation for this series is fixing two possible race conditions
> > in the guest-suspend-* API due to the complexity that arose from the way
> > we handle terminated children processes today. Full details in the first
> > patch.
> > 
> > This series applies on top of my two other qemu-ga series submitted
> > previously:
> > 
> >  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00999.html
> > 
> >  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01507.html
> > 
> >  qapi-schema-guest.json   |   56 ++++++++++------
> >  qapi/qmp-core.h          |   10 ++-
> >  qapi/qmp-dispatch.c      |    8 ++-
> >  qapi/qmp-registry.c      |    4 +-
> >  qemu-ga.c                |   40 ++++++------
> >  qga/commands-posix.c     |  162 ++++++++++++++++++----------------------------
> >  qga/guest-agent-core.h   |    4 ++
> >  scripts/qapi-commands.py |   14 +++-
> >  8 files changed, 154 insertions(+), 144 deletions(-)
> > 
> 
> Okay, this is definitely an enhancement and fix of bogus implementation.
> One thing that I'd like to ask is - how can user distinguish between
> these implementations. I am asking basically from libvirt POV.
> Because if I assume I am dealing with the previous implementation and
> thus waiting for the {'return':{}} before I can return form an API, but
> the GA actually uses the new implementation I will block endlessly.

There's no way to distinguish. It's a bug to wait for an OK response for
the guest-shutdown and guest-suspend-*, because a response may not been
sent even before this series (and this is correctly documented).

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On 05/14/2012 11:06 AM, Michal Privoznik wrote:
> On 11.05.2012 21:19, Luiz Capitulino wrote:
>> The main motivation for this series is fixing two possible race conditions
>> in the guest-suspend-* API due to the complexity that arose from the way
>> we handle terminated children processes today. Full details in the first
>> patch.
>>
>> This series applies on top of my two other qemu-ga series submitted
>> previously:
>>
>>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00999.html
>>
>>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01507.html
>>
>>  qapi-schema-guest.json   |   56 ++++++++++------
>>  qapi/qmp-core.h          |   10 ++-
>>  qapi/qmp-dispatch.c      |    8 ++-
>>  qapi/qmp-registry.c      |    4 +-
>>  qemu-ga.c                |   40 ++++++------
>>  qga/commands-posix.c     |  162 ++++++++++++++++++----------------------------
>>  qga/guest-agent-core.h   |    4 ++
>>  scripts/qapi-commands.py |   14 +++-
>>  8 files changed, 154 insertions(+), 144 deletions(-)
>>
> 
> Okay, this is definitely an enhancement and fix of bogus implementation.
> One thing that I'd like to ask is - how can user distinguish between
> these implementations. I am asking basically from libvirt POV.
> Because if I assume I am dealing with the previous implementation and
> thus waiting for the {'return':{}} before I can return form an API, but
> the GA actually uses the new implementation I will block endlessly.

I think the point was that you would block endlessly waiting for the
{'return':{}} anyways, even with the old implementation, because there
was no guarantee that the guest agent could issue the reply in a timely
manner.  Therefore, the only sane implementation in libvirt is to assume
that success will not be reported via the guest agent, and that libvirt
must _always_ probe for the listed side effects (a change in guest
status, qemu exiting, an event, or so forth), and optionally have a
timeout if the associated timeout does not occur in a reasonable time.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Michael Roth]

On Mon, May 14, 2012 at 11:17:18AM -0600, Eric Blake wrote:
> On 05/14/2012 11:06 AM, Michal Privoznik wrote:
> > On 11.05.2012 21:19, Luiz Capitulino wrote:
> >> The main motivation for this series is fixing two possible race conditions
> >> in the guest-suspend-* API due to the complexity that arose from the way
> >> we handle terminated children processes today. Full details in the first
> >> patch.
> >>
> >> This series applies on top of my two other qemu-ga series submitted
> >> previously:
> >>
> >>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00999.html
> >>
> >>  http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg01507.html
> >>
> >>  qapi-schema-guest.json   |   56 ++++++++++------
> >>  qapi/qmp-core.h          |   10 ++-
> >>  qapi/qmp-dispatch.c      |    8 ++-
> >>  qapi/qmp-registry.c      |    4 +-
> >>  qemu-ga.c                |   40 ++++++------
> >>  qga/commands-posix.c     |  162 ++++++++++++++++++----------------------------
> >>  qga/guest-agent-core.h   |    4 ++
> >>  scripts/qapi-commands.py |   14 +++-
> >>  8 files changed, 154 insertions(+), 144 deletions(-)
> >>
> > 
> > Okay, this is definitely an enhancement and fix of bogus implementation.
> > One thing that I'd like to ask is - how can user distinguish between
> > these implementations. I am asking basically from libvirt POV.
> > Because if I assume I am dealing with the previous implementation and
> > thus waiting for the {'return':{}} before I can return form an API, but
> > the GA actually uses the new implementation I will block endlessly.
> 
> I think the point was that you would block endlessly waiting for the
> {'return':{}} anyways, even with the old implementation, because there
> was no guarantee that the guest agent could issue the reply in a timely
> manner.  Therefore, the only sane implementation in libvirt is to assume
> that success will not be reported via the guest agent, and that libvirt
> must _always_ probe for the listed side effects (a change in guest
> status, qemu exiting, an event, or so forth), and optionally have a
> timeout if the associated timeout does not occur in a reasonable time.

Additionally, *all* commands carry the risk that qemu-ga can die
(killed, crashed, guest reboots, not being installed to begin with, etc) before
issuing a response, so a timeout mechanism *must* be in place if it blocks
libvirt.

Let me know if it's unclear how timeout+reset is to be implemented/used. I've
captured most of the ins/outs here but I realize it's still a bit of a
headache:

http://wiki.qemu.org/Features/QAPI/GuestAgent#QEMU_Guest_Agent_Protocol

> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 0/2]: qemu-ga: make shutdown & suspend synchronous

[Luiz Capitulino]

On Mon, 14 May 2012 13:12:35 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Additionally, *all* commands carry the risk that qemu-ga can die
> (killed, crashed, guest reboots, not being installed to begin with, etc) before
> issuing a response, so a timeout mechanism *must* be in place if it blocks
> libvirt.

True. This is unrelated to qemu-ga, but I think libvirt should allow the user
to change the timeout. You could have a very long default timeout, or even
infinite.