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

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

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

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

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;
 		}
 	}

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--;
 	}
 

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