qemu-ga: guest-suspend: make the API synchronous
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>
This commit is contained in:
parent
226a48949c
commit
dc8764f061
17
qemu-ga.c
17
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;
|
||||
}
|
||||
|
||||
|
@ -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 (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);
|
||||
}
|
||||
|
||||
ret = read(fd, buf, sizeof(buf)-1);
|
||||
if (ret <= 0) {
|
||||
_exit(SUSPEND_NOT_SUPPORTED);
|
||||
}
|
||||
buf[ret] = '\0';
|
||||
|
||||
if (strstr(buf, sysfile_str)) {
|
||||
_exit(SUSPEND_SUPPORTED);
|
||||
}
|
||||
/*
|
||||
* 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);
|
||||
}
|
||||
|
||||
if (pid > 0) {
|
||||
wait(&status);
|
||||
} else {
|
||||
status = SUSPEND_NOT_SUPPORTED;
|
||||
fd = open(LINUX_SYS_STATE_FILE, O_RDONLY);
|
||||
if (fd < 0) {
|
||||
_exit(SUSPEND_NOT_SUPPORTED);
|
||||
}
|
||||
|
||||
ret = write(pipefds[1], &status, sizeof(status));
|
||||
if (ret != sizeof(status)) {
|
||||
_exit(EXIT_FAILURE);
|
||||
ret = read(fd, buf, sizeof(buf)-1);
|
||||
if (ret <= 0) {
|
||||
_exit(SUSPEND_NOT_SUPPORTED);
|
||||
}
|
||||
buf[ret] = '\0';
|
||||
|
||||
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 undef_err;
|
||||
}
|
||||
|
||||
ret = read(pipefds[0], &status, sizeof(status));
|
||||
if (ret == sizeof(status) && WIFEXITED(status) &&
|
||||
WEXITSTATUS(status) == SUSPEND_SUPPORTED) {
|
||||
goto out;
|
||||
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)
|
||||
|
Loading…
Reference in New Issue
Block a user