From patchwork Thu Apr 19 17:36:54 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luiz Capitulino X-Patchwork-Id: 153842 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 93E4AB6FEE for ; Fri, 20 Apr 2012 03:37:13 +1000 (EST) Received: from localhost ([::1]:54854 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKvI7-0005TE-Ef for incoming@patchwork.ozlabs.org; Thu, 19 Apr 2012 13:37:11 -0400 Received: from eggs.gnu.org ([208.118.235.92]:36389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKvHs-0005N1-TJ for qemu-devel@nongnu.org; Thu, 19 Apr 2012 13:36:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SKvHn-0007QZ-O5 for qemu-devel@nongnu.org; Thu, 19 Apr 2012 13:36:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43789) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SKvHn-0007QL-Fi for qemu-devel@nongnu.org; Thu, 19 Apr 2012 13:36:51 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3JHamQ8013997 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 19 Apr 2012 13:36:48 -0400 Received: from localhost (ovpn-113-135.phx2.redhat.com [10.3.113.135]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q3JHaljh022085; Thu, 19 Apr 2012 13:36:48 -0400 From: Luiz Capitulino To: qemu-devel@nongnu.org Date: Thu, 19 Apr 2012 14:36:54 -0300 Message-Id: <1334857015-8726-2-git-send-email-lcapitulino@redhat.com> In-Reply-To: <1334857015-8726-1-git-send-email-lcapitulino@redhat.com> References: <1334857015-8726-1-git-send-email-lcapitulino@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: pbonzini@redhat.com, lersek@redhat.com, mdroth@linux.vnet.ibm.com Subject: [Qemu-devel] [RFC 1/2] qemu-ga: suspend: collect child exit status X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Currently, qemu-ga has a SIGCHLD handler that automatically reaps terminated children processes. However, if a function is interested in the exit status of a terminated child, it has to play tricks like bios_supports_mode() does, it double forks and get the child's exit status via pipe. This is too hacky, complex, and can lead to signal related race conditions. A much simpler approach would be to drop the SIGCHLD handler and wait for children manually by using waitpid(). This is what this patch does in bios_supports_mode() and guest_suspend(). It also drops the SIGCHLD handler. There are two behavior changes that can be perceived by clients: 1. On success, the current code _may_ be able to send an OK response. With this commit this will never happen: the response will always be sent only after the guest resumes. This shouldn't be a problem though, as it's correctly documented that an OK response may not be sent 2. The current code isn't able to detect an error in the child, meaning that it will return an OK response even if the child fails or explodes. This commit fixes that by only returning an OK response when the child really succeeds Signed-off-by: Luiz Capitulino --- qemu-ga.c | 17 +------- qga/commands-posix.c | 117 +++++++++++++++++++------------------------------- 2 files changed, 46 insertions(+), 88 deletions(-) diff --git a/qemu-ga.c b/qemu-ga.c index d6f786e..88c8478 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -76,16 +76,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)); @@ -102,14 +95,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; } #endif diff --git a/qga/commands-posix.c b/qga/commands-posix.c index faf970d..1fcbfce 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -517,117 +517,83 @@ 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; + goto exit_err; } - ret = read(pipefds[0], &status, sizeof(status)); - if (ret == sizeof(status) && WIFEXITED(status) && - WEXITSTATUS(status) == SUSPEND_SUPPORTED) { - goto out; + rpid = waitpid(pid, &status, 0); + if (rpid == pid && WIFEXITED(status)) { + if (WEXITSTATUS(status) == SUSPEND_SUPPORTED) { + return; + } else if (WEXITSTATUS(status) == SUSPEND_NOT_SUPPORTED) { + error_set(err, QERR_UNSUPPORTED); + return; + } } - error_set(err, QERR_UNSUPPORTED); - -out: - close(pipefds[0]); +exit_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); @@ -669,9 +635,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)