[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 09/13] qemu-ga: guest-suspend: make the API synchron
From: |
Michael Roth |
Subject: |
[Qemu-devel] [PATCH 09/13] qemu-ga: guest-suspend: make the API synchronous |
Date: |
Tue, 15 May 2012 09:48:55 -0500 |
From: Luiz Capitulino <address@hidden>
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 <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Michael Roth <address@hidden>
---
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 02/13] qemu-ga: don't warn on no command return, (continued)
- [Qemu-devel] [PATCH 02/13] qemu-ga: don't warn on no command return, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 04/13] qemu-ga: guest-suspend-disk: don't emit a success response, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 01/13] qapi: add support for command options, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 03/13] qemu-ga: guest-shutdown: don't emit a success response, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 05/13] qemu-ga: guest-suspend-ram: don't emit a success response, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 12/13] qemu-ga: fix segv after failure to open log file, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 06/13] qemu-ga: guest-suspend-hybrid: don't emit a success response, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 11/13] qemu-ga: guest-shutdown: use only async-signal-safe functions, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 08/13] qemu-ga: become_daemon(): reopen standard fds to /dev/null, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 07/13] qemu-ga: make reopen_fd_to_null() public, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 09/13] qemu-ga: guest-suspend: make the API synchronous,
Michael Roth <=
- [Qemu-devel] [PATCH 10/13] qemu-ga: guest-shutdown: become synchronous, Michael Roth, 2012/05/15
- [Qemu-devel] [PATCH 13/13] qemu-ga: align versioning with QEMU_VERSION, Michael Roth, 2012/05/15
- Re: [Qemu-devel] [PULL] qemu-ga fixes for 1.1, Luiz Capitulino, 2012/05/21