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

@@ -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

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

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 */

src/main/main.c

@@ -19,7 +19,6 @@
 #include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
-#include <termios.h>
 #include <sys/ioctl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -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);

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,

src/pv/signal.c

@@ -10,6 +10,8 @@
 #include "pv.h"
 #include "pv-internal.h"
 
+#include <string.h>
+#include <errno.h>
 #include <signal.h>
 #include <termios.h>
 #include <unistd.h>
@@ -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(&current_time);
+
+		/* time spent stopped = current time - time SIGTSTP received */
+		pv_elapsedtime_subtract(&time_spent_stopped, &current_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(&current_time);
-
-	/* time spent stopped = current time - time SIGTSTP received */
-	pv_elapsedtime_subtract(&time_spent_stopped, &current_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

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) {