Fix Client::configure* on unix

Fixed #99

Author: NobodyXu

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,16 +69,17 @@ 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}"),
         }
     }
 }
 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<std::io::Error>),
     Unsupported,
+    CannotClone(RawFd, std::io::Error),
 }

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<Path>),
+}
+
+#[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<AtomicBool>,
 }
 
 #[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<Option<Acquired>> {
         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<usize> {
         let mut len = MaybeUninit::<c_int>::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)?;