From 09e4f8ff4214b951579e05101371c1641649d168 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 29 Jun 2019 15:58:36 -0700 Subject: [PATCH] Refactor how the terminal is transferred to jobs Centralize the logic around when a job acquires the terminal. --- src/exec.cpp | 11 ++--------- src/proc.cpp | 48 ++++++++++++++++++++++++------------------------ src/proc.h | 11 ++++++----- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index b15398c86..c6ac1b634 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -117,13 +117,6 @@ char *get_interpreter(const char *command, char *interpreter, size_t buff_size) return NULL; } -/// Assign the terminal to a job. -static void maybe_assign_terminal(const job_t *j) { - if (j->wants_terminal() && j->is_foreground()) { - terminal_give_to_job(j, false /*new job, so not continuing*/); - } -} - /// This function is executed by the child process created by a call to fork(). It should be called /// after \c child_setup_process. It calls execve to replace the fish process image with the command /// specified in \c p. It never returns. Called in a forked child! Do not allocate memory, etc. @@ -472,7 +465,7 @@ static bool fork_child_for_process(const std::shared_ptr &job, process_t p->pid = pid; on_process_created(job, p->pid); set_child_group(job.get(), p->pid); - maybe_assign_terminal(job.get()); + terminal_maybe_give_to_job(job.get(), false); return true; } @@ -763,7 +756,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr } #endif - maybe_assign_terminal(j.get()); + terminal_maybe_give_to_job(j.get(), false); } else #endif { diff --git a/src/proc.cpp b/src/proc.cpp index c3e055864..2c780abc3 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -211,12 +211,6 @@ static int64_t next_proc_id() { internal_proc_t::internal_proc_t() : internal_proc_id_(next_proc_id()) {} -static void mark_job_complete(const job_t *j) { - for (auto &p : j->processes) { - p->completed = 1; - } -} - void job_mark_process_as_failed(const std::shared_ptr &job, const process_t *failed_proc) { // The given process failed to even lift off (e.g. posix_spawn failed) and so doesn't have a // valid pid. Mark it and everything after it as dead. @@ -684,10 +678,22 @@ void proc_update_jiffies(parser_t &parser) { // 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. -bool terminal_give_to_job(const job_t *j, bool restore_attrs) { +int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { + enum { notneeded = 0, success = 1, error = -1 }; + + if (!j->wants_terminal() || !j->is_foreground()) { + // We don't want the job. + return notneeded; + } + if (j->pgid == 0) { FLOG(proc_termowner, L"terminal_give_to_job() returning early due to no process group"); - return true; + return notneeded; + } + + // If we are continuing, ensure that stdin is marked as blocking first (issue #176). + if (continuing_from_stopped) { + make_fd_blocking(STDIN_FILENO); } // It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no @@ -749,7 +755,7 @@ bool terminal_give_to_job(const job_t *j, bool restore_attrs) { 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"); - return false; + return error; } if (pgroup_terminated) { @@ -759,15 +765,14 @@ bool terminal_give_to_job(const job_t *j, bool restore_attrs) { // process in 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); - return true; + return notneeded; } break; } } - if (restore_attrs) { + if (continuing_from_stopped) { auto result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes); if (result == -1) { // No need to test for EINTR and retry since we have blocked all signals @@ -778,11 +783,11 @@ bool terminal_give_to_job(const job_t *j, bool restore_attrs) { debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id, j->preview().c_str()); wperror(L"tcsetattr"); - return false; + return error; } } - return true; + return success; } pid_t terminal_acquire_before_builtin(int job_pgid) { @@ -857,17 +862,12 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se }); if (!is_completed()) { - if (wants_terminal() && is_foreground()) { - // Put the job into the foreground and give it control of the terminal. - // Hack: ensure that stdin is marked as blocking first (issue #176). - make_fd_blocking(STDIN_FILENO); - if (!terminal_give_to_job(this, send_sigcont)) { - // This scenario has always returned without any error handling. Presumably that is - // OK. - return; - } - term_transferred = true; + int transfer = terminal_maybe_give_to_job(this, send_sigcont); + if (transfer < 0) { + // terminal_maybe_give_to_job prints an error. + return; } + term_transferred = (transfer > 0); // If both requested and necessary, send the job a continue signal. if (send_sigcont) { diff --git a/src/proc.h b/src/proc.h index faa906f84..2133fe160 100644 --- a/src/proc.h +++ b/src/proc.h @@ -515,13 +515,14 @@ bool is_within_fish_initialization(); /// Terminate all background jobs void hup_background_jobs(const parser_t &parser); -/// Give ownership of the terminal to the specified job. +/// Give ownership of the terminal to the specified job, if it wants it. /// /// \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); +/// \param continuing_from_stopped 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. +/// \return 1 if transferred, 0 if no transfer was necessary, -1 on error. +int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped); /// 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.