From f30ce21aaa91a3ea1a3a871eed40b2971c551455 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 17 Jul 2020 14:01:03 -0700 Subject: [PATCH] terminal_maybe_give_to_job to operate on groups, not jobs Assigning the tty is really a function of a job group, not an individual job. Reflect that in terminal_maybe_give_to_job_group and also terminal_return_from_job_group. --- src/exec.cpp | 4 ++-- src/parse_execution.cpp | 1 - src/proc.cpp | 30 ++++++++++++++++-------------- src/proc.h | 16 ++++++++-------- 4 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 9ef1a6464..1717a12da 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -347,7 +347,7 @@ static bool fork_child_for_process(const std::shared_ptr &job, process_t if (int err = execute_setpgid(p->pid, pgid, true /* is parent */)) { if (err != EACCES) report_setpgid_error(err, pgid, job.get(), p); } - terminal_maybe_give_to_job(job.get(), false); + terminal_maybe_give_to_job_group(job->group.get(), false); return true; } @@ -546,7 +546,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr // therefore the parent may not have seen it be set yet. // Ensure it gets set. See #4715, also https://github.com/Microsoft/WSL/issues/2997. execute_setpgid(p->pid, pgid, true /* is parent */); - terminal_maybe_give_to_job(j.get(), false); + terminal_maybe_give_to_job_group(j->group.get(), false); } else #endif { diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 9cf937fc3..824478585 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1308,7 +1308,6 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo props.job_control = wants_job_control; shared_ptr job = std::make_shared(props); - job->tmodes = tmodes; // We are about to populate a job. One possible argument to the job is a command substitution // which may be interested in the job that's populating it, via '--on-job-exit caller'. Record diff --git a/src/proc.cpp b/src/proc.cpp index a268ede63..7dd729054 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -801,17 +801,17 @@ 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. -int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { +int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from_stopped) { enum { notneeded = 0, success = 1, error = -1 }; - if (!j->group->should_claim_terminal()) { + if (!jg->should_claim_terminal()) { // The job doesn't want the terminal. return notneeded; } // Get the pgid; we may not have one. pid_t pgid{}; - if (auto mpgid = j->get_pgid()) { + if (auto mpgid = jg->get_pgid()) { pgid = *mpgid; } else { FLOG(proc_termowner, L"terminal_give_to_job() returning early due to no process group"); @@ -888,7 +888,7 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { return notneeded; } else { FLOGF(warning, _(L"Could not send job %d ('%ls') with pgid %d to foreground"), - j->job_id(), j->command_wcstr(), pgid); + jg->get_id(), jg->get_command().c_str(), pgid); wperror(L"tcsetpgrp"); return error; } @@ -906,8 +906,8 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { break; } - if (continuing_from_stopped) { - auto result = tcsetattr(STDIN_FILENO, TCSADRAIN, &j->tmodes); + if (continuing_from_stopped && jg->tmodes.has_value()) { + int result = tcsetattr(STDIN_FILENO, TCSADRAIN, &jg->tmodes.value()); if (result == -1) { wperror(L"tcsetattr"); } @@ -916,11 +916,11 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) { return success; } -/// Returns control of the terminal to the shell, and saves the terminal attribute state to the job, -/// so that we can restore the terminal ownership to the job at a later time. -static bool terminal_return_from_job(job_t *j, int restore_attrs) { +/// Returns control of the terminal to the shell, and saves the terminal attribute state to the job +/// group, so that we can restore the terminal ownership to the job at a later time. +static bool terminal_return_from_job_group(job_group_t *jg, bool restore_attrs) { errno = 0; - auto pgid = j->get_pgid(); + auto pgid = jg->get_pgid(); if (!pgid.has_value()) { FLOG(proc_pgroup, "terminal_return_from_job() returning early due to no process group"); return true; @@ -935,7 +935,8 @@ static bool terminal_return_from_job(job_t *j, int restore_attrs) { } // Save jobs terminal modes. - if (tcgetattr(STDIN_FILENO, &j->tmodes)) { + struct termios tmodes {}; + if (tcgetattr(STDIN_FILENO, &tmodes)) { // If it's not a tty, it's not a tty, and there are no attributes to save (or restore) if (errno == ENOTTY) return false; if (errno == EIO) redirect_tty_output(); @@ -943,6 +944,7 @@ static bool terminal_return_from_job(job_t *j, int restore_attrs) { wperror(L"tcgetattr"); return false; } + jg->tmodes = tmodes; // Need to restore the terminal's attributes or `bind \cF fg` will put the // terminal into a broken state (until "enter" is pressed). @@ -974,17 +976,17 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se // Make sure we retake control of the terminal before leaving this function. bool term_transferred = false; - cleanup_t take_term_back([&]() { + cleanup_t take_term_back([&] { if (term_transferred && reclaim_foreground_pgrp) { // Only restore terminal attrs if we're continuing a job. See: // https://github.com/fish-shell/fish-shell/issues/121 // https://github.com/fish-shell/fish-shell/issues/2114 - terminal_return_from_job(this, send_sigcont); + terminal_return_from_job_group(this->group.get(), send_sigcont); } }); if (!is_completed()) { - int transfer = terminal_maybe_give_to_job(this, send_sigcont); + int transfer = terminal_maybe_give_to_job_group(this->group.get(), send_sigcont); if (transfer < 0) { // terminal_maybe_give_to_job prints an error. return; diff --git a/src/proc.h b/src/proc.h index af571b092..77ceeb761 100644 --- a/src/proc.h +++ b/src/proc.h @@ -230,6 +230,11 @@ class job_group_t { ~job_group_t(); + /// If set, the saved terminal modes of this job. This needs to be saved so that we can restore + /// the terminal to the same state after temporarily taking control over the terminal when a job + /// stops. + maybe_t tmodes{}; + private: // The pgid to assign to jobs, or none if not yet set. maybe_t pgid_{}; @@ -505,11 +510,6 @@ class job_t { /// A non-user-visible, never-recycled job ID. const internal_job_id_t internal_job_id; - /// The saved terminal modes of this job. This needs to be saved so that we can restore the - /// terminal to the same state after temporarily taking control over the terminal when a job - /// stops. - struct termios tmodes {}; - /// Flags associated with the job. struct flags_t { /// Whether the specified job is completely constructed: every process in the job has been @@ -667,14 +667,14 @@ bool is_within_fish_initialization(); /// Send SIGHUP to the list \p jobs, excepting those which are in fish's pgroup. void hup_jobs(const job_list_t &jobs); -/// Give ownership of the terminal to the specified job, if it wants it. +/// Give ownership of the terminal to the specified job group, if it wants it. /// -/// \param j The job to give the terminal to. +/// \param jg The job group to give the terminal to. /// \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); +int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from_stopped); /// Add a pid to the list of pids we wait on even though they are not associated with any jobs. /// Used to avoid zombie processes after disown.