Use strongly typed Option<Pid> 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

@@ -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(

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 => {
+            EventDescription::ProcessExit { pid: Some(pid) } => {
-                let wh = parser.get_wait_handles().get_by_pid(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 => {
+            EventDescription::JobExit { pid: Some(pid), .. } => {
-                let wh = parser.get_wait_handles().get_by_pid(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));

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 {

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.
+        /// Process ID. Use [`None`] to match any pid.
-        pid: pid_t,
+        pid: Option<Pid>,
     },
     /// An event triggered by a job exit.
     JobExit {
-        /// pid requested by the event, or [`ANY_PID`] for all.
+        /// pid requested by the event, or [`None`] for all.
-        pid: pid_t,
+        pid: Option<Pid>,
         /// `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::ProcessExit { pid } => pid.is_some(),
-            EventDescription::JobExit { pid, .. } => pid != ANY_PID,
+            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) {
+            if let Some(pid) = pid {
-                format!("exit handler for job {}, '{}'", job.job_id(), job.command())
+                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 { .. } => {

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 { .. } => {

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 {
+pub fn proc_get_jiffies(inpid: Pid) -> ClockTicks {
-    if inpid <= 0 || !have_proc_stat() {
+    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<RefCell<Vec<libc::pid_t>>> =
+static DISOWNED_PIDS: MainThread<RefCell<Vec<Pid>>> = MainThread::new(RefCell::new(Vec::new()));
-    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<Event>) {
             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<Event>) {
         // 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));
             }
         }
     }