From 78bb93b28821862d942676174f5e18781366ed31 Mon Sep 17 00:00:00 2001 From: Andrew Wood Date: Sat, 2 Sep 2023 22:06:31 +0100 Subject: [PATCH] Move terminal TOSTOP attribute changes into signal handling functions and only set/unset that attribute rather than saving and restoring the whole terminal state; keep track of whether we have changed it and change cursor positioning IPC data to a structure so the state change can be shared with all "pv -c" instances. --- doc/NEWS.md | 1 + doc/TODO.md | 3 - src/include/pv-internal.h | 12 +++- src/main/main.c | 103 ++++++++------------------- src/pv/cursor.c | 45 +++++++++--- src/pv/signal.c | 144 ++++++++++++++++++++++++++++++-------- src/pv/transfer.c | 2 +- 7 files changed, 190 insertions(+), 120 deletions(-) diff --git a/doc/NEWS.md b/doc/NEWS.md index 39bf41d..1cde59a 100644 --- a/doc/NEWS.md +++ b/doc/NEWS.md @@ -11,6 +11,7 @@ * fix: use `clock_gettime()` in ETA calculation to cope with machine suspend/resume ([#13](https://codeberg.org/a-j-wood/pv/issues/13)) * fix: if "`--width`" or "`--height`" were provided, do not change them when the window size changes ([#36](https://codeberg.org/a-j-wood/pv/issues/36)) * fix: when a file descriptor position in "`--watchfd`" moves backwards, show the rate using the correct prefix ([#41](https://codeberg.org/a-j-wood/pv/issues/41)) + * fix: rewrite terminal state save/restore so state is not intermittently garbled on exit when using "`--cursor`" ([#20](https://codeberg.org/a-j-wood/pv/issues/20)), ([#24](https://codeberg.org/a-j-wood/pv/issues/24)) * cleanup: switched the build system to GNU Automake * cleanup: added a test for terminal width detection to "`make check`" * cleanup: added a test to "`make check`" to ensure that "`make install`" installs everything expected diff --git a/doc/TODO.md b/doc/TODO.md index 5c9bbe9..eb809ab 100644 --- a/doc/TODO.md +++ b/doc/TODO.md @@ -3,9 +3,6 @@ Things still to do. (#n) indicates the issue tracker number. Bugs ---- - * ([#5](https://codeberg.org/a-j-wood/pv/issues/5)) Transfer IPC leadership on exit of leader - * ([#20](https://codeberg.org/a-j-wood/pv/issues/20)) Terminal state is not restored correctly in all cases (VA) - * ([#24](https://codeberg.org/a-j-wood/pv/issues/24)) Debian #890901 - Race condition with multiple "`pv -c`" leaves terminal state inconsistent (Lars Ellenberg, Viktor Ashirov) * ([#34](https://codeberg.org/a-j-wood/pv/issues/34)) Continue timer even if input or output is blocking (Martin Probst - Jun 2017) Feature requests diff --git a/src/include/pv-internal.h b/src/include/pv-internal.h index a7fd931..5ba8f8c 100644 --- a/src/include/pv-internal.h +++ b/src/include/pv-internal.h @@ -71,6 +71,15 @@ typedef struct pvhistory { #define PV_SIZEOF_DISPLAY_NAME 512 +/* + * Structure for data shared between multiple "pv -c" instances. + */ +struct pvcursorstate_s { + int y_topmost; /* terminal row of topmost "pv" instance */ + bool tty_tostop_added; /* whether any instance had to set TOSTOP on the terminal */ +}; + + /* * Structure for holding PV internal state. Opaque outside the PV library. */ @@ -127,6 +136,7 @@ struct pvstate_s { * Signal handling * *******************/ int pv_sig_old_stderr; /* see pv_sig_ttou() */ + bool pv_tty_tostop_added; /* whether we had to set TOSTOP on the terminal */ struct timespec pv_sig_tstp_time; /* see pv_sig_tstp() / __cont() */ struct timespec pv_sig_toffset; /* total time spent stopped */ volatile sig_atomic_t pv_sig_newsize; /* whether we need to get term size again */ @@ -188,7 +198,7 @@ struct pvstate_s { int crs_shmid; /* ID of our shared memory segment */ int crs_pvcount; /* number of `pv' processes in total */ int crs_pvmax; /* highest number of `pv's seen */ - int *crs_y_top; /* pointer to Y coord of topmost `pv' */ + struct pvcursorstate_s *crs_shared; /* data shared between instances */ int crs_y_lastread; /* last value of _y_top seen */ int crs_y_offset; /* our Y offset from this top position */ int crs_needreinit; /* counter if we need to reinit cursor pos */ diff --git a/src/main/main.c b/src/main/main.c index ec428f8..7d60bc8 100644 --- a/src/main/main.c +++ b/src/main/main.c @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -36,24 +35,25 @@ void pv_remote_fini(void); */ int main(int argc, char **argv) { - struct termios t, t_save; /*@only@ */ opts_t opts = NULL; /*@only@ */ pvstate_t state = NULL; - bool t_saved, t_needs_reset; int retcode = 0; #ifdef ENABLE_NLS + /* Initialise language translation. */ (void) setlocale(LC_ALL, ""); (void) bindtextdomain(PACKAGE, LOCALEDIR); (void) textdomain(PACKAGE); #endif + /* Parse the command line arguments. */ opts = opts_parse(argc >= 0 ? (unsigned int) argc : 0, argv); if (NULL == opts) { debug("%s: %d", "exiting with status", 64); return 64; } + /* Early exit if necessary, such as with "-h". */ if (opts->do_nothing) { debug("%s", "nothing to do - exiting with status 0"); opts_free(opts); @@ -180,12 +180,13 @@ int main(int argc, char **argv) } /* - * Put our list of files into the PV internal state. + * Put our list of input files into the PV internal state. */ if (NULL != opts->argv) { pv_state_inputfiles(state, opts->argc, (const char **) (opts->argv)); } + /* Total size calculation, in normal transfer mode. */ if (0 == opts->watch_pid) { /* * If no size was given, try to calculate the total size. @@ -300,85 +301,37 @@ int main(int argc, char **argv) fcntl(STDOUT_FILENO, F_SETFL, O_NONBLOCK | fcntl(STDOUT_FILENO, F_GETFL)); #endif /* MAKE_STDOUT_NONBLOCKING */ - /* - * Keep track of whether we've saved the terminal attributes and - * whether we need to reset them at the end. - */ - t_saved = false; - t_needs_reset = false; + /* Initialise the signal handling. */ + pv_sig_init(state); - /* - * Set terminal option TOSTOP so we get signal SIGTTOU if we try to - * write to the terminal while backgrounded. - * - * Also, save the current terminal attributes for later restoration. - */ - memset(&t, 0, sizeof(t)); - if (0 != isatty(STDERR_FILENO)) { - if (0 == tcgetattr(STDERR_FILENO, &t)) { - debug("%s", "saved terminal attributes"); - t_saved = true; - } else { - /*@-mustfreefresh@ *//* see above */ - fprintf(stderr, "%s: %s: %s\n", opts->program_name, - _("failed to read terminal attributes"), strerror(errno)); - /*@+mustfreefresh@ */ - } - } - t_save = t; - if (t_saved && pv_in_foreground()) { - t.c_lflag |= TOSTOP; - (void) tcsetattr(STDERR_FILENO, TCSANOW, &t); - t_needs_reset = true; - debug("%s", "set terminal TOSTOP attribute"); - } - - if (0 != opts->watch_pid) { - if (0 <= opts->watch_fd) { - pv_sig_init(state); - retcode = pv_watchfd_loop(state); - if (t_needs_reset && pv_in_foreground()) { - (void) tcsetattr(STDERR_FILENO, TCSANOW, &t_save); - } - if (opts->pidfile != NULL) { - if (0 != remove(opts->pidfile)) { - fprintf(stderr, "%s: %s: %s\n", - opts->program_name, opts->pidfile, strerror(errno)); - } - } - pv_sig_fini(state); - } else { - pv_sig_init(state); - retcode = pv_watchpid_loop(state); - if (t_needs_reset && pv_in_foreground()) { - (void) tcsetattr(STDERR_FILENO, TCSANOW, &t_save); - } - if (opts->pidfile != NULL) { - if (0 != remove(opts->pidfile)) { - fprintf(stderr, "%s: %s: %s\n", - opts->program_name, opts->pidfile, strerror(errno)); - } - } - pv_sig_fini(state); - } - } else { - pv_sig_init(state); + /* Run the appropriate main loop. */ + if (0 == opts->watch_pid) { + /* Normal "transfer data" mode. */ pv_remote_init(); retcode = pv_main_loop(state); pv_remote_fini(); - if (t_needs_reset && pv_in_foreground()) { - (void) tcsetattr(STDERR_FILENO, TCSANOW, &t_save); - } - if (opts->pidfile != NULL) { - if (0 != remove(opts->pidfile)) { - fprintf(stderr, "%s: %s: %s\n", opts->program_name, opts->pidfile, strerror(errno)); - } - } - pv_sig_fini(state); + } else if (0 != opts->watch_pid && 0 == opts->watch_fd) { + /* "Watch all file descriptors of another process" mode. */ + retcode = pv_watchpid_loop(state); + } else if (0 != opts->watch_pid && 0 != opts->watch_fd) { + /* "Watch a specific file descriptor of another process" mode. */ + retcode = pv_watchfd_loop(state); } + /* Clear up the PID file, if one was written. */ + if (opts->pidfile != NULL) { + if (0 != remove(opts->pidfile)) { + fprintf(stderr, "%s: %s: %s\n", opts->program_name, opts->pidfile, strerror(errno)); + } + } + + /* Close down the signal handling. */ + pv_sig_fini(state); + + /* Free the internal PV state. */ pv_state_free(state); + /* Free the data from parsing the command-line arguments. */ opts_free(opts); debug("%s: %d", "exiting with status", retcode); diff --git a/src/pv/cursor.c b/src/pv/cursor.c index 84f2a13..97eea97 100644 --- a/src/pv/cursor.c +++ b/src/pv/cursor.c @@ -321,25 +321,27 @@ static int pv_crs_ipcinit(pvstate_t state, char *ttyfile, int terminalfd) return 1; } - state->crs_shmid = shmget(key, sizeof(int), 0600 | IPC_CREAT); + state->crs_shmid = shmget(key, sizeof(struct pvcursorstate_s), 0600 | IPC_CREAT); if (state->crs_shmid < 0) { debug("%s: %s", "shmget failed", strerror(errno)); pv_crs_unlock(state, terminalfd); return 1; } - state->crs_y_top = shmat(state->crs_shmid, NULL, 0); + state->crs_shared = shmat(state->crs_shmid, NULL, 0); pv_crs_ipccount(state); /* * If nobody else is attached to the shared memory segment, we're * the first, so we need to initialise the shared memory with our - * current Y cursor co-ordinate. + * current Y cursor co-ordinate and with an initial false value for + * the TOSTOP-added flag. */ if (state->crs_pvcount < 2) { state->crs_y_start = pv_crs_get_ypos(terminalfd); - *(state->crs_y_top) = state->crs_y_start; + state->crs_shared->y_topmost = state->crs_y_start; + state->crs_shared->tty_tostop_added = false; state->crs_y_lastread = state->crs_y_start; debug("%s", "we are the first to attach"); } @@ -353,7 +355,7 @@ static int pv_crs_ipcinit(pvstate_t state, char *ttyfile, int terminalfd) * to read the top Y co-ordinate from it. */ if (state->crs_pvcount > 1) { - state->crs_y_start = *(state->crs_y_top); + state->crs_y_start = state->crs_shared->y_topmost; state->crs_y_lastread = state->crs_y_start; debug("%s: %d", "not the first to attach - got top y", state->crs_y_start); } @@ -384,7 +386,7 @@ void pv_crs_init(pvstate_t state) ttyfile = ttyname(STDERR_FILENO); if (!ttyfile) { debug("%s: %s", "disabling cursor positioning because ttyname failed", strerror(errno)); - state->cursor = 0; + state->cursor = false; return; } @@ -400,6 +402,15 @@ void pv_crs_init(pvstate_t state) state->crs_noipc = true; } + /* + * If we have already set the terminal TOSTOP attribute, set the + * flag in shared memory to let the other instances know. + */ + if ((!state->crs_noipc) && state->pv_tty_tostop_added && (NULL != state->crs_shared)) { + debug("%s", "propagating local pv_tty_tostop_added true value to shared flag"); + state->crs_shared->tty_tostop_added = true; + } + /* * If we are not using IPC, then we need to get the current Y * co-ordinate. If we are using IPC, then the pv_crs_ipcinit() @@ -469,7 +480,7 @@ void pv_crs_reinit(pvstate_t state) state->crs_y_start = pv_crs_get_ypos(STDERR_FILENO); if (state->crs_y_offset < 1) - *(state->crs_y_top) = state->crs_y_start; + state->crs_shared->y_topmost = state->crs_y_start; state->crs_y_lastread = state->crs_y_start; pv_crs_unlock(state, STDERR_FILENO); @@ -492,8 +503,8 @@ void pv_crs_update(pvstate_t state, const char *str) pv_crs_reinit(state); pv_crs_ipccount(state); - if (state->crs_y_lastread != *(state->crs_y_top)) { - state->crs_y_start = *(state->crs_y_top); + if (state->crs_y_lastread != state->crs_shared->y_topmost) { + state->crs_y_start = state->crs_shared->y_topmost; state->crs_y_lastread = state->crs_y_start; } @@ -599,9 +610,23 @@ void pv_crs_fini(pvstate_t state) pv_write_retry(STDERR_FILENO, pos, strlen(pos)); + /* + * If any other "pv -c" instances have set the terminal TOSTOP + * attribute, set our local flag so pv_sig_fini() will know about + * it. + */ + if ((!state->crs_noipc) && (NULL != state->crs_shared) && state->crs_shared->tty_tostop_added) { + if (!state->pv_tty_tostop_added) { + debug("%s", "propagating shared tty_tostop_added true value to local flag"); + state->pv_tty_tostop_added = true; + } + } + + #ifdef HAVE_IPC pv_crs_ipccount(state); - (void) shmdt((void *) state->crs_y_top); + (void) shmdt(state->crs_shared); + state->crs_shared = NULL; /* * If we are the last instance detaching from the shared memory, diff --git a/src/pv/signal.c b/src/pv/signal.c index 6ae460e..093e5c2 100644 --- a/src/pv/signal.c +++ b/src/pv/signal.c @@ -10,6 +10,8 @@ #include "pv.h" #include "pv-internal.h" +#include +#include #include #include #include @@ -23,6 +25,40 @@ void pv_crs_needreinit(pvstate_t); static pvstate_t pv_sig_state = NULL; +/* + * Ensure that terminal attribute TOSTOP is set. If we have to set it, + * record that fact by setting the state boolean "pv_tty_tostop_added" to + * true, so that in pv_sig_fini() we can turn it back off again. + */ +static void pv_sig_ensure_tty_tostop() +{ + struct termios terminal_attributes; + + if (0 != tcgetattr(STDERR_FILENO, &terminal_attributes)) { + debug("%s: %s", "failed to read terminal attributes", strerror(errno)); + return; + } + + if (0 == (terminal_attributes.c_lflag & TOSTOP)) { + terminal_attributes.c_lflag |= TOSTOP; + if (0 == tcsetattr(STDERR_FILENO, TCSANOW, &terminal_attributes)) { + pv_sig_state->pv_tty_tostop_added = true; + debug("%s", "set terminal TOSTOP attribute"); + } else { + debug("%s: %s", "failed to set terminal TOSTOP attribute", strerror(errno)); + } +#if HAVE_IPC + /* + * In "-c" mode with IPC, make all "pv -c" instances aware + * that we set TOSTOP, so the last one can clear it on exit. + */ + if (pv_sig_state->cursor && (NULL != pv_sig_state->crs_shared) && (!pv_sig_state->crs_noipc)) { + pv_sig_state->crs_shared->tty_tostop_added = true; + } +#endif + } +} + /* * Handle SIGTTOU (tty output for background process) by redirecting stderr * to /dev/null, so that we can be stopped and backgrounded without messing @@ -69,41 +105,38 @@ static void pv_sig_cont( __attribute__((unused)) { struct timespec current_time; struct timespec time_spent_stopped; - struct termios t; pv_sig_state->pv_sig_newsize = 1; - /* if this SIGCONT didn't follow a SIGTSTP so we have no stop time */ - if (0 == pv_sig_state->pv_sig_tstp_time.tv_sec) { - tcgetattr(STDERR_FILENO, &t); - t.c_lflag |= TOSTOP; - tcsetattr(STDERR_FILENO, TCSANOW, &t); -#ifdef HAVE_IPC - pv_crs_needreinit(pv_sig_state); -#endif - return; + /* + * We can only make the time adjustments if this SIGCONT followed a + * SIGTSTP such that we have a stop time. + */ + if (0 != pv_sig_state->pv_sig_tstp_time.tv_sec) { + + pv_elapsedtime_read(¤t_time); + + /* time spent stopped = current time - time SIGTSTP received */ + pv_elapsedtime_subtract(&time_spent_stopped, ¤t_time, &(pv_sig_state->pv_sig_tstp_time)); + + /* add time spent stopped the total stopped-time count */ + pv_elapsedtime_add(&(pv_sig_state->pv_sig_toffset), &(pv_sig_state->pv_sig_toffset), + &time_spent_stopped); + + /* reset the SIGTSTP receipt time */ + pv_elapsedtime_zero(&(pv_sig_state->pv_sig_tstp_time)); } - pv_elapsedtime_read(¤t_time); - - /* time spent stopped = current time - time SIGTSTP received */ - pv_elapsedtime_subtract(&time_spent_stopped, ¤t_time, &(pv_sig_state->pv_sig_tstp_time)); - - /* add time spent stopped the total stopped-time count */ - pv_elapsedtime_add(&(pv_sig_state->pv_sig_toffset), &(pv_sig_state->pv_sig_toffset), &time_spent_stopped); - - /* reset the SIGTSTP receipt time */ - pv_elapsedtime_zero(&(pv_sig_state->pv_sig_tstp_time)); - + /* + * Restore the old stderr, if we had replaced it. + */ if (pv_sig_state->pv_sig_old_stderr != -1) { dup2(pv_sig_state->pv_sig_old_stderr, STDERR_FILENO); close(pv_sig_state->pv_sig_old_stderr); pv_sig_state->pv_sig_old_stderr = -1; } - tcgetattr(STDERR_FILENO, &t); - t.c_lflag |= TOSTOP; - tcsetattr(STDERR_FILENO, TCSANOW, &t); + pv_sig_ensure_tty_tostop(); #ifdef HAVE_IPC pv_crs_needreinit(pv_sig_state); @@ -208,14 +241,26 @@ void pv_sig_init(pvstate_t state) sigemptyset(&(sa.sa_mask)); sa.sa_flags = 0; sigaction(SIGTERM, &sa, &(pv_sig_state->pv_sig_old_sigterm)); + + /* + * Ensure that the TOSTOP terminal attribute is set, so that a + * SIGTTOU signal will be raised if we try to write to the terminal + * while backgrounded (see the SIGTTOU handler above). + */ + pv_sig_ensure_tty_tostop(); } /* - * Shut down signal handling. + * Shut down signal handling. If we had set the TOSTOP terminal attribute, + * and we're in the foreground, also turn that off (though if we're in + * cursor "-c" mode, only do that if we're the last PV instance, otherwise + * leave the terminal alone). */ void pv_sig_fini( __attribute__((unused)) pvstate_t state) { + bool need_to_clear_tostop = false; + sigaction(SIGPIPE, &(pv_sig_state->pv_sig_old_sigpipe), NULL); sigaction(SIGTTOU, &(pv_sig_state->pv_sig_old_sigttou), NULL); sigaction(SIGTSTP, &(pv_sig_state->pv_sig_old_sigtstp), NULL); @@ -224,6 +269,49 @@ void pv_sig_fini( __attribute__((unused)) pvstate_t state) sigaction(SIGINT, &(pv_sig_state->pv_sig_old_sigint), NULL); sigaction(SIGHUP, &(pv_sig_state->pv_sig_old_sighup), NULL); sigaction(SIGTERM, &(pv_sig_state->pv_sig_old_sigterm), NULL); + + need_to_clear_tostop = pv_sig_state->pv_tty_tostop_added; + + if (pv_sig_state->cursor) { +#ifdef HAVE_IPC + /* + * We won't clear TOSTOP if other "pv -c" instances + * were still running when pv_crs_fini() ran. + * + * TODO: we need a better way to determine if we're the last + * "pv" left. + */ + if (pv_sig_state->cursor && pv_sig_state->crs_pvcount > 1) { + need_to_clear_tostop = false; + } +#else /* !HAVE_IPC */ + /* + * Without IPC we can't tell whether the other "pv -c" + * instances in the pipeline have finished so we will just + * have to clear TOSTOP anyway. + */ +#endif /* !HAVE_IPC */ + } + + debug("%s=%s", "need_to_clear_tostop", need_to_clear_tostop ? "true" : "false"); + + if (need_to_clear_tostop && pv_in_foreground()) { + struct termios terminal_attributes; + + debug("%s", "about to to clear TOSTOP terminal attribute if it is set"); + + tcgetattr(STDERR_FILENO, &terminal_attributes); + if (0 != (terminal_attributes.c_lflag & TOSTOP)) { + terminal_attributes.c_lflag -= TOSTOP; + if (0 == tcsetattr(STDERR_FILENO, TCSANOW, &terminal_attributes)) { + debug("%s", "cleared TOSTOP terminal attribute"); + } else { + debug("%s: %s", "failed to clear TOSTOP terminal attribute", strerror(errno)); + } + } + + pv_sig_state->pv_tty_tostop_added = false; + } } @@ -274,7 +362,6 @@ void pv_sig_allowpause(void) void pv_sig_checkbg(void) { static time_t next_check = 0; - struct termios t; if (time(NULL) < next_check) return; @@ -288,10 +375,7 @@ void pv_sig_checkbg(void) close(pv_sig_state->pv_sig_old_stderr); pv_sig_state->pv_sig_old_stderr = -1; - tcgetattr(STDERR_FILENO, &t); - t.c_lflag |= TOSTOP; - tcsetattr(STDERR_FILENO, TCSANOW, &t); - + pv_sig_ensure_tty_tostop(); #ifdef HAVE_IPC pv_crs_needreinit(pv_sig_state); #endif diff --git a/src/pv/transfer.c b/src/pv/transfer.c index ceeaf6e..6f5ff71 100644 --- a/src/pv/transfer.c +++ b/src/pv/transfer.c @@ -409,7 +409,7 @@ static int pv__transfer_read(pvstate_t state, int fd, int *eof_in, int *eof_out, */ if (state->error_skip_block > 0) { amount_to_skip = state->error_skip_block; - } else { + } else { if (state->read_errors_in_a_row < 10) { amount_to_skip = state->read_errors_in_a_row < 5 ? 1 : 2; } else if (state->read_errors_in_a_row < 20) {