diff --git a/src/error.rs b/src/error.rs index f3baaf0..cae82ea 100644 --- a/src/error.rs +++ b/src/error.rs @@ -36,6 +36,8 @@ pub enum FromEnvErrorKind { NotAPipe, /// Jobserver inheritance is not supported on this platform. Unsupported, + /// Cannot clone the jobserver fifo fd + CannotClone, } impl FromEnvError { @@ -50,6 +52,7 @@ impl FromEnvError { FromEnvErrorInner::NegativeFd(..) => FromEnvErrorKind::NegativeFd, FromEnvErrorInner::NotAPipe(..) => FromEnvErrorKind::NotAPipe, FromEnvErrorInner::Unsupported => FromEnvErrorKind::Unsupported, + FromEnvErrorInner::CannotClone(..) => FromEnvErrorKind::CannotClone, } } } @@ -66,6 +69,7 @@ impl std::fmt::Display for FromEnvError { FromEnvErrorInner::NotAPipe(fd, None) => write!(f, "file descriptor {fd} from the jobserver environment variable value is not a pipe"), FromEnvErrorInner::NotAPipe(fd, Some(err)) => write!(f, "file descriptor {fd} from the jobserver environment variable value is not a pipe: {err}"), FromEnvErrorInner::Unsupported => write!(f, "jobserver inheritance is not supported on this platform"), + FromEnvErrorInner::CannotClone(fd, err) => write!(f, "file descriptor {fd} created fromjobserver environment variable value cannot be cloned: {err}"), } } } @@ -73,9 +77,9 @@ impl std::error::Error for FromEnvError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self.inner { FromEnvErrorInner::CannotOpenPath(_, err) => Some(err), - FromEnvErrorInner::NotAPipe(_, Some(err)) | FromEnvErrorInner::CannotOpenFd(_, err) => { - Some(err) - } + FromEnvErrorInner::NotAPipe(_, Some(err)) + | FromEnvErrorInner::CannotOpenFd(_, err) + | FromEnvErrorInner::CannotClone(_, err) => Some(err), _ => None, } } @@ -92,4 +96,5 @@ pub(crate) enum FromEnvErrorInner { NegativeFd(RawFd), NotAPipe(RawFd, Option), Unsupported, + CannotClone(RawFd, std::io::Error), } diff --git a/src/unix.rs b/src/unix.rs index 8f3905e..048b0bc 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -6,7 +6,7 @@ use std::io::{self, Read, Write}; use std::mem; use std::mem::MaybeUninit; use std::os::unix::prelude::*; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::Command; use std::ptr; use std::sync::{ @@ -17,17 +17,19 @@ use std::thread::{self, Builder, JoinHandle}; use std::time::Duration; #[derive(Debug)] -pub enum Client { - /// `--jobserver-auth=R,W` - Pipe { read: File, write: File }, - /// `--jobserver-auth=fifo:PATH` - Fifo { - file: File, - path: PathBuf, - /// it can only go from false -> true but not the other way around, since that - /// could cause a race condition. - is_non_blocking: AtomicBool, - }, +enum ClientCreationArg { + Fds { read: c_int, write: c_int }, + Fifo(Box), +} + +#[derive(Debug)] +pub struct Client { + read: File, + write: File, + creation_arg: ClientCreationArg, + /// it can only go from Some(false) -> Some(true) but not the other way around, since that + /// could cause a race condition. + is_non_blocking: Option, } #[derive(Debug)] @@ -43,7 +45,7 @@ impl Client { // wrong! const BUFFER: [u8; 128] = [b'|'; 128]; - let mut write = client.write(); + let mut write = &client.write; set_nonblocking(write.as_raw_fd(), true)?; @@ -111,16 +113,20 @@ impl Client { FromEnvErrorInner::CannotParse("expected a path after `fifo:`".to_string()) })?; let path = Path::new(path_str); + let file = OpenOptions::new() .read(true) .write(true) .open(path) .map_err(|err| FromEnvErrorInner::CannotOpenPath(path_str.to_string(), err))?; - Ok(Some(Client::Fifo { - file, - path: path.into(), - is_non_blocking: AtomicBool::new(false), + Ok(Some(Client { + read: file + .try_clone() + .map_err(|err| FromEnvErrorInner::CannotClone(file.as_raw_fd(), err))?, + write: file, + creation_arg: ClientCreationArg::Fifo(path.into()), + is_non_blocking: Some(AtomicBool::new(false)), })) } @@ -148,6 +154,8 @@ impl Client { return Err(FromEnvErrorInner::NegativeFd(write)); } + let creation_arg = ClientCreationArg::Fds { read, write }; + // Ok so we've got two integers that look like file descriptors, but // for extra sanity checking let's see if they actually look like // valid files and instances of a pipe if feature enabled before we @@ -174,40 +182,36 @@ impl Client { // // I tested this on macOS 14 and Linux 6.5.13 #[cfg(target_os = "linux")] - if let Ok(Some(jobserver)) = - Self::from_fifo(&format!("fifo:/dev/fd/{}", read.as_raw_fd())) - { - return Ok(Some(jobserver)); + if let (Ok(read), Ok(write)) = ( + File::open(format!("/dev/fd/{}", read)), + OpenOptions::new() + .write(true) + .open(format!("/dev/fd/{}", write)), + ) { + return Ok(Some(Client { + read, + write, + creation_arg, + is_non_blocking: Some(AtomicBool::new(false)), + })); } } } - Ok(Some(Client::Pipe { + Ok(Some(Client { read: clone_fd_and_set_cloexec(read)?, write: clone_fd_and_set_cloexec(write)?, + creation_arg, + is_non_blocking: None, })) } unsafe fn from_fds(read: c_int, write: c_int) -> Client { - Client::Pipe { + Client { read: File::from_raw_fd(read), write: File::from_raw_fd(write), - } - } - - /// Gets the read end of our jobserver client. - fn read(&self) -> &File { - match self { - Client::Pipe { read, .. } => read, - Client::Fifo { file, .. } => file, - } - } - - /// Gets the write end of our jobserver client. - fn write(&self) -> &File { - match self { - Client::Pipe { write, .. } => write, - Client::Fifo { file, .. } => file, + creation_arg: ClientCreationArg::Fds { read, write }, + is_non_blocking: None, } } @@ -245,7 +249,7 @@ impl Client { // to shut us down, so we otherwise punt all errors upwards. unsafe { let mut fd: libc::pollfd = mem::zeroed(); - let mut read = self.read(); + let mut read = &self.read; fd.fd = read.as_raw_fd(); fd.events = libc::POLLIN; loop { @@ -284,19 +288,15 @@ impl Client { pub fn try_acquire(&self) -> io::Result> { let mut buf = [0]; + let mut fifo = &self.read; - let (mut fifo, is_non_blocking) = match self { - Self::Fifo { - file, - is_non_blocking, - .. - } => (file, is_non_blocking), - _ => return Err(io::ErrorKind::Unsupported.into()), - }; - - if !is_non_blocking.load(Ordering::Relaxed) { - set_nonblocking(fifo.as_raw_fd(), true)?; - is_non_blocking.store(true, Ordering::Relaxed); + if let Some(is_non_blocking) = self.is_non_blocking.as_ref() { + if !is_non_blocking.load(Ordering::Relaxed) { + set_nonblocking(fifo.as_raw_fd(), true)?; + is_non_blocking.store(true, Ordering::Relaxed); + } + } else { + return Err(io::ErrorKind::Unsupported.into()); } loop { @@ -323,7 +323,7 @@ impl Client { // always quickly release a token). If that turns out to not be the // case we'll get an error anyway! let byte = data.map(|d| d.byte).unwrap_or(b'+'); - match self.write().write(&[byte])? { + match (&self.write).write(&[byte])? { 1 => Ok(()), _ => Err(io::Error::new( io::ErrorKind::Other, @@ -333,31 +333,30 @@ impl Client { } pub fn string_arg(&self) -> String { - match self { - Client::Pipe { read, write } => format!("{},{}", read.as_raw_fd(), write.as_raw_fd()), - Client::Fifo { path, .. } => format!("fifo:{}", path.to_str().unwrap()), + match &self.creation_arg { + ClientCreationArg::Fifo(path) => format!("fifo:{}", path.display()), + ClientCreationArg::Fds { read, write } => format!("{},{}", read, write), } } pub fn available(&self) -> io::Result { let mut len = MaybeUninit::::uninit(); - cvt(unsafe { libc::ioctl(self.read().as_raw_fd(), libc::FIONREAD, len.as_mut_ptr()) })?; + cvt(unsafe { libc::ioctl(self.read.as_raw_fd(), libc::FIONREAD, len.as_mut_ptr()) })?; Ok(unsafe { len.assume_init() } as usize) } pub fn configure(&self, cmd: &mut Command) { - match self { + if matches!(self.creation_arg, ClientCreationArg::Fifo { .. }) { // We `File::open`ed it when inheriting from environment, // so no need to set cloexec for fifo. - Client::Fifo { .. } => return, - Client::Pipe { .. } => {} - }; + return; + } // Here we basically just want to say that in the child process // we'll configure the read/write file descriptors to *not* be // cloexec, so they're inherited across the exec and specified as // integers through `string_arg` above. - let read = self.read().as_raw_fd(); - let write = self.write().as_raw_fd(); + let read = self.read.as_raw_fd(); + let write = self.write.as_raw_fd(); unsafe { cmd.pre_exec(move || { set_cloexec(read, false)?;