From b119c4b3bbad8f4b0987e3a0656afc79f439834d Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 29 May 2020 15:28:53 -0700 Subject: [PATCH] Eliminate pgroup_provenance_t Now that job trees are a single source of truth for a job's pgid, we no longer need fancy logic around how the pgroup is assigned. --- src/exec.cpp | 57 +++-------------------------------------- src/exec.h | 4 --- src/parse_execution.cpp | 1 - src/postfork.cpp | 3 +-- src/proc.cpp | 25 +++++++++++++----- src/proc.h | 25 +++--------------- 6 files changed, 27 insertions(+), 88 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 2e51d0297..0faa5e1e5 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -52,35 +52,6 @@ /// Number of calls to fork() or posix_spawn(). static relaxed_atomic_t s_fork_count{0}; -pgroup_provenance_t get_pgroup_provenance(const shared_ptr &j) { - bool first_proc_is_internal = j->processes.front()->is_internal(); - bool has_internal = j->has_internal_proc(); - bool has_external = j->has_external_proc(); - assert(first_proc_is_internal ? has_internal : has_external); - - if (j->job_tree->get_pgid().has_value()) { - // Our job tree already has a pgid. - return pgroup_provenance_t::lineage; - } else if (!j->wants_job_control()) { - // This job doesn't need job control, it can just live in the fish pgroup. - return pgroup_provenance_t::fish_itself; - } else if (has_external && !first_proc_is_internal) { - // The first process is external, it will own the pgroup. - return pgroup_provenance_t::first_external_proc; - } else if (has_external && first_proc_is_internal) { - // The terminal owner has to be the process which is permitted to read from stdin. - // This is the first process in the pipeline. When executing, a process in the job will - // claim the pgrp if it's not set; therefore set it according to the first process. - // Only do this if there's an external process - see #6011. - return pgroup_provenance_t::fish_itself; - } else { - assert(has_internal && !has_external && "Should be internal only"); - // This job consists of only internal functions or builtins; we do not need to assign a - // pgroup (yet). - return pgroup_provenance_t::unassigned; - } -} - /// 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. @@ -204,11 +175,11 @@ 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) { - if (j->pgroup_provenance == pgroup_provenance_t::first_external_proc && - !j->job_tree->get_pgid()) { - j->job_tree->set_pgid(child_pid); + auto &jt = j->job_tree; + if (jt->needs_pgid_assignment()) { + jt->set_pgid(child_pid); } - return *j->job_tree->get_pgid(); + return *jt->get_pgid(); } /// Construct an internal process for the process p. In the background, write the data \p outdata to @@ -951,26 +922,6 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const io_chain_t &bl pid_t pgrp = getpgrp(); // Check to see if we should reclaim the foreground pgrp after the job finishes or stops. const bool reclaim_foreground_pgrp = (tcgetpgrp(STDIN_FILENO) == pgrp); - - // Perhaps we know our pgroup already. - switch (j->pgroup_provenance) { - case pgroup_provenance_t::lineage: { - break; - } - - case pgroup_provenance_t::fish_itself: - // TODO: should not need this 'if' here. Rationalize this. - if (! j->job_tree->is_placeholder()) { - j->job_tree->set_pgid(pgrp); - } - break; - - // The remaining cases are all deferred until later. - case pgroup_provenance_t::unassigned: - case pgroup_provenance_t::first_external_proc: - break; - } - const size_t stdout_read_limit = parser.libdata().read_limit; // Get the list of all FDs so we can ensure our pipes do not conflict. diff --git a/src/exec.h b/src/exec.h index 02aa63488..5098e6580 100644 --- a/src/exec.h +++ b/src/exec.h @@ -37,10 +37,6 @@ int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tr /// Loops over close until the syscall was run without being interrupted. void exec_close(int fd); -/// Compute the "pgroup provenance" for a job. This is a description of how the pgroup is -/// assigned. It's factored out because the logic has subtleties, and this centralizes it. -pgroup_provenance_t get_pgroup_provenance(const std::shared_ptr &j); - /// Add signals that should be masked for external processes in this job. bool blocked_signals_for_job(const job_t &job, sigset_t *sigmask); diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 52ec7b07b..512aa2196 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1289,7 +1289,6 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t job_ if (pop_result == end_execution_reason_t::ok) { // Set the pgroup assignment mode and job tree, now that the job is populated. job_tree_t::populate_tree_for_job(job.get(), ctx.job_tree); - job->pgroup_provenance = get_pgroup_provenance(job); assert(job->job_tree && "Should have a job tree"); // Success. Give the job to the parser - it will clean it up. diff --git a/src/postfork.cpp b/src/postfork.cpp index 1a9b29466..caec20aea 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -208,8 +208,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, if (auto job_pgid = j->job_tree->get_pgid()) { desired_pgid = *job_pgid; } else { - assert(j->pgroup_provenance == pgroup_provenance_t::first_external_proc && - "We should have already known our pgroup"); + assert(j->job_tree->needs_pgid_assignment() && "We should be expecting a pgid"); // We are the first external proc in the job tree. Set the desired_pgid to 0 to indicate we // should creating a new process group. desired_pgid = 0; diff --git a/src/proc.cpp b/src/proc.cpp index 9e9fc80e2..ee1528c96 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -253,9 +253,9 @@ job_tree_t::~job_tree_t() { } void job_tree_t::set_pgid(pid_t pgid) { - // TODO: thread safety? - assert(!pgid_.has_value() && "Already has a pgid"); - assert(!is_placeholder() && "Cannot set a pgid on the placeholder"); + // Note we need not be concerned about thread safety. job_trees 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"); pgid_ = pgid; } @@ -265,11 +265,12 @@ maybe_t job_tree_t::get_pgid() const { return pgid_; } void job_tree_t::populate_tree_for_job(job_t *job, const job_tree_ref_t &proposed) { // Note there's three cases to consider: // nullptr -> this is a root job, there is no inherited job tree - // placeholder -> we are running as part of a simple function execution, create a new job - // tree for any spawned jobs + // placeholder -> the parent is running as part of a simple function execution + // We may need to create a new job tree if we are going to fork. // non-placeholder -> we are running as part of a real pipeline // Decide if this job can use the placeholder tree. // This is true if it's a simple foreground execution of an internal proc. + bool first_proc_internal = job->processes.front()->is_internal(); bool can_use_placeholder = !job->is_initially_background() && job->processes.size() == 1 && job->processes.front()->is_internal(); @@ -286,12 +287,22 @@ void job_tree_t::populate_tree_for_job(job_t *job, const job_tree_ref_t &propose } job->mut_flags().is_tree_root = needs_new_tree; + bool job_control = job->wants_job_control(); if (!needs_new_tree) { job->job_tree = proposed; + } else if (can_use_placeholder) { + job->job_tree.reset(new job_tree_t(job_control, true)); } else { - job->job_tree = - job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder)); + job->job_tree.reset(new job_tree_t(job_control, false)); + + // 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 (!job_control || first_proc_internal) { + job->job_tree->set_pgid(getpgrp()); + } } } diff --git a/src/proc.h b/src/proc.h index c8808eb4c..8f8c3b0ae 100644 --- a/src/proc.h +++ b/src/proc.h @@ -191,6 +191,10 @@ class job_tree_t { /// \return whether this is a placeholder. bool is_placeholder() const { return is_placeholder_; } + /// \return whether this job tree is awaiting a pgid. + /// This is true for non-placeholder trees that don't already have a pgid. + bool needs_pgid_assignment() const { return !is_placeholder_ && !pgid_.has_value(); } + /// \return the job ID, or -1 if none. job_id_t get_id() const { return job_id_; } @@ -344,24 +348,6 @@ job_id_t acquire_job_id(void); void release_job_id(job_id_t jid); -/// A job has a mode which describes how its pgroup is assigned (before the value is known). -/// This is a constant property of the job. -enum class pgroup_provenance_t { - /// The job has no pgroup assignment. This is used for e.g. a simple function invocation with no - /// pipeline. - unassigned, - - /// The job's pgroup is fish's pgroup. This is used when fish needs to read from the terminal, - /// or if job control is disabled. - fish_itself, - - /// The job's pgroup will come from its first external process. - first_external_proc, - - /// The job's pgroup will come from its lineage. This is used for jobs that are run nested. - lineage, -}; - /// A struct representing a job. A job is a pipeline of one or more processes. class job_t { public: @@ -466,9 +452,6 @@ class job_t { /// This may also be fish itself. maybe_t get_pgid() const { return job_tree->get_pgid(); } - /// How the above pgroup is assigned. This should be set at construction and not modified after. - pgroup_provenance_t pgroup_provenance{}; - /// The id of this job. /// This is user-visible, is recycled, and may be -1. job_id_t job_id() const { return job_tree->get_id(); }