From 2cf4b12d4131b6c6ef11102e0fd14c7bd89f86ff Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 11 Nov 2024 14:27:37 -0600 Subject: [PATCH] Use strongly typed `Option` for event handler This caught an incorrect description for process/job exit handlers for ANY_PID (now removed) which has been replaced with a message stating the handler is for any process exit event. --- src/bin/fish.rs | 7 +++++-- src/builtins/function.rs | 15 +++++++------- src/builtins/jobs.rs | 2 +- src/event.rs | 43 ++++++++++++++++++++++------------------ src/function.rs | 4 +++- src/proc.rs | 19 +++++++++--------- 6 files changed, 50 insertions(+), 40 deletions(-) diff --git a/src/bin/fish.rs b/src/bin/fish.rs index ca80b5948..aee360043 100644 --- a/src/bin/fish.rs +++ b/src/bin/fish.rs @@ -50,7 +50,7 @@ use fish::{ printf, proc::{ get_login, is_interactive_session, mark_login, mark_no_exec, proc_init, - set_interactive_session, + set_interactive_session, Pid, }, reader::{reader_init, reader_read, term_copy_modes}, signal::{signal_clear_cancel, signal_unblock_all}, @@ -695,7 +695,10 @@ fn throwing_main() -> i32 { parser.get_last_status() }; - event::fire(parser, Event::process_exit(getpid(), exit_status)); + event::fire( + parser, + Event::process_exit(Pid::new(getpid()).unwrap(), exit_status), + ); // Trigger any exit handlers. event::fire_generic( diff --git a/src/builtins/function.rs b/src/builtins/function.rs index 1b198c21f..fa926e750 100644 --- a/src/builtins/function.rs +++ b/src/builtins/function.rs @@ -10,6 +10,7 @@ use crate::global_safety::RelaxedAtomicBool; use crate::nix::getpid; use crate::parse_tree::NodeRef; use crate::parser_keywords::parser_keywords_is_reserved; +use crate::proc::Pid; use crate::signal::Signal; use std::sync::Arc; @@ -148,7 +149,7 @@ fn parse_cmd_opts( } e = EventDescription::CallerExit { caller_id }; } else if opt == 'p' && woptarg == "%self" { - let pid: i32 = getpid(); + let pid = Pid::new(getpid()); e = EventDescription::ProcessExit { pid }; } else { let Ok(pid @ 0..) = fish_wcstoi(woptarg) else { @@ -160,12 +161,12 @@ fn parse_cmd_opts( return STATUS_INVALID_ARGS; }; if opt == 'p' { - e = EventDescription::ProcessExit { pid }; + e = EventDescription::ProcessExit { pid: Pid::new(pid) }; } else { // TODO: rationalize why a default of 0 is sensible. let internal_job_id = job_id_for_pid(pid, parser).unwrap_or(0); e = EventDescription::JobExit { - pid, + pid: Pid::new(pid), internal_job_id, }; } @@ -370,14 +371,14 @@ pub fn function( // process has already exited, run it immediately (#7210). for ed in &opts.events { match *ed { - EventDescription::ProcessExit { pid } if pid != event::ANY_PID => { - let wh = parser.get_wait_handles().get_by_pid(pid); + EventDescription::ProcessExit { pid: Some(pid) } => { + let wh = parser.get_wait_handles().get_by_pid(pid.as_pid_t()); if let Some(status) = wh.and_then(|wh| wh.status()) { event::fire(parser, event::Event::process_exit(pid, status)); } } - EventDescription::JobExit { pid, .. } if pid != event::ANY_PID => { - let wh = parser.get_wait_handles().get_by_pid(pid); + EventDescription::JobExit { pid: Some(pid), .. } => { + let wh = parser.get_wait_handles().get_by_pid(pid.as_pid_t()); if let Some(wh) = wh { if wh.is_completed() { event::fire(parser, event::Event::job_exit(pid, wh.internal_job_id)); diff --git a/src/builtins/jobs.rs b/src/builtins/jobs.rs index 98d7fd1cc..89b7ef9c0 100644 --- a/src/builtins/jobs.rs +++ b/src/builtins/jobs.rs @@ -37,7 +37,7 @@ fn cpu_use(j: &Job) -> f64 { let mut u = 0.0; for p in j.external_procs() { let now = timef(); - let jiffies = proc_get_jiffies(p.pid.load().unwrap().as_pid_t()); + let jiffies = proc_get_jiffies(p.pid.load().unwrap()); let last_jiffies = p.last_times.get().jiffies; let since = now - last_jiffies as f64; if since > 0.0 && jiffies > last_jiffies { diff --git a/src/event.rs b/src/event.rs index 4320b2807..330836524 100644 --- a/src/event.rs +++ b/src/event.rs @@ -4,7 +4,6 @@ //! defined when these functions produce output or perform memory allocations, since such functions //! may not be safely called by signal handlers. -use libc::pid_t; use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; @@ -13,6 +12,7 @@ use crate::flog::FLOG; use crate::io::{IoChain, IoStreams}; use crate::job_group::MaybeJobId; use crate::parser::{Block, Parser}; +use crate::proc::Pid; use crate::signal::{signal_check_cancel, signal_handle, Signal}; use crate::termsize; use crate::wchar::prelude::*; @@ -27,8 +27,6 @@ pub enum event_type_t { generic, } -pub const ANY_PID: pid_t = 0; - #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub enum EventDescription { /// Matches any event type (not always any event, as the function name may limit the choice as @@ -40,13 +38,13 @@ pub enum EventDescription { Variable { name: WString }, /// An event triggered by a process exit. ProcessExit { - /// Process ID. Use [`ANY_PID`] to match any pid. - pid: pid_t, + /// Process ID. Use [`None`] to match any pid. + pid: Option, }, /// An event triggered by a job exit. JobExit { - /// pid requested by the event, or [`ANY_PID`] for all. - pid: pid_t, + /// pid requested by the event, or [`None`] for all. + pid: Option, /// `internal_job_id` of the job to match. /// If this is 0, we match either all jobs (`pid == ANY_PID`) or no jobs (otherwise). internal_job_id: u64, @@ -147,8 +145,8 @@ impl EventHandler { /// Return true if a handler is "one shot": it fires at most once. fn is_one_shot(&self) -> bool { match self.desc { - EventDescription::ProcessExit { pid } => pid != ANY_PID, - EventDescription::JobExit { pid, .. } => pid != ANY_PID, + EventDescription::ProcessExit { pid } => pid.is_some(), + EventDescription::JobExit { pid, .. } => pid.is_some(), EventDescription::CallerExit { .. } => true, EventDescription::Signal { .. } | EventDescription::Variable { .. } @@ -171,7 +169,7 @@ impl EventHandler { ( EventDescription::ProcessExit { pid }, EventDescription::ProcessExit { pid: ev_pid }, - ) => *pid == ANY_PID || pid == ev_pid, + ) => pid.is_none() || pid == ev_pid, ( EventDescription::JobExit { pid, @@ -181,7 +179,7 @@ impl EventHandler { internal_job_id: ev_internal_job_id, .. }, - ) => *pid == ANY_PID || internal_job_id == ev_internal_job_id, + ) => pid.is_none() || internal_job_id == ev_internal_job_id, ( EventDescription::CallerExit { caller_id }, EventDescription::CallerExit { @@ -226,9 +224,9 @@ impl Event { } } - pub fn process_exit(pid: pid_t, status: i32) -> Self { + pub fn process_exit(pid: Pid, status: i32) -> Self { Self { - desc: EventDescription::ProcessExit { pid }, + desc: EventDescription::ProcessExit { pid: Some(pid) }, arguments: vec![ "PROCESS_EXIT".into(), pid.to_string().into(), @@ -237,10 +235,10 @@ impl Event { } } - pub fn job_exit(pgid: pid_t, jid: u64) -> Self { + pub fn job_exit(pgid: Pid, jid: u64) -> Self { Self { desc: EventDescription::JobExit { - pid: pgid, + pid: Some(pgid), internal_job_id: jid, }, arguments: vec![ @@ -382,12 +380,19 @@ pub fn get_desc(parser: &Parser, evt: &Event) -> WString { format!("signal handler for {} ({})", signal.name(), signal.desc(),) } EventDescription::Variable { name } => format!("handler for variable '{name}'"), - EventDescription::ProcessExit { pid } => format!("exit handler for process {pid}"), + EventDescription::ProcessExit { pid: None } => format!("exit handler for any process"), + EventDescription::ProcessExit { pid: Some(pid) } => { + format!("exit handler for process {pid}") + } EventDescription::JobExit { pid, .. } => { - if let Some(job) = parser.job_get_from_pid(*pid) { - format!("exit handler for job {}, '{}'", job.job_id(), job.command()) + if let Some(pid) = pid { + if let Some(job) = parser.job_get_from_pid(pid.as_pid_t()) { + format!("exit handler for job {}, '{}'", job.job_id(), job.command()) + } else { + format!("exit handler for job with pid {pid}") + } } else { - format!("exit handler for job with pid {pid}") + format!("exit handler for any job") } } EventDescription::CallerExit { .. } => { diff --git a/src/function.rs b/src/function.rs index 6f4408dd3..55dabc547 100644 --- a/src/function.rs +++ b/src/function.rs @@ -442,9 +442,11 @@ impl FunctionProperties { sprintf!(=> &mut out, " --on-variable %ls", name); } EventDescription::ProcessExit { pid } => { - sprintf!(=> &mut out, " --on-process-exit %d", pid); + let pid = pid.map(|p| p.get()).unwrap_or(0); + sprintf!(=> &mut out, " --on-process-exit %d", pid) } EventDescription::JobExit { pid, .. } => { + let pid = pid.map(|p| p.get()).unwrap_or(0); sprintf!(=> &mut out, " --on-job-exit %d", pid); } EventDescription::CallerExit { .. } => { diff --git a/src/proc.rs b/src/proc.rs index c429f7f61..e99bc9672 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -512,7 +512,7 @@ impl Drop for TtyTransfer { /// A type-safe equivalent to [`libc::pid_t`]. #[repr(transparent)] -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq)] pub struct Pid(NonZeroU32); impl Pid { @@ -1276,8 +1276,8 @@ pub fn print_exit_warning_for_jobs(jobs: &JobList) { /// Use the procfs filesystem to look up how many jiffies of cpu time was used by a given pid. This /// function is only available on systems with the procfs file entry 'stat', i.e. Linux. -pub fn proc_get_jiffies(inpid: libc::pid_t) -> ClockTicks { - if inpid <= 0 || !have_proc_stat() { +pub fn proc_get_jiffies(inpid: Pid) -> ClockTicks { + if !have_proc_stat() { return 0; } @@ -1315,7 +1315,7 @@ pub fn proc_update_jiffies(parser: &Parser) { for p in job.external_procs() { p.last_times.replace(ProcTimes { time: timef(), - jiffies: proc_get_jiffies(p.pid.load().map(|p| p.as_pid_t()).unwrap()), + jiffies: proc_get_jiffies(p.pid.load().unwrap()), }); } } @@ -1414,7 +1414,7 @@ pub fn hup_jobs(jobs: &JobList) { pub fn add_disowned_job(j: &Job) { let mut disowned_pids = DISOWNED_PIDS.get().borrow_mut(); for process in j.external_procs() { - disowned_pids.push(process.pid().unwrap().as_pid_t()); + disowned_pids.push(process.pid().unwrap()); } } @@ -1425,7 +1425,7 @@ fn reap_disowned_pids() { // if it has changed or an error occurs (presumably ECHILD because the child does not exist) disowned_pids.retain(|pid| { let mut status: libc::c_int = 0; - let ret = unsafe { libc::waitpid(*pid, &mut status, WNOHANG) }; + let ret = unsafe { libc::waitpid(pid.as_pid_t(), &mut status, WNOHANG) }; if ret > 0 { FLOGF!(proc_reap_external, "Reaped disowned PID or PGID %d", pid); } @@ -1435,8 +1435,7 @@ fn reap_disowned_pids() { /// 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 DISOWNED_PIDS: MainThread>> = - MainThread::new(RefCell::new(Vec::new())); +static DISOWNED_PIDS: MainThread>> = MainThread::new(RefCell::new(Vec::new())); /// See if any reapable processes have exited, and mark them accordingly. /// \param block_ok if no reapable processes have exited, block until one is (or until we receive a @@ -1598,7 +1597,7 @@ fn generate_process_exit_events(j: &Job, out_evts: &mut Vec) { if p.is_completed() && !p.posted_proc_exit.load() { p.posted_proc_exit.store(true); out_evts.push(Event::process_exit( - p.pid().unwrap().as_pid_t(), + p.pid().unwrap(), p.status.status_value(), )); } @@ -1613,7 +1612,7 @@ fn generate_job_exit_events(j: &Job, out_evts: &mut Vec) { // job_exit events. if j.posts_job_exit_events() { if let Some(last_pid) = j.get_last_pid() { - out_evts.push(Event::job_exit(last_pid.as_pid_t(), j.internal_job_id)); + out_evts.push(Event::job_exit(last_pid, j.internal_job_id)); } } }