From 79e3a4694365b58b482f2f437052773711ec67dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Wed, 10 Feb 2021 16:22:51 +0100 Subject: [PATCH] reaper: monitor SIGCHLD using the FDM instead of via a signalfd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In addition to letting the FDM do the low-level signal watching, this patch also fixes a bug; multiple SIGCHLDs, be it delivered either through a signal, or via a signalfd, can be coalesced, like all signals. This means we need to loop on waitpid() with WNOHANG until there are no more processes to reap. This in turn requires a small change to the way reaper callbacks are implemented. Previously, the callback was allowed to do the wait(). This was signalled back to the reaper through the callback’s return value. Now, since we’ve already wait():ed, the process’ exit status is passed as an argument to the reaper callback. The callback for the client application has been updated accordingly; it sets a flag in the terminal struct, telling term_destroy() that the process has already been wait():ed on, and also stores the exit status. --- reaper.c | 116 ++++++++++++++--------------------------------------- reaper.h | 3 +- terminal.c | 110 +++++++++++++++++++++++++++----------------------- terminal.h | 3 +- 4 files changed, 92 insertions(+), 140 deletions(-) diff --git a/reaper.c b/reaper.c index 9d98854e..4abc785e 100644 --- a/reaper.c +++ b/reaper.c @@ -3,13 +3,13 @@ #include #include #include +#include #include -#include +#include #define LOG_MODULE "reaper" #define LOG_ENABLE_DBG 0 #include "log.h" -#include "tllist.h" struct child { pid_t pid; @@ -19,56 +19,32 @@ struct child { struct reaper { struct fdm *fdm; - int fd; tll(struct child) children; }; -static bool fdm_reap(struct fdm *fdm, int fd, int events, void *data); +static bool fdm_reap(struct fdm *fdm, int signo, void *data); struct reaper * reaper_init(struct fdm *fdm) { - sigset_t mask; - sigemptyset(&mask); - sigaddset(&mask, SIGCHLD); - - /* Block normal signal handling - we're using a signalfd instead */ - if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0) { - LOG_ERRNO("failed to block SIGCHLD"); - return NULL; - } - - int fd = signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC); - if (fd < 0) { - LOG_ERRNO("failed to create signal FD"); - sigprocmask(SIG_UNBLOCK, &mask, NULL); - return NULL; - } - struct reaper *reaper = malloc(sizeof(*reaper)); if (unlikely(reaper == NULL)) { LOG_ERRNO("malloc() failed"); - close(fd); - sigprocmask(SIG_UNBLOCK, &mask, NULL); return NULL; } *reaper = (struct reaper){ .fdm = fdm, - .fd = fd, .children = tll_init(), }; - if (!fdm_add(fdm, fd, EPOLLIN, &fdm_reap, reaper)){ - LOG_ERR("failed to register with the FDM"); + if (!fdm_signal_add(fdm, SIGCHLD, &fdm_reap, reaper)) goto err; - } return reaper; err: tll_free(reaper->children); - close(fd); free(reaper); return NULL; } @@ -79,14 +55,9 @@ reaper_destroy(struct reaper *reaper) if (reaper == NULL) return; - fdm_del(reaper->fdm, reaper->fd); + fdm_signal_del(reaper->fdm, SIGCHLD); tll_free(reaper->children); free(reaper); - - sigset_t mask; - sigemptyset(&mask); - sigaddset(&mask, SIGCHLD); - sigprocmask(SIG_UNBLOCK, &mask, NULL); } void @@ -110,69 +81,40 @@ reaper_del(struct reaper *reaper, pid_t pid) } static bool -fdm_reap(struct fdm *fdm, int fd, int events, void *data) +fdm_reap(struct fdm *fdm, int signo, void *data) { struct reaper *reaper = data; - bool pollin = events & EPOLLIN; - bool hup = events & EPOLLHUP; + while (true) { + int status; + pid_t pid = waitpid(-1, &status, WNOHANG); + if (pid <= 0) + break; - if (hup && !pollin) - return false; + if (WIFEXITED(status)) + LOG_DBG("pid=%d: exited with status=%d", pid, WEXITSTATUS(status)); + else if (WIFSIGNALED(status)) + LOG_DBG("pid=%d: killed by signal=%d", pid, WTERMSIG(status)); + else + LOG_DBG("pid=%d: died of unknown resason", pid); - assert(pollin); + tll_foreach(reaper->children, it) { + struct child *_child = &it->item; - struct signalfd_siginfo info; - ssize_t amount = read(reaper->fd, &info, sizeof(info)); - - if (amount < 0) { - LOG_ERRNO("failed to read"); - return false; - } - - assert((size_t)amount >= sizeof(info)); - - if (info.ssi_signo != SIGCHLD) { - LOG_WARN("got non-SIGCHLD signal: %d", info.ssi_signo); - return true; - } - - tll_foreach(reaper->children, it) { - struct child *_child = &it->item; - - if (_child->pid != (pid_t)info.ssi_pid) - continue; - - /* Make sure we remove it *before* the callback, since it too - * may remove it */ - struct child child = it->item; - tll_remove(reaper->children, it); - - bool reap_ourselves = true; - if (child.cb != NULL) - reap_ourselves = !child.cb(reaper, child.pid, child.cb_data); - - if (reap_ourselves) { - int result; - int res = waitpid(child.pid, &result, WNOHANG); - - if (res <= 0) { - if (res < 0) - LOG_ERRNO("waitpid failed for pid=%d", child.pid); + if (_child->pid != pid) continue; - } - else if (WIFEXITED(result)) - LOG_DBG("pid=%d: exited with status=%d", pid, WEXITSTATUS(result)); - else if (WIFSIGNALED(result)) - LOG_DBG("pid=%d: killed by signal=%d", pid, WTERMSIG(result)); - else - LOG_DBG("pid=%d: died of unknown resason", pid); + /* Make sure we remove it *before* the callback, since it too + * may remove it */ + struct child child = it->item; + tll_remove(reaper->children, it); + + if (child.cb != NULL) + child.cb(reaper, child.pid, status, child.cb_data); + + break; } } - if (hup) - return false; - return true; } diff --git a/reaper.h b/reaper.h index 2cd6dd6e..4416af03 100644 --- a/reaper.h +++ b/reaper.h @@ -10,7 +10,8 @@ struct reaper; struct reaper *reaper_init(struct fdm *fdm); void reaper_destroy(struct reaper *reaper); -typedef bool (*reaper_cb)(struct reaper *reaper, pid_t pid, void *data); +typedef void (*reaper_cb)( + struct reaper *reaper, pid_t pid, int status, void *data); void reaper_add(struct reaper *reaper, pid_t pid, reaper_cb cb, void *cb_data); void reaper_del(struct reaper *reaper, pid_t pid); diff --git a/terminal.c b/terminal.c index c65fe974..f12465c3 100644 --- a/terminal.c +++ b/terminal.c @@ -953,22 +953,25 @@ load_fonts_from_conf(struct terminal *term) return reload_fonts(term); } -static bool -slave_died(struct reaper *reaper, pid_t pid, void *data) +static void +slave_died(struct reaper *reaper, pid_t pid, int status, void *data) { struct terminal *term = data; LOG_DBG("slave (PID=%u) died", pid); + term->slave_has_been_reaped = true; + term->exit_status = status; + if (term->conf->hold_at_exit) { /* The PTMX FDM handler may already have closed our end */ if (term->ptmx >= 0) { fdm_del(term->fdm, term->ptmx); term->ptmx = -1; } - return true; + return; } - return term_shutdown(term); + term_shutdown(term); } struct terminal * @@ -1044,7 +1047,6 @@ term_init(const struct config *conf, struct fdm *fdm, struct reaper *reaper, .fdm = fdm, .reaper = reaper, .conf = conf, - .quit = false, .ptmx = ptmx, .ptmx_buffers = tll_init(), .ptmx_paste_buffers = tll_init(), @@ -1457,70 +1459,76 @@ term_destroy(struct terminal *term) int ret = EXIT_SUCCESS; if (term->slave > 0) { - LOG_DBG("waiting for slave (PID=%u) to die", term->slave); + int exit_status; - /* - * Note: we've closed ptmx, so the slave *should* exit... - * - * But, since it is possible to write clients that ignore - * this, we need to handle it in *some* way. - * - * So, what we do is register a SIGALRM handler, and configure - * a 2 second alarm. If the slave hasn't died after this time, - * we send it a SIGTERM, then wait another 2 seconds (using - * the same alarm mechanism). If it still hasn't died, we send - * it a SIGKILL. - * - * Note that this solution is *not* asynchronous, and any - * other events etc will be ignored during this time. This of - * course only applies to a 'foot --server' instance, where - * there might be other terminals running. - */ - sigaction(SIGALRM, &(const struct sigaction){.sa_handler = &sig_alarm}, NULL); - alarm(2); + if (term->slave_has_been_reaped) + exit_status = term->exit_status; + else { + LOG_DBG("waiting for slave (PID=%u) to die", term->slave); - int status; - int kill_signal = SIGTERM; + /* + * Note: we've closed ptmx, so the slave *should* exit... + * + * But, since it is possible to write clients that ignore + * this, we need to handle it in *some* way. + * + * So, what we do is register a SIGALRM handler, and configure + * a 2 second alarm. If the slave hasn't died after this time, + * we send it a SIGTERM, then wait another 2 seconds (using + * the same alarm mechanism). If it still hasn't died, we send + * it a SIGKILL. + * + * Note that this solution is *not* asynchronous, and any + * other events etc will be ignored during this time. This of + * course only applies to a 'foot --server' instance, where + * there might be other terminals running. + */ + sigaction(SIGALRM, &(const struct sigaction){.sa_handler = &sig_alarm}, NULL); + alarm(2); - while (true) { - int r = waitpid(term->slave, &status, 0); + int kill_signal = SIGTERM; - if (r == term->slave) - break; + while (true) { + int r = waitpid(term->slave, &exit_status, 0); - if (r == -1) { - assert(errno == EINTR); + if (r == term->slave) + break; - if (alarm_raised) { - LOG_DBG("slave hasn't died yet, sending: %s (%d)", - kill_signal == SIGTERM ? "SIGTERM" : "SIGKILL", - kill_signal); + if (r == -1) { + assert(errno == EINTR); - kill(term->slave, kill_signal); + if (alarm_raised) { + LOG_DBG("slave hasn't died yet, sending: %s (%d)", + kill_signal == SIGTERM ? "SIGTERM" : "SIGKILL", + kill_signal); - alarm_raised = 0; - if (kill_signal != SIGKILL) - alarm(2); + kill(term->slave, kill_signal); - kill_signal = SIGKILL; + alarm_raised = 0; + if (kill_signal != SIGKILL) + alarm(2); + kill_signal = SIGKILL; + + } } } + + /* Cancel alarm */ + alarm(0); + sigaction(SIGALRM, &(const struct sigaction){.sa_handler = SIG_DFL}, NULL); } - /* Cancel alarm */ - alarm(0); - sigaction(SIGALRM, &(const struct sigaction){.sa_handler = SIG_DFL}, NULL); - ret = EXIT_FAILURE; - if (WIFEXITED(status)) { - ret = WEXITSTATUS(status); + if (WIFEXITED(exit_status)) { + ret = WEXITSTATUS(exit_status); LOG_DBG("slave exited with code %d", ret); - } else if (WIFSIGNALED(status)) { - ret = WTERMSIG(status); + } else if (WIFSIGNALED(exit_status)) { + ret = WTERMSIG(exit_status); LOG_WARN("slave exited with signal %d (%s)", ret, strsignal(ret)); } else { - LOG_WARN("slave exited for unknown reason (status = 0x%08x)", status); + LOG_WARN("slave exited for unknown reason (status = 0x%08x)", + exit_status); } } diff --git a/terminal.h b/terminal.h index b6b150ae..3c341299 100644 --- a/terminal.h +++ b/terminal.h @@ -487,8 +487,9 @@ struct terminal { } ime; #endif - bool quit; bool is_shutting_down; + bool slave_has_been_reaped; + int exit_status; void (*shutdown_cb)(void *data, int exit_code); void *shutdown_data;