diff --git a/src/builtins/bg.rs b/src/builtins/bg.rs index 8bc4545cd..10373603a 100644 --- a/src/builtins/bg.rs +++ b/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 = STATUS_CMD_OK; - let pids: Vec = args[opts.optind..] + let pids: Vec = args[opts.optind..] .iter() - .map(|arg| { - fish_wcstoi(arg).unwrap_or_else(|_| { + .filter_map(|arg| match fish_wcstoi(arg).map(Pid::new) { + 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(); diff --git a/src/builtins/disown.rs b/src/builtins/disown.rs index 61905527a..41c4ef939 100644 --- a/src/builtins/disown.rs +++ b/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) - .ok() - .and_then(|pid| if pid < 0 { None } else { Some(pid) }) - { + match fish_wcstoi(arg).ok().and_then(Pid::new) { None => { streams.err.append(wgettext_fmt!( "%ls: '%ls' is not a valid job specifier\n", diff --git a/src/builtins/fg.rs b/src/builtins/fg.rs index 3dd092f39..26ffe1738 100644 --- a/src/builtins/fg.rs +++ b/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]) { - Ok(pid) if pid > 0 => found_job = parser.job_get_from_pid(pid).is_some(), + match fish_wcstoi(argv[optind]).map(Pid::new) { + 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 j = parser.job_get_with_index_from_pid(pid); + let raw_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 diff --git a/src/builtins/function.rs b/src/builtins/function.rs index fa926e750..4bf4a81b2 100644 --- a/src/builtins/function.rs +++ b/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 { +fn job_id_for_pid(pid: Pid, parser: &Parser) -> Option { 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)); diff --git a/src/builtins/jobs.rs b/src/builtins/jobs.rs index 89b7ef9c0..a0828f611 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}; +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", diff --git a/src/builtins/wait.rs b/src/builtins/wait.rs index a5d730965..f49233259 100644 --- a/src/builtins/wait.rs +++ b/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); - if mpid <= 0 { + let mpid: i32 = fish_wcstoi(item).unwrap_or(-1); + 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", diff --git a/src/event.rs b/src/event.rs index 330836524..77fc30139 100644 --- a/src/event.rs +++ b/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}") diff --git a/src/exec.rs b/src/exec.rs index 655382b65..a4bc34333 100644 --- a/src/exec.rs +++ b/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() }; - pid - }); + unsafe { libc::getpid() } + }; + 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 - .mut_wait_handles() - .remove_by_pid(p.pid().unwrap().as_pid_t()); + parser.mut_wait_handles().remove_by_pid(p.pid().unwrap()); Ok(()) } ProcessType::exec => { diff --git a/src/parser.rs b/src/parser.rs index 295b36ced..12457f037 100644 --- a/src/parser.rs +++ b/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 { + pub fn job_get_from_pid(&self, pid: Pid) -> Option { 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())); } } diff --git a/src/proc.rs b/src/proc.rs index e99bc9672..1aaa1e928 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, 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(), ))); diff --git a/src/wait_handle.rs b/src/wait_handle.rs index 8cddb1e54..dfdc3d331 100644 --- a/src/wait_handle.rs +++ b/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, + cache: lru::LruCache, } 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 { + pub fn get_by_pid(&self, pid: Pid) -> Option { 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(5, 0, L!("second").to_owned())); + whs.add(WaitHandle::new(p(5), 0, L!("first").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(2, 0, L!("2").to_owned())); - whs.add(WaitHandle::new(3, 0, L!("3").to_owned())); - whs.add(WaitHandle::new(4, 0, L!("4").to_owned())); - whs.add(WaitHandle::new(5, 0, L!("5").to_owned())); + whs.add(WaitHandle::new(p(1), 0, L!("1").to_owned())); + whs.add(WaitHandle::new(p(2), 0, L!("2").to_owned())); + whs.add(WaitHandle::new(p(3), 0, L!("3").to_owned())); + whs.add(WaitHandle::new(p(4), 0, L!("4").to_owned())); + whs.add(WaitHandle::new(p(5), 0, L!("5").to_owned())); assert_eq!(whs.size(), 4); let entries = whs.get_list(); diff --git a/tests/checks/basic.fish b/tests/checks/basic.fish index eedc04035..64264fab9 100644 --- a/tests/checks/basic.fish +++ b/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