From e84f47abc8d5923f65fcff6314f264203099ab23 Mon Sep 17 00:00:00 2001 From: Andrew Wood Date: Thu, 3 Oct 2024 23:03:31 +0100 Subject: [PATCH] Refactor handling of TTOU so that instead of shuffling stderr to another file descriptor and using /dev/null when backgrounded, we set a flag to suspend terminal output, and perform all terminal writes through a new pv_tty_write function which checks that flag. --- docs/NEWS.md | 1 + src/include/pv-internal.h | 3 +- src/pv/cursor.c | 24 +++++++++---- src/pv/display.c | 6 ++-- src/pv/loop.c | 22 ++++++------ src/pv/signal.c | 76 +++++++++++---------------------------- 6 files changed, 55 insertions(+), 77 deletions(-) diff --git a/docs/NEWS.md b/docs/NEWS.md index 6699865..5896c6c 100644 --- a/docs/NEWS.md +++ b/docs/NEWS.md @@ -4,6 +4,7 @@ * cleanup: removed TODO.md, since it's just an outdated copy of the issue tracker * cleanup: re-ordered structure members to reduce padding * cleanup: improved readability of SIGTTOU handling code + * cleanup: instead of moving stderr when backgrounded, set a suspend-output flag ### 1.8.14 - 7 September 2024 diff --git a/src/include/pv-internal.h b/src/include/pv-internal.h index ec5cc63..7694a98 100644 --- a/src/include/pv-internal.h +++ b/src/include/pv-internal.h @@ -160,7 +160,6 @@ struct pvstate_s { volatile sig_atomic_t rxusr2; /* whether SIGUSR2 was received */ volatile pid_t sender; /* PID of sending process for SIGUSR2 */ #endif - int old_stderr; /* see pv_sig_ttou() */ } signal; /******************* @@ -171,6 +170,7 @@ struct pvstate_s { volatile sig_atomic_t terminal_resized; /* whether we need to get term size again */ volatile sig_atomic_t trigger_exit; /* whether we need to abort right now */ volatile sig_atomic_t clear_tty_tostop_on_exit; /* whether to clear tty TOSTOP on exit */ + volatile sig_atomic_t suspend_stderr; /* whether writing to stderr is suspended */ } flag; /***************** @@ -330,6 +330,7 @@ int pv_next_file(pvstate_t, unsigned int, int); /*@out@*/ const char *pv_current_file_name(pvstate_t); void pv_write_retry(int, const char *, size_t); +void pv_tty_write(pvstate_t, const char *, size_t); void pv_crs_fini(pvstate_t); void pv_crs_init(pvstate_t); diff --git a/src/pv/cursor.c b/src/pv/cursor.c index ea87916..89f565d 100644 --- a/src/pv/cursor.c +++ b/src/pv/cursor.c @@ -69,6 +69,18 @@ void pv_write_retry(int fd, const char *buf, size_t count) } +/* + * Write the given buffer to the terminal with pv_write_retry(), unless + * stderr is suspended. + */ +void pv_tty_write(pvstate_t state, const char *buf, size_t count) +{ + if (1 == state->flag.suspend_stderr) + return; + pv_write_retry(STDERR_FILENO, buf, count); +} + + /* * Create a per-euid, per-tty, lockfile in ${TMPDIR:-${TMP:-/tmp}} for the * tty on the given file descriptor. @@ -462,7 +474,7 @@ void pv_crs_init(pvstate_t state) * initial ypos. */ if (state->cursor.y_start > 0) - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); pv_crs_unlock(state, terminalfd); if (state->cursor.y_start < 1) @@ -589,9 +601,9 @@ void pv_crs_update(pvstate_t state, const char *output_line) memset(cup_cmd, 0, sizeof(cup_cmd)); (void) pv_snprintf(cup_cmd, sizeof(cup_cmd), "\033[%u;1H", state->control.height); cup_cmd_length = strlen(cup_cmd); /* flawfinder: ignore */ - pv_write_retry(STDERR_FILENO, cup_cmd, cup_cmd_length); + pv_tty_write(state, cup_cmd, cup_cmd_length); for (; offs > 0; offs--) { - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); } pv_crs_unlock(state, STDERR_FILENO); @@ -624,8 +636,8 @@ void pv_crs_update(pvstate_t state, const char *output_line) pv_crs_lock(state, STDERR_FILENO); - pv_write_retry(STDERR_FILENO, cup_cmd, cup_cmd_length); - pv_write_retry(STDERR_FILENO, output_line, output_line_length); + pv_tty_write(state, cup_cmd, cup_cmd_length); + pv_tty_write(state, output_line, output_line_length); pv_crs_unlock(state, STDERR_FILENO); } @@ -664,7 +676,7 @@ void pv_crs_fini(pvstate_t state) pv_crs_lock(state, STDERR_FILENO); - pv_write_retry(STDERR_FILENO, cup_cmd, strlen(cup_cmd)); /* flawfinder: ignore */ + pv_tty_write(state, cup_cmd, strlen(cup_cmd)); /* flawfinder: ignore */ /* flawfinder - pv_snprintf() always \0-terminates (see above). */ #ifdef HAVE_IPC diff --git a/src/pv/display.c b/src/pv/display.c index 2102864..794a083 100644 --- a/src/pv/display.c +++ b/src/pv/display.c @@ -1321,7 +1321,7 @@ void pv_display(pvstate_t state, long double esec, off_t sl, off_t tot) return; if (state->control.numeric) { - pv_write_retry(STDERR_FILENO, state->display.display_buffer, state->display.display_string_len); + pv_tty_write(state, state->display.display_buffer, state->display.display_string_len); } else if (state->control.cursor) { if (state->control.force || pv_in_foreground()) { pv_crs_update(state, state->display.display_buffer); @@ -1329,8 +1329,8 @@ void pv_display(pvstate_t state, long double esec, off_t sl, off_t tot) } } else { if (state->control.force || pv_in_foreground()) { - pv_write_retry(STDERR_FILENO, state->display.display_buffer, state->display.display_string_len); - pv_write_retry(STDERR_FILENO, "\r", 1); + pv_tty_write(state, state->display.display_buffer, state->display.display_string_len); + pv_tty_write(state, "\r", 1); state->display.display_visible = true; } } diff --git a/src/pv/loop.c b/src/pv/loop.c index ecf3156..510edb0 100644 --- a/src/pv/loop.c +++ b/src/pv/loop.c @@ -354,7 +354,7 @@ int pv_main_loop(pvstate_t state) } else { if ((!state->control.numeric) && (!state->control.no_display) && (state->display.display_visible)) - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); } if (1 == state->flag.trigger_exit) @@ -517,7 +517,7 @@ int pv_watchfd_loop(pvstate_t state) } if (!state->control.numeric) - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); if (1 == state->flag.trigger_exit) state->status.exit_status |= PV_ERROREXIT_SIGNAL; @@ -742,7 +742,7 @@ int pv_watchpid_loop(pvstate_t state) if (displayed_lines > 0) { debug("%s", "adding newline"); - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); } debug("%s %d [%d]: %Lf / %Ld / %Ld", "fd", fd, idx, elapsed_seconds, transferred_since_last, @@ -765,10 +765,10 @@ int pv_watchpid_loop(pvstate_t state) while (blank_lines > 0) { unsigned int x; if (displayed_lines > 0) - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); for (x = 0; x < state->control.width; x++) - pv_write_retry(STDERR_FILENO, " ", 1); - pv_write_retry(STDERR_FILENO, "\r", 1); + pv_tty_write(state, " ", 1); + pv_tty_write(state, "\r", 1); blank_lines--; displayed_lines++; } @@ -776,7 +776,7 @@ int pv_watchpid_loop(pvstate_t state) debug("%s: %d", "displayed lines", displayed_lines); while (displayed_lines > 1) { - pv_write_retry(STDERR_FILENO, "\033[A", 3); + pv_tty_write(state, "\033[A", 3); displayed_lines--; } } @@ -788,14 +788,14 @@ int pv_watchpid_loop(pvstate_t state) while (blank_lines > 0) { unsigned int x; for (x = 0; x < state->control.width; x++) - pv_write_retry(STDERR_FILENO, " ", 1); - pv_write_retry(STDERR_FILENO, "\r", 1); + pv_tty_write(state, " ", 1); + pv_tty_write(state, "\r", 1); blank_lines--; if (blank_lines > 0) - pv_write_retry(STDERR_FILENO, "\n", 1); + pv_tty_write(state, "\n", 1); } while (prev_displayed_lines > 1) { - pv_write_retry(STDERR_FILENO, "\033[A", 3); + pv_tty_write(state, "\033[A", 3); prev_displayed_lines--; } diff --git a/src/pv/signal.c b/src/pv/signal.c index ec1592c..afa393e 100644 --- a/src/pv/signal.c +++ b/src/pv/signal.c @@ -69,44 +69,21 @@ static void pv_sig_ensure_tty_tostop() } /* - * Handle SIGTTOU (tty output for background process) by redirecting stderr - * to /dev/null, so that we can be stopped and backgrounded without messing - * up the terminal. We store the old stderr file descriptor so that on a - * subsequent SIGCONT we can try writing to the terminal again, in case we - * get backgrounded and later get foregrounded again. + * Handle SIGTTOU (tty output for background process) by setting the flag to + * suspend writes to stderr, so that we can be stopped and backgrounded + * without messing up the terminal. On a subsequent SIGCONT we will try + * writing to the terminal again, in case we get backgrounded and later get + * foregrounded again. */ static void pv_sig_ttou( /*@unused@ */ __attribute__((unused)) int s) { - int fd; - if (NULL == pv_sig_state) return; - fd = open("/dev/null", O_RDWR); /* flawfinder: ignore */ - if (fd < 0) { - debug("%s: %s", "failed to open /dev/null", strerror(errno)); - return; - } - - /* - * flawfinder rationale: not checking for symlinks because this is - * explicitly a device file under /dev so we assume it is safe to - * open. - * - * TODO: look at just preventing stderr output instead of writing to - * stderr while backgrounded. - */ - - if (-1 == pv_sig_state->signal.old_stderr) - pv_sig_state->signal.old_stderr = dup(STDERR_FILENO); - - if (dup2(fd, STDERR_FILENO) < 0) { - debug("%s: %s", "failed to replace stderr", strerror(errno)); - } - - if (0 != close(fd)) { - debug("%s: %s", "failed to close /dev/null", strerror(errno)); + if (1 != pv_sig_state->flag.suspend_stderr) { + debug("%s", "SIGTTOU - suspending stderr"); + pv_sig_state->flag.suspend_stderr = 1; } } @@ -130,7 +107,7 @@ static void pv_sig_tstp( /*@unused@ */ __attribute__((unused)) /* * Handle SIGCONT (continue if stopped) by adding the elapsed time since the * last SIGTSTP to the elapsed time offset, and by trying to write to the - * terminal again (by replacing the /dev/null stderr with the old stderr). + * terminal again. */ static void pv_sig_cont( /*@unused@ */ __attribute__((unused)) int s) @@ -166,16 +143,11 @@ static void pv_sig_cont( /*@unused@ */ __attribute__((unused)) } /* - * Restore the old stderr, if we had replaced it. + * Try resuming our use of stderr, if we had suspended it. */ - if (pv_sig_state->signal.old_stderr != -1) { - if (dup2(pv_sig_state->signal.old_stderr, STDERR_FILENO) < 0) { - debug("%s: %s", "failed to restore old stderr", strerror(errno)); - } - if (0 != close(pv_sig_state->signal.old_stderr)) { - debug("%s: %s", "failed to close duplicate old stderr", strerror(errno)); - } - pv_sig_state->signal.old_stderr = -1; + if (1 == pv_sig_state->flag.suspend_stderr) { + debug("%s", "SIGCONT - resuming stderr"); + pv_sig_state->flag.suspend_stderr = 0; } pv_sig_ensure_tty_tostop(); @@ -283,7 +255,7 @@ void pv_sig_init(pvstate_t state) pv_sig_state = state; - pv_sig_state->signal.old_stderr = -1; + pv_sig_state->flag.suspend_stderr = 0; pv_elapsedtime_zero(&(pv_sig_state->signal.tstp_time)); pv_elapsedtime_zero(&(pv_sig_state->signal.toffset)); @@ -510,10 +482,9 @@ void pv_sig_allowpause(void) /* - * If we have redirected stderr to /dev/null, check every second or so to - * see whether we can write to the terminal again - this is so that if we - * get backgrounded, then foregrounded again, we start writing to the - * terminal again. + * If we have suspended stderr, check every second or so to see whether we + * can write to the terminal again - this is so that if we get backgrounded, + * then foregrounded again, we start writing to the terminal again. */ void pv_sig_checkbg(void) { @@ -527,18 +498,11 @@ void pv_sig_checkbg(void) next_check = time(NULL) + 1; - if (-1 == pv_sig_state->signal.old_stderr) + if (0 == pv_sig_state->flag.suspend_stderr) return; - if (dup2(pv_sig_state->signal.old_stderr, STDERR_FILENO) < 0) { - debug("%s: %s", "failed to restore old stderr", strerror(errno)); - } - - if (0 != close(pv_sig_state->signal.old_stderr)) { - debug("%s: %s", "failed to close duplicate old stderr", strerror(errno)); - } - - pv_sig_state->signal.old_stderr = -1; + debug("%s: %s", "pv_sig_checkbg", "attempting to resume stderr"); + pv_sig_state->flag.suspend_stderr = 0; pv_sig_ensure_tty_tostop(); #ifdef HAVE_IPC