From 33a170d614a6129ca97851773dddbcfecc9cd9b3 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 4 Nov 2024 16:19:22 -0600 Subject: [PATCH] Replace INVALID_PID constant with Option If we end up using this in more places, we can create a `Pid` newtype. Note that while the constant is no longer used in code, its previous value of -2 is still printed by `jobs` when no pgid is associated with a job. I will open a PR to change this to something else, likely either `0` or `-`. --- src/builtins/jobs.rs | 10 +++------- src/exec.rs | 10 ++++++---- src/fork_exec/postfork.rs | 8 ++++++-- src/proc.rs | 8 ++------ 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/builtins/jobs.rs b/src/builtins/jobs.rs index 04835939b..a7a42114e 100644 --- a/src/builtins/jobs.rs +++ b/src/builtins/jobs.rs @@ -8,7 +8,7 @@ use crate::common::{escape_string, timef, EscapeFlags, EscapeStringStyle}; use crate::io::IoStreams; use crate::job_group::{JobId, MaybeJobId}; use crate::parser::Parser; -use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job, INVALID_PID}; +use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job}; use crate::wchar_ext::WExt; use crate::wgetopt::{wopt, ArgType, WGetopter, WOption}; use crate::wutil::wgettext; @@ -50,12 +50,8 @@ fn cpu_use(j: &Job) -> f64 { /// Print information about the specified job. fn builtin_jobs_print(j: &Job, mode: JobsPrintMode, header: bool, streams: &mut IoStreams) { - let mut pgid = INVALID_PID; - { - if let Some(job_pgid) = j.get_pgid() { - pgid = job_pgid; - } - } + // TODO: Breaking change, but don't print meaningless -2 for jobs without a pgid + let pgid = j.get_pgid().unwrap_or(-2); // the old INVALID_PGID value let mut out = WString::new(); match mode { diff --git a/src/exec.rs b/src/exec.rs index c656de1a6..815287a97 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -37,7 +37,7 @@ use crate::parser::{Block, BlockId, BlockType, EvalRes, Parser}; use crate::proc::{ hup_jobs, is_interactive_session, jobs_requiring_warning_on_exit, no_exec, print_exit_warning_for_jobs, InternalProc, Job, JobGroupRef, ProcStatus, Process, ProcessType, - TtyTransfer, INVALID_PID, + TtyTransfer, }; use crate::reader::{reader_run_count, restore_term_mode}; use crate::redirection::{dup2_list_resolve_chain, Dup2List}; @@ -56,6 +56,7 @@ use nix::fcntl::OFlag; use nix::sys::stat; use std::ffi::CStr; use std::io::{Read, Write}; +use std::num::NonZeroU32; use std::os::fd::{AsRawFd, OwnedFd, RawFd}; use std::slice; use std::sync::atomic::Ordering; @@ -491,7 +492,7 @@ fn internal_exec(vars: &EnvStack, j: &Job, block_io: IoChain) { // child_setup_process makes sure signals are properly set up. let redirs = dup2_list_resolve_chain(&all_ios); if child_setup_process( - 0, /* not claim_tty */ + None, /* not claim_tty */ blocked_signals, false, /* not is_forked */ &redirs, @@ -677,9 +678,10 @@ fn fork_child_for_process( ) -> LaunchResult { // Claim the tty from fish, if the job wants it and we are the pgroup leader. let claim_tty_from = if p.leads_pgrp && job.group().wants_terminal() { - unsafe { libc::getpgrp() } + // getpgrp(2) cannot fail and always returns the (positive) caller's pgid + Some(NonZeroU32::new(unsafe { libc::getpgrp() } as u32).unwrap()) } else { - INVALID_PID + None }; // Decide if the job wants to set a custom sigmask. diff --git a/src/fork_exec/postfork.rs b/src/fork_exec/postfork.rs index 14bcb4c7a..9222d4dca 100644 --- a/src/fork_exec/postfork.rs +++ b/src/fork_exec/postfork.rs @@ -8,6 +8,7 @@ use crate::signal::signal_reset_handlers; use crate::{common::exit_without_destructors, wutil::fstat}; use libc::{c_char, pid_t, O_RDONLY}; use std::ffi::CStr; +use std::num::NonZeroU32; use std::os::unix::fs::MetadataExt; use std::time::Duration; @@ -143,7 +144,7 @@ pub fn execute_setpgid(pid: pid_t, pgroup: pid_t, is_parent: bool) -> i32 { /// Set up redirections and signal handling in the child process. pub fn child_setup_process( - claim_tty_from: pid_t, + claim_tty_from: Option, sigmask: Option<&libc::sigset_t>, is_forked: bool, dup2s: &Dup2List, @@ -173,7 +174,10 @@ pub fn child_setup_process( return err; } } - if claim_tty_from >= 0 && unsafe { libc::tcgetpgrp(libc::STDIN_FILENO) } == claim_tty_from { + if claim_tty_from + // tcgetpgrp() can return -1 but pid.get() cannot, so cast the latter to the former + .is_some_and(|pid| unsafe { libc::tcgetpgrp(libc::STDIN_FILENO) } == pid.get() as i32) + { // 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 diff --git a/src/proc.rs b/src/proc.rs index e154beec3..31e7a62b7 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -308,12 +308,6 @@ impl InternalProc { } } -/// 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 -pub const INVALID_PID: i32 = -2; - // Allows transferring the tty to a job group, while it runs. #[derive(Default)] pub struct TtyTransfer { @@ -855,6 +849,7 @@ impl Job { /// Return our pgid, or none if we don't have one, or are internal to fish /// This never returns fish's own pgroup. + // TODO: Return a type-safe result. pub fn get_pgid(&self) -> Option { self.group().get_pgid() } @@ -862,6 +857,7 @@ impl Job { /// Return the pid of the last external process in the job. /// This may be none if the job consists of just internal fish functions or builtins. /// This will never be fish's own pid. + // TODO: Return a type-safe result. pub fn get_last_pid(&self) -> Option { self.processes() .iter()