Use strongly typed Pid for job control

src/builtins/bg.rs

@@ -1,6 +1,6 @@
 // Implementation of the bg builtin.
 
-use libc::pid_t;
+use crate::proc::Pid;
 
 use super::prelude::*;
 
@@ -81,18 +81,19 @@ pub fn bg(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> Optio
     // If one argument is not a valid pid (i.e. integer >= 0), fail without backgrounding anything,
     // but still print errors for all of them.
     let mut retval: Option<i32> = STATUS_CMD_OK;
-    let pids: Vec<pid_t> = args[opts.optind..]
+    let pids: Vec<Pid> = args[opts.optind..]
         .iter()
-        .map(|arg| {
+        .filter_map(|arg| match fish_wcstoi(arg).map(Pid::new) {
-            fish_wcstoi(arg).unwrap_or_else(|_| {
+            Ok(Some(pid)) => Some(pid),
+            _ => {
                 streams.err.append(wgettext_fmt!(
                     "%ls: '%ls' is not a valid job specifier\n",
                     cmd,
                     arg
                 ));
                 retval = STATUS_INVALID_ARGS;
-                0
+                None
-            })
+            }
         })
         .collect();
 

src/builtins/disown.rs

@@ -3,7 +3,7 @@
 use super::shared::{builtin_print_help, STATUS_CMD_ERROR, STATUS_INVALID_ARGS};
 use crate::io::IoStreams;
 use crate::parser::Parser;
-use crate::proc::{add_disowned_job, Job};
+use crate::proc::{add_disowned_job, Job, Pid};
 use crate::{
     builtins::shared::{HelpOnlyCmdOpts, STATUS_CMD_OK},
     wchar::wstr,
@@ -80,6 +80,7 @@ pub fn disown(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O
             retval = STATUS_CMD_ERROR;
         }
     } else {
+        // TODO: This is supposed to be deduplicated or a hash set per comments below!
         let mut jobs = vec![];
 
         retval = STATUS_CMD_OK;
@@ -89,10 +90,7 @@ pub fn disown(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr]) -> O
         // Non-existent jobs aren't an error, but information about them is useful.
         // Multiple PIDs may refer to the same job; include the job only once by using a set.
         for arg in &args[1..] {
-            match fish_wcstoi(arg)
+            match fish_wcstoi(arg).ok().and_then(Pid::new) {
-                .ok()
-                .and_then(|pid| if pid < 0 { None } else { Some(pid) })
-            {
                 None => {
                     streams.err.append(wgettext_fmt!(
                         "%ls: '%ls' is not a valid job specifier\n",

src/builtins/fg.rs

@@ -1,6 +1,7 @@
 //! Implementation of the fg builtin.
 
 use crate::fds::make_fd_blocking;
+use crate::proc::Pid;
 use crate::reader::reader_write_title;
 use crate::tokenizer::tok_command;
 use crate::wutil::perror;
@@ -51,8 +52,8 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Optio
         // try to locate the job $argv[1], since we need to determine which error message to
         // emit (ambigous job specification vs malformed job id).
         let mut found_job = false;
-        match fish_wcstoi(argv[optind]) {
+        match fish_wcstoi(argv[optind]).map(Pid::new) {
-            Ok(pid) if pid > 0 => found_job = parser.job_get_from_pid(pid).is_some(),
+            Ok(Some(pid)) => found_job = parser.job_get_from_pid(pid).is_some(),
             _ => (),
         };
 
@@ -82,14 +83,15 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Optio
                 builtin_print_error_trailer(parser, streams.err, cmd);
             }
             Ok(pid) => {
-                let pid = pid.abs();
+                let raw_pid = pid;
-                let j = parser.job_get_with_index_from_pid(pid);
+                let pid = Pid::new(pid.abs());
+                let j = pid.and_then(|pid| parser.job_get_with_index_from_pid(pid));
                 if j.as_ref()
                     .map_or(true, |(_pos, j)| !j.is_constructed() || j.is_completed())
                 {
                     streams
                         .err
-                        .append(wgettext_fmt!("%ls: No suitable job: %d\n", cmd, pid));
+                        .append(wgettext_fmt!("%ls: No suitable job: %d\n", cmd, raw_pid));
                     job_pos = None;
                     job = None
                 } else {
@@ -102,7 +104,7 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Optio
                                 "it is not under job control\n"
                             ),
                             cmd,
-                            pid,
+                            raw_pid,
                             j.command()
                         ));
                         None

src/builtins/function.rs

@@ -58,7 +58,7 @@ const LONG_OPTIONS: &[WOption] = &[
 
 /// Return the internal_job_id for a pid, or None if none.
 /// This looks through both active and finished jobs.
-fn job_id_for_pid(pid: i32, parser: &Parser) -> Option<u64> {
+fn job_id_for_pid(pid: Pid, parser: &Parser) -> Option<u64> {
     if let Some(job) = parser.job_get_from_pid(pid) {
         Some(job.internal_job_id)
     } else {
@@ -164,7 +164,10 @@ fn parse_cmd_opts(
                         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);
+                        let internal_job_id = match Pid::new(pid) {
+                            Some(pid) => job_id_for_pid(pid, parser).unwrap_or(0),
+                            None => 0,
+                        };
                         e = EventDescription::JobExit {
                             pid: Pid::new(pid),
                             internal_job_id,
@@ -372,13 +375,13 @@ pub fn function(
     for ed in &opts.events {
         match *ed {
             EventDescription::ProcessExit { pid: Some(pid) } => {
-                let wh = parser.get_wait_handles().get_by_pid(pid.as_pid_t());
+                let wh = parser.get_wait_handles().get_by_pid(pid);
                 if let Some(status) = wh.and_then(|wh| wh.status()) {
                     event::fire(parser, event::Event::process_exit(pid, status));
                 }
             }
             EventDescription::JobExit { pid: Some(pid), .. } => {
-                let wh = parser.get_wait_handles().get_by_pid(pid.as_pid_t());
+                let wh = parser.get_wait_handles().get_by_pid(pid);
                 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

@@ -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};
+use crate::proc::{clock_ticks_to_seconds, have_proc_stat, proc_get_jiffies, Job, Pid};
 use crate::wchar_ext::WExt;
 use crate::wgetopt::{wopt, ArgType, WGetopter, WOption};
 use crate::wutil::wgettext;
@@ -211,7 +211,7 @@ pub fn jobs(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
                     }
                 }
             } else {
-                match fish_wcstoi(arg).ok().filter(|&pid| pid >= 0) {
+                match fish_wcstoi(arg).ok().and_then(Pid::new) {
                     None => {
                         streams.err.append(wgettext_fmt!(
                             "%ls: '%ls' is not a valid process id\n",

src/builtins/wait.rs

@@ -1,7 +1,5 @@
-use libc::pid_t;
-
 use super::prelude::*;
-use crate::proc::{proc_wait_any, Job};
+use crate::proc::{proc_wait_any, Job, Pid};
 use crate::signal::SigChecker;
 use crate::wait_handle::{WaitHandleRef, WaitHandleStore};
 
@@ -25,7 +23,7 @@ fn iswnumeric(s: &wstr) -> bool {
 
 #[derive(Copy, Clone)]
 enum WaitHandleQuery<'a> {
-    Pid(pid_t),
+    Pid(Pid),
     ProcName(&'a wstr),
 }
 
@@ -181,15 +179,15 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
     for item in &argv[optind..argc] {
         if iswnumeric(item) {
             // argument is pid
-            let mpid: pid_t = fish_wcstoi(item).unwrap_or(-1);
+            let mpid: i32 = fish_wcstoi(item).unwrap_or(-1);
-            if mpid <= 0 {
+            let Some(mpid) = Pid::new(mpid) else {
                 streams.err.append(wgettext_fmt!(
                     "%ls: '%ls' is not a valid process id\n",
                     cmd,
                     item,
                 ));
                 continue;
-            }
+            };
             if !find_wait_handles(WaitHandleQuery::Pid(mpid), parser, &mut wait_handles) {
                 streams.err.append(wgettext_fmt!(
                     "%ls: Could not find a job with process id '%d'\n",

src/event.rs

@@ -386,7 +386,7 @@ pub fn get_desc(parser: &Parser, evt: &Event) -> WString {
         }
         EventDescription::JobExit { pid, .. } => {
             if let Some(pid) = pid {
-                if let Some(job) = parser.job_get_from_pid(pid.as_pid_t()) {
+                if let Some(job) = parser.job_get_from_pid(*pid) {
                     format!("exit handler for job {}, '{}'", job.job_id(), job.command())
                 } else {
                     format!("exit handler for job with pid {pid}")

src/exec.rs

@@ -698,7 +698,7 @@ fn fork_child_for_process(
     let narrow_cmd = wcs2zstring(job.command());
     let narrow_argv0 = wcs2zstring(p.argv0().unwrap_or_default());
 
-    let mut pid = execute_fork();
+    let pid = execute_fork();
     if pid < 0 {
         return Err(());
     }
@@ -706,12 +706,12 @@ fn fork_child_for_process(
 
     // Record the pgroup if this is the leader.
     // Both parent and child attempt to send the process to its new group, to resolve the race.
-    p.set_pid(if is_parent {
+    let pid = if is_parent {
         pid
     } else {
-        pid = unsafe { libc::getpid() };
+        unsafe { libc::getpid() }
-        pid
+    };
-    });
+    p.set_pid(pid);
     if p.leads_pgrp {
         job.group().set_pgid(pid);
     }
@@ -1328,9 +1328,7 @@ fn exec_process_in_job(
             exec_external_command(parser, j, p, &process_net_io_chain)?;
             // It's possible (though unlikely) that this is a background process which recycled a
             // pid from another, previous background process. Forget any such old process.
-            parser
+            parser.mut_wait_handles().remove_by_pid(p.pid().unwrap());
-                .mut_wait_handles()
-                .remove_by_pid(p.pid().unwrap().as_pid_t());
             Ok(())
         }
         ProcessType::exec => {

src/parser.rs

@@ -23,7 +23,7 @@ use crate::parse_constants::{
 };
 use crate::parse_execution::{EndExecutionReason, ExecutionContext};
 use crate::parse_tree::{parse_source, LineCounter, ParsedSourceRef};
-use crate::proc::{job_reap, JobGroupRef, JobList, JobRef, ProcStatus};
+use crate::proc::{job_reap, JobGroupRef, JobList, JobRef, Pid, ProcStatus};
 use crate::signal::{signal_check_cancel, signal_clear_cancel, Signal};
 use crate::threads::assert_is_main_thread;
 use crate::util::get_time;
@@ -942,15 +942,15 @@ impl Parser {
     }
 
     /// Returns the job with the given pid.
-    pub fn job_get_from_pid(&self, pid: libc::pid_t) -> Option<JobRef> {
+    pub fn job_get_from_pid(&self, pid: Pid) -> Option<JobRef> {
         self.job_get_with_index_from_pid(pid).map(|t| t.1)
     }
 
     /// Returns the job and job index with the given pid.
-    pub fn job_get_with_index_from_pid(&self, pid: libc::pid_t) -> Option<(usize, JobRef)> {
+    pub fn job_get_with_index_from_pid(&self, pid: Pid) -> Option<(usize, JobRef)> {
         for (i, job) in self.jobs().iter().enumerate() {
             for p in job.external_procs() {
-                if p.pid.load().unwrap().get() == pid {
+                if p.pid.load().unwrap() == pid {
                     return Some((i, job.clone()));
                 }
             }

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, PartialOrd, Ord, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)]
 pub struct Pid(NonZeroU32);
 
 impl Pid {
@@ -780,7 +780,7 @@ impl Process {
         } else {
             if self.wait_handle.borrow().is_none() {
                 self.wait_handle.replace(Some(WaitHandle::new(
-                    self.pid().unwrap().get(),
+                    self.pid().unwrap(),
                     jid,
                     wbasename(&self.actual_cmd.clone()).to_owned(),
                 )));

src/wait_handle.rs

@@ -1,5 +1,5 @@
+use crate::proc::Pid;
 use crate::wchar::prelude::*;
-use libc::pid_t;
 use std::cell::Cell;
 use std::rc::Rc;
 
@@ -11,7 +11,7 @@ pub type InternalJobId = u64;
 /// This may outlive the job.
 pub struct WaitHandle {
     /// The pid of this process.
-    pub pid: pid_t,
+    pub pid: Pid,
 
     /// The internal job id of the job which contained this process.
     pub internal_job_id: InternalJobId,
@@ -42,7 +42,7 @@ impl WaitHandle {
 
 impl WaitHandle {
     /// Construct from a pid, job id, and base name.
-    pub fn new(pid: pid_t, internal_job_id: InternalJobId, base_name: WString) -> WaitHandleRef {
+    pub fn new(pid: Pid, internal_job_id: InternalJobId, base_name: WString) -> WaitHandleRef {
         Rc::new(WaitHandle {
             pid,
             internal_job_id,
@@ -60,7 +60,7 @@ const WAIT_HANDLE_STORE_DEFAULT_LIMIT: usize = 1024;
 /// Note this class is not safe for concurrent access.
 pub struct WaitHandleStore {
     // Map from pid to wait handles.
-    cache: lru::LruCache<pid_t, WaitHandleRef>,
+    cache: lru::LruCache<Pid, WaitHandleRef>,
 }
 
 impl WaitHandleStore {
@@ -84,7 +84,7 @@ impl WaitHandleStore {
 
     /// Return the wait handle for a pid, or None if there is none.
     /// This is a fast lookup.
-    pub fn get_by_pid(&self, pid: pid_t) -> Option<WaitHandleRef> {
+    pub fn get_by_pid(&self, pid: Pid) -> Option<WaitHandleRef> {
         self.cache.peek(&pid).cloned()
     }
 
@@ -99,7 +99,7 @@ impl WaitHandleStore {
     }
 
     /// Remove the wait handle for a pid, if present in this store.
-    pub fn remove_by_pid(&mut self, pid: pid_t) {
+    pub fn remove_by_pid(&mut self, pid: Pid) {
         self.cache.pop(&pid);
     }
 
@@ -125,25 +125,29 @@ fn test_wait_handles() {
     let mut whs = WaitHandleStore::new_with_capacity(limit);
     assert_eq!(whs.size(), 0);
 
-    assert!(whs.get_by_pid(5).is_none());
+    fn p(pid: i32) -> Pid {
+        Pid::new(pid).unwrap()
+    }
+
+    assert!(whs.get_by_pid(p(5)).is_none());
 
     // Duplicate pids drop oldest.
-    whs.add(WaitHandle::new(5, 0, L!("first").to_owned()));
+    whs.add(WaitHandle::new(p(5), 0, L!("first").to_owned()));
-    whs.add(WaitHandle::new(5, 0, L!("second").to_owned()));
+    whs.add(WaitHandle::new(p(5), 0, L!("second").to_owned()));
     assert_eq!(whs.size(), 1);
-    assert_eq!(whs.get_by_pid(5).unwrap().base_name, "second");
+    assert_eq!(whs.get_by_pid(p(5)).unwrap().base_name, "second");
 
-    whs.remove_by_pid(123);
+    whs.remove_by_pid(p(123));
     assert_eq!(whs.size(), 1);
-    whs.remove_by_pid(5);
+    whs.remove_by_pid(p(5));
     assert_eq!(whs.size(), 0);
 
     // Test evicting oldest.
-    whs.add(WaitHandle::new(1, 0, L!("1").to_owned()));
+    whs.add(WaitHandle::new(p(1), 0, L!("1").to_owned()));
-    whs.add(WaitHandle::new(2, 0, L!("2").to_owned()));
+    whs.add(WaitHandle::new(p(2), 0, L!("2").to_owned()));
-    whs.add(WaitHandle::new(3, 0, L!("3").to_owned()));
+    whs.add(WaitHandle::new(p(3), 0, L!("3").to_owned()));
-    whs.add(WaitHandle::new(4, 0, L!("4").to_owned()));
+    whs.add(WaitHandle::new(p(4), 0, L!("4").to_owned()));
-    whs.add(WaitHandle::new(5, 0, L!("5").to_owned()));
+    whs.add(WaitHandle::new(p(5), 0, L!("5").to_owned()));
     assert_eq!(whs.size(), 4);
 
     let entries = whs.get_list();

tests/checks/basic.fish

@@ -503,7 +503,7 @@ type --query cp
 echo $status
 #CHECK: 0
 
-jobs --query 0
+jobs --query 1
 echo $status
 #CHECK: 1