diff --git a/src/exec.cpp b/src/exec.cpp index c6658df4d..1636058d8 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -222,14 +222,10 @@ static bool can_use_posix_spawn_for_job(const std::shared_ptr &job, for (const auto &action : dup2s.get_actions()) { if (action.src == action.target) return false; } - if (job->wants_job_control()) { //!OCLINT(collapsible if statements) - // We are going to use job control; therefore when we launch this job it will get its own - // process group ID. But will it be foregrounded? - if (job->group->should_claim_terminal()) { - // It will be foregrounded, so we will call tcsetpgrp(), therefore do not use - // posix_spawn. - return false; - } + if (job->group->wants_terminal()) { + // This job will be foregrounded, so we will call tcsetpgrp(), therefore do not use + // posix_spawn. + return false; } return true; } @@ -263,16 +259,6 @@ static void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_i } } -/// If our pgroup assignment mode wants us to use the first external proc, then apply it here. -/// \returns the job's pgid, which should always be set to something valid after this call. -static pid_t maybe_assign_pgid_from_child(const std::shared_ptr &j, pid_t child_pid) { - auto &jt = j->group; - if (jt->needs_pgid_assignment()) { - jt->set_pgid(child_pid); - } - return *jt->get_pgid(); -} - /// Construct an internal process for the process p. In the background, write the data \p outdata to /// stdout and \p errdata to stderr, respecting the io chain \p ios. For example if target_fd is 1 /// (stdout), and there is a dup2 3->1, then we need to write to fd 3. Then exit the internal @@ -411,46 +397,40 @@ bool blocked_signals_for_job(const job_t &job, sigset_t *sigmask) { static launch_result_t fork_child_for_process(const std::shared_ptr &job, process_t *p, const dup2_list_t &dup2s, const char *fork_type, const std::function &child_action) { - assert(!job->group->is_internal() && "Internal groups should never need to fork"); // Decide if we want to job to control the tty. // If so we need to get our pgroup; if not we don't need the pgroup. - bool claim_tty = job->group->should_claim_terminal(); + bool claim_tty = job->group->wants_terminal(); pid_t fish_pgrp = claim_tty ? getpgrp() : INVALID_PID; pid_t pid = execute_fork(); - if (pid == 0) { - // This is the child process. Setup redirections, print correct output to - // stdout and stderr, and then exit. - p->pid = getpid(); - pid_t pgid = maybe_assign_pgid_from_child(job, p->pid); + if (pid < 0) { + return launch_result_t::failed; + } + const bool is_parent = (pid > 0); - // The child attempts to join the pgroup. - if (int err = execute_setpgid(p->pid, pgid, false /* not parent */)) { - report_setpgid_error(err, false /* is_parent */, pgid, job.get(), p); + // Record the pgroup if this is the leader. + // Both parent and child attempt to send the process to its new group, to resolve the race. + p->pid = is_parent ? pid : getpid(); + if (p->leads_pgrp) { + job->group->set_pgid(p->pid); + } + if (auto pgid = job->group->get_pgid()) { + if (int err = execute_setpgid(p->pid, *pgid, is_parent)) { + report_setpgid_error(err, is_parent, *pgid, job.get(), p); } - child_setup_process(claim_tty ? pgid : INVALID_PID, fish_pgrp, *job, true, dup2s); + } + + if (!is_parent) { + // Child process. + child_setup_process(claim_tty ? *job->group->get_pgid() : INVALID_PID, fish_pgrp, *job, + true, dup2s); child_action(); DIE("Child process returned control to fork_child lambda!"); } - if (pid < 0) { - return launch_result_t::failed; - } - - // This is the parent process. Store away information on the child, and - // possibly give it control over the terminal. s_fork_count++; FLOGF(exec_fork, L"Fork #%d, pid %d: %s for '%ls'", int(s_fork_count), pid, fork_type, p->argv0()); - - p->pid = pid; - pid_t pgid = maybe_assign_pgid_from_child(job, p->pid); - - // The parent attempts to send the child to its pgroup. - // EACCESS is an expected benign error as the child may have called exec(). - if (int err = execute_setpgid(p->pid, pgid, true /* is parent */)) { - if (err != EACCES) report_setpgid_error(err, true /* is_parent */, pgid, job.get(), p); - } terminal_maybe_give_to_job_group(job->group.get(), false); return launch_result_t::ok; } @@ -573,13 +553,14 @@ static launch_result_t exec_external_command(parser_t &parser, const std::shared // these are all things do_fork() takes care of normally (for forked processes): p->pid = *pid; - pid_t pgid = maybe_assign_pgid_from_child(j, p->pid); - - // posix_spawn should in principle set the pgid before returning. - // In glibc, posix_spawn uses fork() and the pgid group is set on the child side; - // 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 */); + if (p->leads_pgrp) { + j->group->set_pgid(p->pid); + // posix_spawn should in principle set the pgid before returning. + // In glibc, posix_spawn uses fork() and the pgid group is set on the child side; + // 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, p->pid, true /* is parent */); + } terminal_maybe_give_to_job_group(j->group.get(), false); return launch_result_t::ok; } else @@ -1124,8 +1105,7 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const io_chain_t &bl } } - FLOGF(exec_job_exec, L"Executed job %d from command '%ls' with pgrp %d", j->job_id(), - j->command_wcstr(), j->get_pgid() ? *j->get_pgid() : -2); + FLOGF(exec_job_exec, L"Executed job %d from command '%ls'", j->job_id(), j->command_wcstr()); j->mark_constructed(); diff --git a/src/job_group.cpp b/src/job_group.cpp index 2df7f0aba..8576906fe 100644 --- a/src/job_group.cpp +++ b/src/job_group.cpp @@ -31,77 +31,40 @@ static void release_job_id(job_id_t jid) { consumed_job_ids->erase(where); } +job_group_t::job_group_t(wcstring command, cancellation_group_ref_t cg, job_id_t job_id, + bool job_control, bool wants_terminal) + : cancel_group(std::move(cg)), + job_control_(job_control), + wants_terminal_(wants_terminal), + command_(std::move(command)), + job_id_(job_id) {} + job_group_t::~job_group_t() { - if (props_.job_id > 0) { - release_job_id(props_.job_id); + if (job_id_ > 0) { + release_job_id(job_id_); } } +// static +job_group_ref_t job_group_t::create(wcstring command, cancellation_group_ref_t cg, + bool wants_job_id) { + job_id_t jid = wants_job_id ? acquire_job_id() : 0; + return job_group_ref_t(new job_group_t(std::move(command), std::move(cg), jid)); +} + +// static +job_group_ref_t job_group_t::create_with_job_control(wcstring command, cancellation_group_ref_t cg, + bool wants_terminal) { + return job_group_ref_t(new job_group_t(std::move(command), std::move(cg), acquire_job_id(), + true /* job_control */, wants_terminal)); +} + void job_group_t::set_pgid(pid_t pgid) { // Note we need not be concerned about thread safety. job_groups are intended to be shared - // across threads, but their pgid should always have been set beforehand. - assert(needs_pgid_assignment() && "We should not be setting a pgid"); - assert(pgid >= 0 && "Invalid pgid"); + // across threads, but any pgid should always have been set beforehand, since it's set + // immediately after thfe first process launches. + assert(pgid >= 0 && "invalid pgid"); + assert(wants_job_control() && "should not set a pgid for this group"); + assert(!pgid_.has_value() && "pgid already set"); pgid_ = pgid; } - -maybe_t job_group_t::get_pgid() const { return pgid_; } - -// static -job_group_ref_t job_group_t::resolve_group_for_job(const job_t &job, - const cancellation_group_ref_t &cancel_group, - const job_group_ref_t &proposed) { - assert(!job.group && "Job already has a group"); - assert(cancel_group && "Null cancel group"); - // Note there's three cases to consider: - // nullptr -> this is a root job, there is no inherited job group - // internal -> the parent is running as part of a simple function execution - // We may need to create a new job group if we are going to fork. - // non-internal -> we are running as part of a real pipeline - // Decide if this job can use an internal group. - // This is true if it's a simple foreground execution of an internal proc. - bool initial_bg = job.is_initially_background(); - bool first_proc_internal = job.processes.front()->is_internal(); - bool can_use_internal = - !initial_bg && job.processes.size() == 1 && job.processes.front()->is_internal(); - - bool needs_new_group = false; - if (!proposed) { - // We don't have a group yet. - needs_new_group = true; - } else if (initial_bg) { - // Background jobs always get a new group. - needs_new_group = true; - } else if (proposed->is_internal() && !can_use_internal) { - // We cannot use the internal group for this job. - needs_new_group = true; - } - - if (!needs_new_group) return proposed; - - // We share a cancel group unless we are a background job. - // For example, if we write "begin ; true ; sleep 1 &; end" the `begin` and `true` should cancel - // together, but the `sleep` should not. - cancellation_group_ref_t resolved_cg = - initial_bg ? cancellation_group_t::create() : cancel_group; - - properties_t props{}; - props.job_control = job.wants_job_control(); - props.wants_terminal = job.wants_job_control() && !job.from_event_handler(); - props.is_internal = can_use_internal; - props.job_id = can_use_internal ? -1 : acquire_job_id(); - - job_group_ref_t result{new job_group_t(props, resolved_cg, job.command())}; - - // Mark if it's foreground. - result->set_is_foreground(!initial_bg); - - // Perhaps this job should immediately live in fish's pgroup. - // There's two reasons why it may be so: - // 1. The job doesn't need job control. - // 2. The first process in the job is internal to fish; this needs to own the tty. - if (!can_use_internal && (!props.job_control || first_proc_internal)) { - result->set_pgid(getpgrp()); - } - return result; -} diff --git a/src/job_group.h b/src/job_group.h index 92b578d70..0a10027e0 100644 --- a/src/job_group.h +++ b/src/job_group.h @@ -64,20 +64,11 @@ using job_group_ref_t = std::shared_ptr; class job_group_t { public: - /// Set the pgid for this job group, latching it to this value. - /// The pgid should not already have been set. - /// Of course this does not keep the pgid alive by itself. - /// An internal job group does not have a pgid and it is an error to set it. - void set_pgid(pid_t pgid); + /// \return whether this group wants job control. + bool wants_job_control() const { return job_control_; } - /// Get the pgid, or none() if it has not been set. - maybe_t get_pgid() const; - - /// \return whether we want job control - bool wants_job_control() const { return props_.job_control; } - - /// \return whether this is an internal group. - bool is_internal() const { return props_.is_internal; } + /// \return if this job group should own the terminal when it runs. + bool wants_terminal() const { return wants_terminal_ && is_foreground(); } /// \return whether we are currently the foreground group. bool is_foreground() const { return is_foreground_; } @@ -85,82 +76,70 @@ class job_group_t { /// Mark whether we are in the foreground. void set_is_foreground(bool flag) { is_foreground_ = flag; } - /// \return if this job group should own the terminal when it runs. - bool should_claim_terminal() const { return props_.wants_terminal && is_foreground(); } - - /// \return whether this job group is awaiting a pgid. - /// This is true for non-internal trees that don't already have a pgid. - bool needs_pgid_assignment() const { return !props_.is_internal && !pgid_.has_value(); } + /// \return the command which produced this job tree. + const wcstring &get_command() const { return command_; } /// \return the job ID, or -1 if none. - job_id_t get_id() const { return props_.job_id; } + job_id_t get_job_id() const { return job_id_; } + + /// \return whether we have a valid job ID. "Simple block" groups like function calls do not. + bool has_job_id() const { return job_id_ > 0; } /// Get the cancel signal, or 0 if none. int get_cancel_signal() const { return cancel_group->get_cancel_signal(); } - /// \return the command which produced this job tree. - const wcstring &get_command() const { return command_; } - /// Mark that a process in this group got a signal, and so should cancel. void cancel_with_signal(int sig) { cancel_group->cancel_with_signal(sig); } - /// Mark the root as constructed. - /// This is used to avoid reaping a process group leader while there are still procs that may - /// want to enter its group. - void mark_root_constructed() { root_constructed_ = true; }; - bool is_root_constructed() const { return root_constructed_; } - - /// Given a job and a proposed job group (possibly null), return a group for the job. - /// The proposed group is the group from the parent job, or null if this is a root. - /// This never returns null. - static job_group_ref_t resolve_group_for_job(const job_t &job, - const cancellation_group_ref_t &cancel_group, - const job_group_ref_t &proposed_group); - - ~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{}; - /// The cancellation group. This is never null. const cancellation_group_ref_t cancel_group{}; + /// 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 when resuming a stopped job. + maybe_t tmodes{}; + + /// Set the pgid for this job group, latching it to this value. + /// This should only be called if job control is active for this group. + /// The pgid should not already have been set, and should be different from fish's pgid. + /// Of course this does not keep the pgid alive by itself. + void set_pgid(pid_t pgid); + + /// Get the pgid. This never returns fish's pgid. + maybe_t get_pgid() const { return pgid_; } + + /// Construct a group for a job that will live internal to fish, optionally claiming a job ID. + static job_group_ref_t create(wcstring command, cancellation_group_ref_t cg, bool wants_job_id); + + /// Construct a group for a job which will assign its first process as pgroup leader. + static job_group_ref_t create_with_job_control(wcstring command, cancellation_group_ref_t cg, + bool wants_terminal); + + ~job_group_t(); + private: - // The pgid to assign to jobs, or none if not yet set. - maybe_t pgid_{}; + job_group_t(wcstring command, cancellation_group_ref_t cg, job_id_t job_id, + bool job_control = false, bool wants_terminal = false); - // Set of properties, which are constant. - struct properties_t { - // Whether jobs in this group should have job control. - bool job_control{}; + // Whether job control is enabled. + // If this is set, then the first process in the root job must be external. + // It will become the process group leader. + const bool job_control_; - // Whether we should claim the terminal when we run in the foreground. - // TODO: this is effectively the same as job control, rationalize this. - bool wants_terminal{}; - - // Whether we are an internal job group. - bool is_internal{}; - - // The job ID of this group. - job_id_t job_id{}; - }; - const properties_t props_; - - // The original command which produced this job tree. - const wcstring command_; + // Whether we should tcsetpgrp to the job when it runs in the foreground. + const bool wants_terminal_; // Whether we are in the foreground, meaning that the user is waiting for this. relaxed_atomic_bool_t is_foreground_{}; - // Whether the root job is constructed. If not, we cannot reap it yet. - relaxed_atomic_bool_t root_constructed_{}; + // The pgid leading our group. This is only ever set if job_control_ is true. + // This is never fish's pgid. + maybe_t pgid_{}; - job_group_t(const properties_t &props, cancellation_group_ref_t cg, wcstring command) - : cancel_group(std::move(cg)), props_(props), command_(std::move(command)) { - assert(cancel_group && "Null cancel group"); - } + // The original command which produced this job tree. + const wcstring command_; + + /// Our job ID. -1 if none. + const job_id_t job_id_; }; #endif diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 64a263880..c248caea2 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1338,20 +1338,11 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo const auto &ld = parser->libdata(); - auto job_control_mode = get_job_control_mode(); - // Run all command substitutions in our pgroup. - bool wants_job_control = - !parser->is_command_substitution() && - ((job_control_mode == job_control_t::all) || - ((job_control_mode == job_control_t::interactive) && parser->is_interactive()) || - (ctx.job_group && ctx.job_group->wants_job_control())); - job_t::properties_t props{}; props.initial_background = job_node.bg.has_value(); props.skip_notification = ld.is_subshell || parser->is_block() || ld.is_event || !parser->is_interactive(); props.from_event_handler = ld.is_event; - props.job_control = wants_job_control; props.wants_timing = job_node_wants_timing(job_node); // It's an error to have 'time' in a background job. @@ -1374,23 +1365,12 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo // Clean up the job on failure or cancellation. if (pop_result == end_execution_reason_t::ok) { - // Resolve the job's group and mark if this job is the first to get it. - job->group = job_group_t::resolve_group_for_job(*job, cancel_group, ctx.job_group); + this->setup_group(job.get()); assert(job->group && "Should not have a null group"); - job->mut_flags().is_group_root = (job->group != ctx.job_group); - // Success. Give the job to the parser - it will clean it up. + // Give the job to the parser - it will clean it up. parser->job_add(job); - // Check to see if this contained any external commands. - bool job_contained_external_command = false; - for (const auto &proc : job->processes) { - if (proc->type == process_type_t::external) { - job_contained_external_command = true; - break; - } - } - // Actually execute the job. if (!exec_job(*parser, job, block_io)) { // No process in the job successfully launched. @@ -1404,7 +1384,7 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo // Update universal variables on external commands. // TODO: justify this, why not on every command? - if (job_contained_external_command) { + if (job->has_external_proc()) { parser->vars().universal_barrier(); } } @@ -1545,6 +1525,42 @@ end_execution_reason_t parse_execution_context_t::eval_node(const ast::job_list_ return this->run_job_list(job_list, associated_block); } +void parse_execution_context_t::setup_group(job_t *j) { + // We can use the parent group if it's compatible and we're not backgrounded. + if (ctx.job_group && (ctx.job_group->has_job_id() || !j->wants_job_id()) && + !j->is_initially_background()) { + j->group = ctx.job_group; + return; + } + + if (j->processes.front()->is_internal() || !this->use_job_control()) { + // This job either doesn't have a pgroup (e.g. a simple block), or lives in fish's pgroup. + j->group = job_group_t::create(j->command(), cancel_group, j->wants_job_id()); + } else { + // This is a "real job" that gets its own pgroup. + j->processes.front()->leads_pgrp = true; + bool wants_terminal = !parser->libdata().is_event; + j->group = job_group_t::create_with_job_control(j->command(), cancel_group, wants_terminal); + } + j->group->set_is_foreground(!j->is_initially_background()); + j->mut_flags().is_group_root = true; +} + +bool parse_execution_context_t::use_job_control() const { + if (parser->is_command_substitution()) { + return false; + } + switch (get_job_control_mode()) { + case job_control_t::all: + return true; + case job_control_t::interactive: + return parser->is_interactive(); + case job_control_t::none: + return false; + } + DIE("Unreachable"); +} + int parse_execution_context_t::line_offset_of_node(const ast::job_t *node) { // If we're not executing anything, return -1. if (!node) { diff --git a/src/parse_execution.h b/src/parse_execution.h index 572db7e14..9695af66e 100644 --- a/src/parse_execution.h +++ b/src/parse_execution.h @@ -141,6 +141,12 @@ class parse_execution_context_t : noncopyable_t { end_execution_reason_t populate_job_from_job_node(job_t *j, const ast::job_t &job_node, const block_t *associated_block); + // Assign a job group to the given job. + void setup_group(job_t *j); + + // \return whether we should apply job control to our processes. + bool use_job_control() const; + // Returns the line number of the node. Not const since it touches cached_lineno_offset. int line_offset_of_node(const ast::job_t *node); int line_offset_of_character_at_offset(size_t offset); diff --git a/src/postfork.cpp b/src/postfork.cpp index 980e72889..df465b829 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -283,12 +283,9 @@ posix_spawner_t::posix_spawner_t(const job_t *j, const dup2_list_t &dup2s) { // desired_pgid tracks the pgroup for the process. If it is none, the pgroup is left unchanged. // If it is zero, create a new pgroup from the pid. If it is >0, join that pgroup. maybe_t desired_pgid = none(); - if (auto job_pgid = j->group->get_pgid()) { - desired_pgid = *job_pgid; - } else { - assert(j->group->needs_pgid_assignment() && "We should be expecting a pgid"); - // We are the first external proc in the job group. Set the desired_pgid to 0 to indicate we - // should creating a new process group. + if (auto pgid = j->group->get_pgid()) { + desired_pgid = *pgid; + } else if (j->processes.front()->leads_pgrp) { desired_pgid = 0; } diff --git a/src/proc.cpp b/src/proc.cpp index 96296caf3..cc3e6c67f 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -120,9 +120,6 @@ bool job_t::is_completed() const { } bool job_t::posts_job_exit_events() const { - // If we never got a pgid then we never launched the external process, so don't report it. - if (!this->get_pgid()) return false; - // Only report root job exits. // For example in `ls | begin ; cat ; end` we don't need to report the cat sub-job. if (!flags().is_group_root) return false; @@ -131,14 +128,8 @@ bool job_t::posts_job_exit_events() const { return this->has_external_proc(); } -bool job_t::job_chain_is_fully_constructed() const { return group->is_root_constructed(); } - bool job_t::signal(int signal) { - // Presumably we are distinguishing between the two cases below because we do - // not want to send ourselves the signal in question in case the job shares - // a pgid with the shell. - auto pgid = get_pgid(); - if (pgid.has_value() && *pgid != getpgrp()) { + if (auto pgid = group->get_pgid()) { if (killpg(*pgid, signal) == -1) { char buffer[512]; sprintf(buffer, "killpg(%d, %s)", *pgid, strsignal(signal)); @@ -152,7 +143,6 @@ bool job_t::signal(int signal) { } } } - return true; } @@ -312,12 +302,11 @@ job_t::job_t(const properties_t &props, wcstring command_str) job_t::~job_t() = default; +bool job_t::wants_job_control() const { return group->wants_job_control(); } + void job_t::mark_constructed() { assert(!is_constructed() && "Job was already constructed"); mut_flags().constructed = true; - if (flags().is_group_root) { - group->mark_root_constructed(); - } } bool job_t::has_internal_proc() const { @@ -334,27 +323,20 @@ bool job_t::has_external_proc() const { return false; } -/// A list of pids/pgids that have been disowned. They are kept around until either they exit or +bool job_t::wants_job_id() const { + return processes.size() > 1 || !processes.front()->is_internal() || is_initially_background(); +} + +/// A list of pids that have been disowned. They are kept around until either they exit or /// we exit. Poll these from time-to-time to prevent zombie processes from happening (#5342). static owning_lock> s_disowned_pids; void add_disowned_job(const job_t *j) { assert(j && "Null job"); - - // Never add our own (or an invalid) pgid as it is not unique to only - // one job, and may result in a deadlock if we attempt the wait. - auto pgid = j->get_pgid(); auto disowned_pids = s_disowned_pids.acquire(); - if (pgid && *pgid != getpgrp() && *pgid > 0) { - // waitpid(2) is signalled to wait on a process group rather than a - // process id by using the negative of its value. - disowned_pids->push_back(*pgid * -1); - } else { - // Instead, add the PIDs of any external processes - for (auto &process : j->processes) { - if (process->pid) { - disowned_pids->push_back(process->pid); - } + for (auto &process : j->processes) { + if (process->pid) { + disowned_pids->push_back(process->pid); } } } @@ -630,7 +612,7 @@ static void remove_disowned_jobs(job_list_t &jobs) { auto iter = jobs.begin(); while (iter != jobs.end()) { const auto &j = *iter; - if (j->flags().disown_requested && j->job_chain_is_fully_constructed()) { + if (j->flags().disown_requested && j->is_constructed()) { iter = jobs.erase(iter); } else { ++iter; @@ -810,20 +792,14 @@ void proc_update_jiffies(parser_t &parser) { // restoring a previously-stopped job, in which case we need to restore terminal attributes. int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from_stopped) { enum { notneeded = 0, success = 1, error = -1 }; - - if (!jg->should_claim_terminal()) { - // The job doesn't want the terminal. + if (!jg->wants_terminal() || !jg->get_pgid()) { + // The job doesn't want the terminal, or doesn't have a pgroup yet. return notneeded; } - // Get the pgid; we may not have one. - pid_t 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"); - return notneeded; - } + // Get the pgid. + pid_t pgid = *jg->get_pgid(); + assert(pgid >= 0 && "Invalid pgid"); // If we are continuing, ensure that stdin is marked as blocking first (issue #176). // Also restore tty modes. @@ -933,7 +909,7 @@ int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from return notneeded; } else { FLOGF(warning, _(L"Could not send job %d ('%ls') with pgid %d to foreground"), - jg->get_id(), jg->get_command().c_str(), pgid); + jg->get_job_id(), jg->get_command().c_str(), pgid); wperror(L"tcsetpgrp"); return error; } @@ -956,33 +932,22 @@ int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from /// 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) { +static void terminal_return_from_job_group(job_group_t *jg) { errno = 0; - 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; - } - - FLOG(proc_pgroup, "fish reclaiming terminal after job pgid", *pgid); + FLOG(proc_pgroup, "fish reclaiming terminal"); if (tcsetpgrp(STDIN_FILENO, getpgrp()) == -1) { - if (errno == ENOTTY) redirect_tty_output(); FLOGF(warning, _(L"Could not return shell to foreground")); wperror(L"tcsetpgrp"); - return false; + return; } // Save jobs terminal modes. 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; - FLOGF(warning, _(L"Could not return shell to foreground")); + if (tcgetattr(STDIN_FILENO, &tmodes) == 0) { + jg->tmodes = tmodes; + } else if (errno != ENOTTY) { wperror(L"tcgetattr"); - return false; } - jg->tmodes = tmodes; - return true; } bool job_t::is_foreground() const { return group->is_foreground(); } @@ -997,7 +962,7 @@ maybe_t job_t::get_last_pid() const { return none(); } -job_id_t job_t::job_id() const { return group->get_id(); } +job_id_t job_t::job_id() const { return group->get_job_id(); } void job_t::continue_job(parser_t &parser, bool in_foreground) { // Put job first in the job list. diff --git a/src/proc.h b/src/proc.h index 7a589c50c..01abc89bf 100644 --- a/src/proc.h +++ b/src/proc.h @@ -278,6 +278,10 @@ class process_t : noncopyable_t { /// True if process has stopped. bool stopped{false}; + /// If set, this process is (or will become) the pgroup leader. + /// This is only meaningful for external processes. + bool leads_pgrp{false}; + /// Reported status value. proc_status_t status{}; @@ -318,9 +322,6 @@ class job_t : noncopyable_t { /// Whether this job was created as part of an event handler. bool from_event_handler{}; - - /// Whether the job is under job control, i.e. has its own pgrp. - bool job_control{}; }; private: @@ -376,9 +377,8 @@ class job_t : noncopyable_t { // This is never null and not changed after construction. job_group_ref_t group{}; - /// \return the pgid for the job, based on the job group. - /// This may be none if the job consists of just internal fish functions or builtins. - /// This may also be fish itself. + /// \return our pgid, or none if we don't have one, or are internal to fish + /// This never returns fish's own pgroup. maybe_t get_pgid() const; /// \return the pid of the last external process in the job. @@ -424,7 +424,7 @@ class job_t : noncopyable_t { bool wants_timing() const { return properties.wants_timing; } /// \return if we want job control. - bool wants_job_control() const { return properties.job_control; } + bool wants_job_control() const; /// \return whether this job is initially going to run in the background, because & was /// specified. @@ -439,6 +439,10 @@ class job_t : noncopyable_t { bool has_internal_proc() const; bool has_external_proc() const; + /// \return whether this job, when run, will want a job ID. + /// Jobs that are only a single internal block do not get a job ID. + bool wants_job_id() const; + // Helper functions to check presence of flags on instances of jobs /// The job has been fully constructed, i.e. all its member processes have been launched bool is_constructed() const { return flags().constructed; }