From 89fb408eb6caf7ae5ba1d6111245065d15b45aed Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 23 Jun 2019 12:39:29 -0700 Subject: [PATCH] Migrate some job flags into const properties struct This helps clarify which parts of a job are mutable, and which are constant. --- src/builtin_bg.cpp | 4 +-- src/builtin_fg.cpp | 5 ++-- src/exec.cpp | 8 +++--- src/parse_execution.cpp | 23 ++++++++------- src/postfork.cpp | 10 +++---- src/proc.cpp | 18 +++++------- src/proc.h | 62 ++++++++++++++++++++++++++++------------- 7 files changed, 76 insertions(+), 54 deletions(-) diff --git a/src/builtin_bg.cpp b/src/builtin_bg.cpp index de5dce4b6..b2d8f3aae 100644 --- a/src/builtin_bg.cpp +++ b/src/builtin_bg.cpp @@ -19,7 +19,7 @@ /// Helper function for builtin_bg(). static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j) { assert(j != NULL); - if (!j->get_flag(job_flag_t::JOB_CONTROL)) { + if (!j->wants_job_control()) { streams.err.append_format( _(L"%ls: Can't put job %d, '%ls' to background because it is not under job control\n"), L"bg", j->job_id, j->command_wcstr()); @@ -54,7 +54,7 @@ int builtin_bg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { // No jobs were specified so use the most recent (i.e., last) job. job_t *job = nullptr; for (const auto &j : parser.jobs()) { - if (j->is_stopped() && j->get_flag(job_flag_t::JOB_CONTROL) && (!j->is_completed())) { + if (j->is_stopped() && j->wants_job_control() && (!j->is_completed())) { job = j.get(); break; } diff --git a/src/builtin_fg.cpp b/src/builtin_fg.cpp index 42c1bc4f1..7e24477a0 100644 --- a/src/builtin_fg.cpp +++ b/src/builtin_fg.cpp @@ -40,8 +40,7 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { for (const auto &j : parser.jobs()) { if (j->is_constructed() && (!j->is_completed()) && - ((j->is_stopped() || (!j->is_foreground())) && - j->get_flag(job_flag_t::JOB_CONTROL))) { + ((j->is_stopped() || (!j->is_foreground())) && j->wants_job_control())) { job = j.get(); break; } @@ -77,7 +76,7 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) { if (!job || !job->is_constructed() || job->is_completed()) { streams.err.append_format(_(L"%ls: No suitable job: %d\n"), cmd, pid); job = nullptr; - } else if (!job->get_flag(job_flag_t::JOB_CONTROL)) { + } else if (!job->wants_job_control()) { streams.err.append_format(_(L"%ls: Can't put job %d, '%ls' to foreground because " L"it is not under job control\n"), cmd, pid, job->command_wcstr()); diff --git a/src/exec.cpp b/src/exec.cpp index 234f656c2..04a2cd2ee 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -278,10 +278,10 @@ void internal_exec_helper(parser_t &parser, parsed_source_ref_t parsed_source, t // foreground process group, we don't use posix_spawn if we're going to foreground the process. (If // we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race). static bool can_use_posix_spawn_for_job(const std::shared_ptr &job) { - if (job->get_flag(job_flag_t::JOB_CONTROL)) { //!OCLINT(collapsible if statements) + 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->get_flag(job_flag_t::TERMINAL) && job->is_foreground()) { + if (job->wants_terminal() && job->is_foreground()) { // It will be foregrounded, so we will call tcsetpgrp(), therefore do not use // posix_spawn. return false; @@ -327,7 +327,7 @@ static void on_process_created(const std::shared_ptr &j, pid_t child_pid) return; } - if (j->get_flag(job_flag_t::JOB_CONTROL)) { + if (j->wants_job_control()) { j->pgid = child_pid; } else { j->pgid = getpgrp(); @@ -744,7 +744,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr // child group has been set. See discussion here: // https://github.com/Microsoft/WSL/issues/2997 And confirmation that this persists // past glibc 2.24+ here: https://github.com/fish-shell/fish-shell/issues/4715 - if (j->get_flag(job_flag_t::JOB_CONTROL) && getpgid(p->pid) != j->pgid) { + if (j->wants_job_control() && getpgid(p->pid) != j->pgid) { set_child_group(j.get(), p->pid); } #else diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 8a34895ef..94d3becc4 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1231,20 +1231,23 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo return result; } - shared_ptr job = std::make_shared(acquire_job_id(), block_io, parent_job); - auto &ld = parser->libdata(); - job->tmodes = tmodes; + const auto &ld = parser->libdata(); + auto job_control_mode = get_job_control_mode(); - job->set_flag(job_flag_t::JOB_CONTROL, - (job_control_mode == job_control_t::all) || - ((job_control_mode == job_control_t::interactive) && shell_is_interactive())); + bool wants_job_control = + (job_control_mode == job_control_t::all) || + ((job_control_mode == job_control_t::interactive) && shell_is_interactive()); - job->set_flag(job_flag_t::FOREGROUND, !job_node_is_background(job_node)); + job_t::properties_t props{}; + props.foreground = !job_node_is_background(job_node); + props.wants_terminal = wants_job_control && !ld.is_event; + props.skip_notification = + ld.is_subshell || ld.is_block || ld.is_event || !shell_is_interactive(); - job->set_flag(job_flag_t::TERMINAL, job->get_flag(job_flag_t::JOB_CONTROL) && !ld.is_event); + shared_ptr job = std::make_shared(acquire_job_id(), props, block_io, parent_job); + job->tmodes = tmodes; - job->set_flag(job_flag_t::SKIP_NOTIFICATION, - ld.is_subshell || ld.is_block || ld.is_event || !shell_is_interactive()); + job->set_flag(job_flag_t::JOB_CONTROL, wants_job_control); // 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/postfork.cpp b/src/postfork.cpp index 4643fad3e..e0e18acd0 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -41,7 +41,7 @@ /// process if it is the first in a JOB_CONTROL job. /// Returns true on sucess, false on failiure. bool child_set_group(job_t *j, process_t *p) { - if (j->get_flag(job_flag_t::JOB_CONTROL)) { + if (j->wants_job_control()) { if (j->pgid == INVALID_PID) { j->pgid = p->pid; } @@ -107,7 +107,7 @@ bool child_set_group(job_t *j, process_t *p) { /// group in the case of JOB_CONTROL, and we can give the new process group control of the terminal /// if it's to run in the foreground. bool set_child_group(job_t *j, pid_t child_pid) { - if (j->get_flag(job_flag_t::JOB_CONTROL)) { + if (j->wants_job_control()) { assert(j->pgid != INVALID_PID && "set_child_group called with JOB_CONTROL before job pgid determined!"); @@ -135,7 +135,7 @@ bool set_child_group(job_t *j, pid_t child_pid) { } bool maybe_assign_terminal(const job_t *j) { - if (j->get_flag(job_flag_t::TERMINAL) && j->is_foreground()) { //!OCLINT(early exit) + if (j->wants_terminal() && j->is_foreground()) { //!OCLINT(early exit) if (tcgetpgrp(STDIN_FILENO) == j->pgid) { // We've already assigned the process group control of the terminal when the first // process in the job was started. There's no need to do so again, and on some platforms @@ -168,7 +168,7 @@ int child_setup_process(const job_t *job, process_t *p, const dup2_list_t &dup2s // Set the handling for job control signals back to the default. signal_reset_handlers(); - if (job != nullptr && job->get_flag(job_flag_t::TERMINAL) && job->is_foreground()) { + if (job != nullptr && job->wants_terminal() && job->is_foreground()) { // Assign the terminal within the child to avoid the well-known race between tcsetgrp() in // the parent and the child executing. We are not interested in error handling here, except // we try to avoid this for non-terminals; in particular pipelines often make non-terminal @@ -241,7 +241,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, bool should_set_process_group_id = false; int desired_process_group_id = 0; - if (j->get_flag(job_flag_t::JOB_CONTROL)) { + if (j->wants_job_control()) { should_set_process_group_id = true; // set_child_group puts each job into its own process group diff --git a/src/proc.cpp b/src/proc.cpp index 682ddfcf2..50bfd3fd5 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -279,13 +279,9 @@ void process_t::check_generations_before_launch() { gens_ = topic_monitor_t::principal().current_generations(); } -job_t::job_t(job_id_t jobid, io_chain_t bio, std::shared_ptr parent) - : block_io(std::move(bio)), - parent_job(std::move(parent)), - pgid(INVALID_PID), - tmodes(), - job_id(jobid), - flags{} {} +job_t::job_t(job_id_t job_id, const properties_t &props, io_chain_t bio, + std::shared_ptr parent) + : properties(props), block_io(std::move(bio)), parent_job(std::move(parent)), job_id(job_id) {} job_t::~job_t() { release_job_id(job_id); } @@ -490,7 +486,7 @@ static bool try_clean_process_in_job(process_t *p, job_t *j, std::vectorget_flag(job_flag_t::SKIP_NOTIFICATION) && !contains(crashsignals, s.signal_code())) { + if (j->skip_notification() && !contains(crashsignals, s.signal_code())) { return false; } @@ -540,7 +536,7 @@ static bool job_wants_message(const shared_ptr &j) { if (j->get_flag(job_flag_t::NOTIFIED)) return false; // Do we just skip notifications? - if (j->get_flag(job_flag_t::SKIP_NOTIFICATION)) return false; + if (j->skip_notification()) return false; // Are we foreground? // The idea here is to not print status messages for jobs that execute in the foreground (i.e. @@ -872,7 +868,7 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se }); if (!is_completed()) { - if (get_flag(job_flag_t::TERMINAL) && is_foreground()) { + 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); @@ -980,7 +976,7 @@ void hup_background_jobs(const parser_t &parser) { // TODO: we should probably hup all jobs across all parsers here. for (const auto &j : parser.jobs()) { // Make sure we don't try to SIGHUP the calling builtin - if (j->pgid == INVALID_PID || !j->get_flag(job_flag_t::JOB_CONTROL)) { + if (j->pgid == INVALID_PID || !j->wants_job_control()) { continue; } diff --git a/src/proc.h b/src/proc.h index cc52fc845..ad1290491 100644 --- a/src/proc.h +++ b/src/proc.h @@ -140,6 +140,12 @@ class internal_proc_t { internal_proc_t(); }; +/// 0 should not be used; although it is not a valid PGID in userspace, +/// the Linux kernel will use it for kernel processes. +/// -1 should not be used; it is a possible return value of the getpgid() +/// function +enum { INVALID_PID = -2 }; + /// A structure representing a single fish process. Contains variables for tracking process state /// and the process argument list. Actually, a fish process can be either a regular external /// process, an internal builtin which may or may not spawn a fake IO process during execution, a @@ -255,15 +261,10 @@ enum class job_flag_t { /// Whether the specified job is completely constructed, i.e. completely parsed, and every /// process in the job has been forked, etc. CONSTRUCTED, - /// Whether the specified job is a part of a subshell, event handler or some other form of - /// special job that should not be reported. - SKIP_NOTIFICATION, /// Whether the exit status should be negated. This flag can only be set by the not builtin. NEGATE, /// Whether the job is under job control, i.e. has its own pgrp. JOB_CONTROL, - /// Whether the job wants to own the terminal when in the foreground. - TERMINAL, /// This job is disowned, and should be removed from the active jobs list. DISOWN_REQUESTED, @@ -282,6 +283,25 @@ void release_job_id(job_id_t jobid); /// A struct represeting a job. A job is basically a pipeline of one or more processes and a couple /// of flags. class job_t { + public: + /// A set of jobs properties. These are immutable: they do not change for the lifetime of the + /// job. + struct properties_t { + /// Whether this job is in the foreground, i.e. whether it did NOT have a & at the end. + bool foreground{}; + + /// Whether the specified job is a part of a subshell, event handler or some other form of + /// special job that should not be reported. + bool skip_notification{}; + + /// Whether the job wants to own the terminal when in the foreground. + bool wants_terminal{}; + }; + + private: + /// Set of immutable job properties. + const properties_t properties; + /// The original command which led to the creation of this job. It is used for displaying /// messages about job status on the terminal. wcstring command_str; @@ -298,7 +318,8 @@ class job_t { void operator=(const job_t &) = delete; public: - job_t(job_id_t jobid, io_chain_t bio, std::shared_ptr parent); + job_t(job_id_t job_id, const properties_t &props, io_chain_t bio, + std::shared_ptr parent); ~job_t(); /// Returns whether the command is empty. @@ -356,21 +377,29 @@ class job_t { /// Process group ID for the process group that this job is running in. /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor - pid_t pgid; + pid_t pgid{INVALID_PID}; + + /// The id of this job. + const job_id_t 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; - /// The job id of the job. This is a small integer that is a unique identifier of the job within - /// this shell, and is used e.g. in process expansion. - const job_id_t job_id; + struct termios tmodes {}; + /// Bitset containing information about the job. A combination of the JOB_* constants. - enum_set_t flags; + enum_set_t flags{}; // Get and set flags bool get_flag(job_flag_t flag) const; void set_flag(job_flag_t flag, bool set); + /// \return if we want job control. + bool wants_job_control() const { return get_flag(job_flag_t::JOB_CONTROL); } + + /// \return if this job wants to own the terminal in the foreground. + bool wants_terminal() const { return properties.wants_terminal; } + /// Returns the block IO redirections associated with the job. These are things like the IO /// redirections associated with the begin...end statement. const io_chain_t &block_io_chain() const { return this->block_io; } @@ -382,7 +411,7 @@ class job_t { /// The job has been fully constructed, i.e. all its member processes have been launched bool is_constructed() const { return get_flag(job_flag_t::CONSTRUCTED); } /// The job was launched in the foreground and has control of the terminal - bool is_foreground() const { return get_flag(job_flag_t::FOREGROUND); } + bool is_foreground() const { return properties.foreground; } /// The job is complete, i.e. all its member processes have been reaped bool is_completed() const; /// The job is in a stopped state @@ -391,6 +420,7 @@ class job_t { bool is_visible() const { return !is_completed() && is_constructed() && !get_flag(job_flag_t::DISOWN_REQUESTED); }; + bool skip_notification() const { return properties.skip_notification; } /// \return the parent job, or nullptr. const std::shared_ptr get_parent() const { return parent_job; } @@ -509,12 +539,6 @@ pid_t terminal_acquire_before_builtin(int job_pgid); /// Used to avoid zombie processes after disown. void add_disowned_pgid(pid_t pgid); -/// 0 should not be used; although it is not a valid PGID in userspace, -/// the Linux kernel will use it for kernel processes. -/// -1 should not be used; it is a possible return value of the getpgid() -/// function -enum { INVALID_PID = -2 }; - bool have_proc_stat(); #endif