Clean up terminal_give_to_job()

* Use the newly-introduced signal_block_t RAII wrapper
* Remove EINTR loops as all signals are blocked
* Clean up control flow thanks to RAII wrappers
* Rename parameter to clarify what it does and update docs accordingly
* Update outdated comments referencing SIGSTOP code that was removed a
  long time ago.
* Remove no-op CHECK_BLOCK() call

src/proc.cpp

@@ -695,10 +695,9 @@ static int select_try(job_t *j) {
         if (io->io_mode == IO_BUFFER) {
             const io_pipe_t *io_pipe = static_cast<const io_pipe_t *>(io);
             int fd = io_pipe->pipe_fd[0];
-            // fwprintf( stderr, L"fd %d on job %ls\n", fd, j->command );
             FD_SET(fd, &fds);
-            maxfd = maxi(maxfd, fd);
+            maxfd = std::max(maxfd, fd);
-            debug(4, L"select_try on %d", fd);
+            debug(4, L"select_try on fd %d", fd);
         }
     }
 
@@ -754,19 +753,17 @@ static void read_try(job_t *j) {
     }
 }
 
-/// Give ownership of the terminal to the specified job.
+// Return control of the terminal to a job's process group. restore_attrs is true if we are restoring
-///
+// a previously-stopped job, in which case we need to restore terminal attributes.
-/// \param j The job to give the terminal to.
+bool terminal_give_to_job(const job_t *j, bool restore_attrs) {
-/// \param cont If this variable is set, we are giving back control to a job that has previously
-/// been stopped. In that case, we need to set the terminal attributes to those saved in the job.
-bool terminal_give_to_job(const job_t *j, bool cont) {
-    errno = 0;
     if (j->pgid == 0) {
         debug(2, "terminal_give_to_job() returning early due to no process group");
         return true;
     }
 
-    signal_block();
+    // RAII wrappers must have a name so that their scope is tied to the function as it is legal for
+    // the compiler to construct and then immediately deconstruct unnamed objects otherwise.
+    signal_block_t signal_block;
 
     // It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no
     // longer the controlling process group for the terminal and no longer have permission to set
@@ -795,11 +792,11 @@ bool terminal_give_to_job(const job_t *j, bool cont) {
         // guarantee the process isn't going to exit while we wait (which would cause us to possibly
         // block indefinitely).
         while (tcsetpgrp(STDIN_FILENO, j->pgid) != 0) {
-            debug(3, "tcsetpgrp failed");
+            debug(3, "tcsetpgrp failed: %d", errno);
+
             bool pgroup_terminated = false;
-            if (errno == EINTR) {
+            // No need to test for EINTR as we are blocking signals
-                ;  // Always retry on EINTR, see comments in tcsetattr EINTR code below.
+            if (errno == EINVAL) {
-            } else if (errno == EINVAL) {
                 // OS X returns EINVAL if the process group no longer lives. Probably other OSes,
                 // too. Unlike EPERM below, EINVAL can only happen if the process group has
                 // terminated.
@@ -818,49 +815,47 @@ bool terminal_give_to_job(const job_t *j, bool cont) {
                     // Debug the original tcsetpgrp error (not the waitpid errno) to the log, and
                     // then retry until not EPERM or the process group has exited.
                     debug(2, L"terminal_give_to_job(): EPERM.\n", j->pgid);
+                    continue;
                 }
             } else {
-                if (errno == ENOTTY) redirect_tty_output();
+                if (errno == ENOTTY) {
-                debug(1, _(L"Could not send job %d ('%ls') with pgid %d to foreground"), j->job_id,
+                    redirect_tty_output();
-                      j->command_wcstr(), j->pgid);
+                }
+                debug(1, _(L"Could not send job %d ('%ls') with pgid %d to foreground"),
+                            j->job_id, j->command_wcstr(), j->pgid);
                 wperror(L"tcsetpgrp");
-                signal_unblock();
                 return false;
             }
 
             if (pgroup_terminated) {
-                // All processes in the process group has exited. Since we force all child procs to
+                // All processes in the process group has exited.
-                // SIGSTOP on startup, the only way that can happen is if the very last process in
+                // Since we delay reaping any processes in a process group until all members of that
-                // the group terminated, and didn't need to access the terminal, otherwise it would
+                // job/group have been started, the only way this can happen is if the very last process in
-                // have hung waiting for terminal IO (SIGTTIN). We can ignore this.
+                // the group terminated and didn't need to access the terminal, otherwise it would
+                // have hung waiting for terminal IO (SIGTTIN). We can safely ignore this.
                 debug(3, L"tcsetpgrp called but process group %d has terminated.\n", j->pgid);
                 mark_job_complete(j);
-                signal_unblock();
                 return true;
             }
+
+            break;
         }
     }
 
-    if (cont) {
+    if (restore_attrs) {
-        int result = -1;
+        auto result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes);
-        // TODO: Remove this EINTR loop since we have blocked all signals and thus cannot be
-        // interrupted. I'm leaving it in place because all of the logic involving controlling
-        // terminal management is more than a little opaque and smacks of voodoo programming.
-        errno = EINTR;
-        while (result == -1 && errno == EINTR) {
-            result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes);
-        }
         if (result == -1) {
-            if (errno == ENOTTY) redirect_tty_output();
+            // No need to test for EINTR and retry since we have blocked all signals
-            debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id,
+            if (errno == ENOTTY) {
-                  j->command_wcstr());
+                redirect_tty_output();
+            }
+
+            debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->preview().c_str());
             wperror(L"tcsetattr");
-            signal_unblock();
             return false;
         }
     }
 
-    signal_unblock();
     return true;
 }
 
@@ -925,15 +920,14 @@ void job_t::continue_job(bool cont) {
     promote();
     set_flag(job_flag_t::NOTIFIED, false);
 
-    CHECK_BLOCK();
+    debug(4, L"%ls job %d, gid %d (%ls), %ls, %ls", cont ? L"Continue" : L"Start", job_id,
-    debug(4, L"%ls job %d, gid %d (%ls), %ls, %ls", cont ? L"Continue" : L"Start", job_id, pgid,
+          pgid, command_wcstr(), is_completed() ? L"COMPLETED" : L"UNCOMPLETED",
-          command_wcstr(), is_completed() ? L"COMPLETED" : L"UNCOMPLETED",
           is_interactive ? L"INTERACTIVE" : L"NON-INTERACTIVE");
 
     if (!is_completed()) {
         if (get_flag(job_flag_t::TERMINAL) && is_foreground()) {
-            // Put the job into the foreground. Hack: ensure that stdin is marked as blocking first
+            // Put the job into the foreground.
-            // (issue #176).
+            // Hack: ensure that stdin is marked as blocking first (issue #176).
             make_fd_blocking(STDIN_FILENO);
             if (!terminal_give_to_job(this, cont)) return;
         }

src/proc.h

@@ -386,7 +386,12 @@ int proc_format_status(int status);
 /// Wait for any process finishing.
 pid_t proc_wait_any();
 
-bool terminal_give_to_job(const job_t *j, bool cont);
+/// Give ownership of the terminal to the specified job.
+///
+/// \param j The job to give the terminal to.
+/// \param restore_attrs If this variable is set, we are giving back control to a job that was previously
+/// stopped. In that case, we need to set the terminal attributes to those saved in the job.
+bool terminal_give_to_job(const job_t *j, bool restore_attrs);
 
 /// Given that we are about to run a builtin, acquire the terminal if it is owned by the given job.
 /// Returns the pid to restore after running the builtin, or -1 if there is no pid to restore.