From d467bb58d996911211f9c1095521c7447b521300 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 Oct 2018 22:55:18 -0500 Subject: [PATCH] Replace pid/pgid -2 with INVALID_PID --- src/exec.cpp | 8 ++++---- src/postfork.cpp | 7 +++---- src/proc.cpp | 15 +++++---------- src/proc.h | 11 +++++++++-- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index eba71b33d..e4d63b073 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -388,7 +388,7 @@ void internal_exec(job_t *j, const io_chain_t &&all_ios) { static void on_process_created(job_t *j, pid_t child_pid) { // We only need to do this the first time a child is forked/spawned - if (j->pgid != -2) { + if (j->pgid != INVALID_PID) { return; } @@ -724,9 +724,9 @@ static bool exec_external_command(job_t *j, process_t *p, const io_chain_t &proc } #else // In do_fork, the pid of the child process is used as the group leader if j->pgid - // == 2 above, posix_spawn assigned the new group a pgid equal to its own id if - // j->pgid == 2 so this is what we do instead of calling set_child_group: - if (j->pgid == -2) { + // invalid, posix_spawn assigned the new group a pgid equal to its own id if + // j->pgid was invalid, so this is what we do instead of calling set_child_group + if (j->pgid == INVALID_PID) { j->pgid = pid; } #endif diff --git a/src/postfork.cpp b/src/postfork.cpp index dd3b1924e..b9787b8a4 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -65,8 +65,7 @@ static void debug_safe_int(int level, const char *format, int val) { bool child_set_group(job_t *j, process_t *p) { bool retval = true; if (j->get_flag(JOB_CONTROL)) { - // New jobs have the pgid set to -2 - if (j->pgid == -2) { + if (j->pgid == INVALID_PID) { j->pgid = p->pid; } if (setpgid(p->pid, j->pgid) < 0) { @@ -106,7 +105,7 @@ bool child_set_group(job_t *j, process_t *p) { /// if it's to run in the foreground. bool set_child_group(job_t *j, pid_t child_pid) { if (j->get_flag(JOB_CONTROL)) { - assert (j->pgid != -2 + assert (j->pgid != INVALID_PID && "set_child_group called with JOB_CONTROL before job pgid determined!"); // The parent sets the child's group. This incurs the well-known unavoidable race with the @@ -345,7 +344,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, // set_child_group puts each job into its own process group // do the same here if there is no PGID yet (i.e. PGID == -2) desired_process_group_id = j->pgid; - if (desired_process_group_id == -2) { + if (desired_process_group_id == INVALID_PID) { desired_process_group_id = 0; } } diff --git a/src/proc.cpp b/src/proc.cpp index 9ad7956ed..3a472fa15 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -343,13 +343,8 @@ static void handle_child_status(pid_t pid, int status) { process_t::process_t() {} -/// The constructor sets the pgid to -2 as a sentinel value -/// 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 job_t::job_t(job_id_t jobid, io_chain_t bio) - : block_io(std::move(bio)), pgid(-2), tmodes(), job_id(jobid), flags(0) {} + : block_io(std::move(bio)), pgid(INVALID_PID), tmodes(), job_id(jobid), flags(0) {} job_t::~job_t() { release_job_id(job_id); } @@ -416,7 +411,7 @@ static int process_mark_finished_children(bool wants_await) { job_t *j; job_iterator_t jobs; while ((j = jobs.next())) { any_jobs = true; - if (j->pgid == -2 || !j->get_flag(JOB_CONSTRUCTED)) { + if (j->pgid == INVALID_PID || !j->get_flag(JOB_CONSTRUCTED)) { // Job has not been fully constructed yet debug(4, "Skipping iteration of not fully constructed job %d", j->pgid); continue; @@ -627,11 +622,11 @@ static int process_clean_after_marking(bool allow_interactive) { } // TODO: The generic process-exit event is useless and unused. // Remove this in future. - // Don't fire the exit-event for jobs with pgid -2. + // Don't fire the exit-event for jobs with pgid INVALID_PID. // That's our "sentinel" pgid, for jobs that don't (yet) have a pgid, // or jobs that consist entirely of builtins (and hence don't have a process). // This causes issues if fish is PID 2, which is quite common on WSL. See #4582. - if (j->pgid != -2) { + if (j->pgid != INVALID_PID) { proc_fire_event(L"JOB_EXIT", EVENT_EXIT, -j->pgid, 0); } proc_fire_event(L"JOB_EXIT", EVENT_JOB_ID, j->job_id, 0); @@ -825,7 +820,7 @@ bool terminal_give_to_job(const job_t *j, bool cont) { // http://curiousthing.org/sigttin-sigttou-deep-dive-linux In all cases, our goal here was just // to hand over control of the terminal to this process group, which is a no-op if it's already // been done. - if (j->pgid == -2 || tcgetpgrp(STDIN_FILENO) == j->pgid) { + if (j->pgid == INVALID_PID || tcgetpgrp(STDIN_FILENO) == j->pgid) { debug(4, L"Process group %d already has control of terminal\n", j->pgid); } else { debug(4, diff --git a/src/proc.h b/src/proc.h index c85bc3712..e4372d284 100644 --- a/src/proc.h +++ b/src/proc.h @@ -370,10 +370,17 @@ int proc_format_status(int status); /// Wait for any process finishing. pid_t proc_wait_any(); -#endif - bool terminal_give_to_job(const job_t *j, bool cont); /// 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. pid_t terminal_acquire_before_builtin(int job_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 }; + +#endif