Skip to content

Commit

Permalink
Fix Client::configure* on unix
Browse files Browse the repository at this point in the history 
Fixed #99
  • Loading branch information
@NobodyXu
NobodyXu committed Jul 16, 2024
1 parent c0c2898 commit 8b2da68
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 66 deletions.
11 changes: 8 additions & 3 deletions src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -50,6 +52,7 @@ impl FromEnvError {
FromEnvErrorInner::NegativeFd(..) => FromEnvErrorKind::NegativeFd,
FromEnvErrorInner::NotAPipe(..) => FromEnvErrorKind::NotAPipe,
FromEnvErrorInner::Unsupported => FromEnvErrorKind::Unsupported,
FromEnvErrorInner::CannotClone(..) => FromEnvErrorKind::CannotClone,
}
}
}
Expand All @@ -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,
}
}
Expand All @@ -92,4 +96,5 @@ pub(crate) enum FromEnvErrorInner {
NegativeFd(RawFd),
NotAPipe(RawFd, Option<std::io::Error>),
Unsupported,
CannotClone(RawFd, std::io::Error),
}
125 changes: 62 additions & 63 deletions src/unix.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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)]
Expand All @@ -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)?;

Expand Down Expand Up @@ -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)),
}))
}

Expand Down Expand Up @@ -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
Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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)?;
Expand Down

0 comments on commit 8b2da68

Please sign in to comment.