From 23e8b7ee60fe5154849f356786c2882106f39901 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Mon, 6 Mar 2023 17:33:58 +0300 Subject: [PATCH 01/13] [diagnostic] extend from_env function --- Cargo.toml | 3 +++ src/lib.rs | 64 +++++++++++++++++++++++++++++----------------- src/unix.rs | 68 +++++++++++++++++++++++++++++++++++-------------- src/wasm.rs | 9 +++++-- src/windows.rs | 14 +++++++--- tests/client.rs | 6 ++--- 6 files changed, 112 insertions(+), 52 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 807edda..51f3d05 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,9 @@ edition = "2018" [target.'cfg(unix)'.dependencies] libc = "0.2.50" +[features] +check_pipe = [] + [dev-dependencies] futures = "0.1" num_cpus = "1.0" diff --git a/src/lib.rs b/src/lib.rs index cd0cdd7..a775476 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,8 +29,8 @@ //! //! // See API documentation for why this is `unsafe` //! let client = match unsafe { Client::from_env() } { -//! Some(client) => client, -//! None => panic!("client not configured"), +//! Ok(client) => client, +//! Err(_) => panic!("client not configured"), //! }; //! ``` //! @@ -151,6 +151,22 @@ struct HelperInner { consumer_done: bool, } +/// Error type for `from_env` function. +#[derive(Debug)] +pub enum ErrFromEnv { + /// There isn't env var, that describes jobserver to inherit. + IsNotConfigured, + /// Cannot connect following this process's environment. + PlatformSpecific { + /// Error. + err: imp::ErrFromEnv, + /// Name of gotten env var. + env: &'static str, + /// Value of gotten env var. + var: String, + }, +} + impl Client { /// Creates a new jobserver initialized with the given parallelism limit. /// @@ -197,8 +213,9 @@ impl Client { /// # Return value /// /// If a jobserver was found in the environment and it looks correct then - /// `Some` of the connected client will be returned. If no jobserver was - /// found then `None` will be returned. + /// `Ok` of the connected client will be returned. If no relevant env var + /// was found then `Err(IsNotConfigured)` will be returned. In other cases + /// `Err(PlatformSpecific)` will be returned. /// /// Note that on Unix the `Client` returned **takes ownership of the file /// descriptors specified in the environment**. Jobservers on Unix are @@ -211,6 +228,9 @@ impl Client { /// with `CLOEXEC` so they're not automatically inherited by spawned /// children. /// + /// There is a feature to configure, will this function check if provided + /// files are actually pipes. + /// /// # Safety /// /// This function is `unsafe` to call on Unix specifically as it @@ -227,28 +247,24 @@ impl Client { /// /// Note, though, that on Windows it should be safe to call this function /// any number of times. - pub unsafe fn from_env() -> Option { - let var = match env::var("CARGO_MAKEFLAGS") - .or_else(|_| env::var("MAKEFLAGS")) - .or_else(|_| env::var("MFLAGS")) - { - Ok(s) => s, - Err(_) => return None, - }; - let mut arg = "--jobserver-fds="; - let pos = match var.find(arg) { - Some(i) => i, - None => { - arg = "--jobserver-auth="; - match var.find(arg) { - Some(i) => i, - None => return None, - } - } - }; + pub unsafe fn from_env() -> Result { + let (env, var) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] + .iter() + .map(|&env| env::var(env).map(|var| (env, var))) + .find_map(|p| p.ok()) + .ok_or(ErrFromEnv::IsNotConfigured)?; + + let (arg, pos) = ["--jobserver-fds=", "--jobserver-auth="] + .iter() + .map(|&arg| var.find(arg).map(|pos| (arg, pos))) + .find_map(|pos| pos) + .ok_or(ErrFromEnv::IsNotConfigured)?; let s = var[pos + arg.len()..].split(' ').next().unwrap(); - imp::Client::open(s).map(|c| Client { inner: Arc::new(c) }) + match imp::Client::open(s) { + Ok(c) => Ok(Client { inner: Arc::new(c) }), + Err(err) => Err(ErrFromEnv::PlatformSpecific { err, env, var }), + } } /// Acquires a token from this jobserver client. diff --git a/src/unix.rs b/src/unix.rs index e4b1435..b61a85c 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -25,6 +25,13 @@ pub struct Acquired { byte: u8, } +#[derive(Debug)] +pub enum ErrFromEnv { + ParseEnvVar, + OpenFile(String), + InvalidDescriptor(i32, i32), +} + impl Client { pub fn new(mut limit: usize) -> io::Result { let client = unsafe { Client::mk()? }; @@ -81,61 +88,66 @@ impl Client { Ok(Client::from_fds(pipes[0], pipes[1])) } - pub unsafe fn open(s: &str) -> Option { - Client::from_fifo(s).or_else(|| Client::from_pipe(s)) + pub unsafe fn open(s: &str) -> Result { + match (Self::from_fifo(s), Self::from_pipe(s)) { + (Some(result), None) | (None, Some(result)) => result, + (None, None) => Err(ErrFromEnv::ParseEnvVar), + (Some(_), Some(_)) => unreachable!(), + } } /// `--jobserver-auth=fifo:PATH` - fn from_fifo(s: &str) -> Option { + fn from_fifo(s: &str) -> Option> { let mut parts = s.splitn(2, ':'); if parts.next().unwrap() != "fifo" { return None; } let path = match parts.next() { Some(p) => Path::new(p), - None => return None, + None => return Some(Err(ErrFromEnv::ParseEnvVar)), }; let file = match OpenOptions::new().read(true).write(true).open(path) { Ok(f) => f, - Err(_) => return None, + Err(e) => return Some(Err(ErrFromEnv::OpenFile(e.to_string()))), }; - Some(Client::Fifo { + Some(Ok(Client::Fifo { file, path: path.into(), - }) + })) } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> Option { + unsafe fn from_pipe(s: &str) -> Option> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); let write = match parts.next() { Some(s) => s, - None => return None, + None => return Some(Err(ErrFromEnv::ParseEnvVar)), }; let read = match read.parse() { Ok(n) => n, - Err(_) => return None, + Err(_) => return Some(Err(ErrFromEnv::ParseEnvVar)), }; let write = match write.parse() { Ok(n) => n, - Err(_) => return None, + Err(_) => return Some(Err(ErrFromEnv::ParseEnvVar)), }; // 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 - // instances of a pipe before we return the client. + // instances of a pipe if feature enabled or valid files otherwise + // before we return the client. // // If we're called from `make` *without* the leading + on our rule // then we'll have `MAKEFLAGS` env vars but won't actually have // access to the file descriptors. - if is_valid_fd(read) && is_valid_fd(write) { + if check_fd(read) && check_fd(write) { drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); - Some(Client::from_fds(read, write)) + Some(Ok(Client::from_fds(read, write))) } else { - None + Some(Err(ErrFromEnv::InvalidDescriptor(read, write))) } } @@ -207,7 +219,7 @@ impl Client { return Err(io::Error::new( io::ErrorKind::Other, "early EOF on jobserver pipe", - )) + )); } Err(e) => match e.kind() { io::ErrorKind::WouldBlock => { /* fall through to polling */ } @@ -326,7 +338,7 @@ pub(crate) fn spawn_helper( client: client.inner.clone(), data, disabled: false, - })) + })); } Err(e) => break f(Err(e)), Ok(None) if helper.producer_done() => break, @@ -385,8 +397,26 @@ impl Helper { } } -fn is_valid_fd(fd: c_int) -> bool { - unsafe { libc::fcntl(fd, libc::F_GETFD) != -1 } +fn check_fd(fd: c_int) -> bool { + #[cfg(feature = "check_pipe")] + unsafe { + let mut stat = mem::zeroed(); + if libc::fstat(fd, &mut stat) == 0 { + // On android arm and i686 mode_t is u16 and st_mode is u32, + // this generates a type mismatch when S_IFIFO (declared as mode_t) + // is used in operations with st_mode, so we use this workaround + // to get the value of S_IFIFO with the same type of st_mode. + let mut s_ififo = stat.st_mode; + s_ififo = libc::S_IFIFO as _; + stat.st_mode & s_ififo == s_ififo + } else { + false + } + } + #[cfg(not(feature = "check_pipe"))] + unsafe { + libc::fcntl(fd, libc::F_GETFD) != -1 + } } fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> { diff --git a/src/wasm.rs b/src/wasm.rs index 3793bd6..749b994 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -17,6 +17,11 @@ struct Inner { #[derive(Debug)] pub struct Acquired(()); +#[derive(Debug)] +pub enum ErrFromEnv { + UnavailableOnTarget, +} + impl Client { pub fn new(limit: usize) -> io::Result { Ok(Client { @@ -27,8 +32,8 @@ impl Client { }) } - pub unsafe fn open(_s: &str) -> Option { - None + pub unsafe fn open(_s: &str) -> Result { + Err(ErrFromEnv::UnavailableOnTarget) } pub fn acquire(&self) -> io::Result { diff --git a/src/windows.rs b/src/windows.rs index 6791efe..7ae40e8 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -14,6 +14,12 @@ pub struct Client { #[derive(Debug)] pub struct Acquired; +#[derive(Debug)] +pub enum ErrFromEnv { + IsNotCString, + CannotAcquireSemaphore, +} + type BOOL = i32; type DWORD = u32; type HANDLE = *mut u8; @@ -127,17 +133,17 @@ impl Client { )) } - pub unsafe fn open(s: &str) -> Option { + pub unsafe fn open(s: &str) -> Result { let name = match CString::new(s) { Ok(s) => s, - Err(_) => return None, + Err(_) => return Err(ErrFromEnv::IsNotCString), }; let sem = OpenSemaphoreA(SYNCHRONIZE | SEMAPHORE_MODIFY_STATE, FALSE, name.as_ptr()); if sem.is_null() { - None + Err(CannotAcquireSemaphore) } else { - Some(Client { + Ok(Client { sem: Handle(sem), name: s.to_string(), }) diff --git a/tests/client.rs b/tests/client.rs index 2516b8c..bad343e 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -35,7 +35,7 @@ const TESTS: &[Test] = &[ make_args: &[], rule: &|me| me.to_string(), f: &|| { - assert!(unsafe { Client::from_env().is_none() }); + assert!(unsafe { Client::from_env().is_err() }); }, }, Test { @@ -43,7 +43,7 @@ const TESTS: &[Test] = &[ make_args: &[], rule: &|me| format!("+{}", me), f: &|| { - assert!(unsafe { Client::from_env().is_none() }); + assert!(unsafe { Client::from_env().is_err() }); }, }, Test { @@ -51,7 +51,7 @@ const TESTS: &[Test] = &[ make_args: &["-j2"], rule: &|me| format!("+{}", me), f: &|| { - assert!(unsafe { Client::from_env().is_some() }); + assert!(unsafe { Client::from_env().is_ok() }); }, }, Test { From 3275a9a8dd1d3300fd7632a54526f0646b253421 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Sun, 12 Mar 2023 15:30:21 +0300 Subject: [PATCH 02/13] [temp] fix --- src/unix.rs | 4 ++-- src/windows.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index b61a85c..69c30e5 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -90,9 +90,9 @@ impl Client { pub unsafe fn open(s: &str) -> Result { match (Self::from_fifo(s), Self::from_pipe(s)) { - (Some(result), None) | (None, Some(result)) => result, + (Some(Ok(c)), _) | (_, Some(Ok(c))) => Ok(c), + (Some(Err(e)), _) | (_, Some(Err(e))) => Err(e), (None, None) => Err(ErrFromEnv::ParseEnvVar), - (Some(_), Some(_)) => unreachable!(), } } diff --git a/src/windows.rs b/src/windows.rs index 7ae40e8..d67a373 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -141,7 +141,7 @@ impl Client { let sem = OpenSemaphoreA(SYNCHRONIZE | SEMAPHORE_MODIFY_STATE, FALSE, name.as_ptr()); if sem.is_null() { - Err(CannotAcquireSemaphore) + Err(ErrFromEnv::CannotAcquireSemaphore) } else { Ok(Client { sem: Handle(sem), From 8b50514062fc3cbfaf00f1a9527b54db13557cef Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 12:46:25 +0300 Subject: [PATCH 03/13] [temp] move Option into Result --- src/unix.rs | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 69c30e5..4688cec 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -89,50 +89,33 @@ impl Client { } pub unsafe fn open(s: &str) -> Result { - match (Self::from_fifo(s), Self::from_pipe(s)) { - (Some(Ok(c)), _) | (_, Some(Ok(c))) => Ok(c), - (Some(Err(e)), _) | (_, Some(Err(e))) => Err(e), - (None, None) => Err(ErrFromEnv::ParseEnvVar), - } + Ok(Self::from_fifo(s)?.unwrap_or(Self::from_pipe(s)?.ok_or(ErrFromEnv::ParseEnvVar)?)) } /// `--jobserver-auth=fifo:PATH` - fn from_fifo(s: &str) -> Option> { + fn from_fifo(s: &str) -> Result, ErrFromEnv> { let mut parts = s.splitn(2, ':'); if parts.next().unwrap() != "fifo" { - return None; + return Ok(None); } - let path = match parts.next() { - Some(p) => Path::new(p), - None => return Some(Err(ErrFromEnv::ParseEnvVar)), - }; + let path = Path::new(parts.next().ok_or(ErrFromEnv::ParseEnvVar)?); let file = match OpenOptions::new().read(true).write(true).open(path) { Ok(f) => f, - Err(e) => return Some(Err(ErrFromEnv::OpenFile(e.to_string()))), + Err(e) => return Err(ErrFromEnv::OpenFile(e.to_string())), }; - Some(Ok(Client::Fifo { + Ok(Some(Client::Fifo { file, path: path.into(), })) } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> Option> { + unsafe fn from_pipe(s: &str) -> Result, ErrFromEnv> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); - let write = match parts.next() { - Some(s) => s, - None => return Some(Err(ErrFromEnv::ParseEnvVar)), - }; - - let read = match read.parse() { - Ok(n) => n, - Err(_) => return Some(Err(ErrFromEnv::ParseEnvVar)), - }; - let write = match write.parse() { - Ok(n) => n, - Err(_) => return Some(Err(ErrFromEnv::ParseEnvVar)), - }; + let write = parts.next().ok_or(ErrFromEnv::ParseEnvVar)?; + let read = read.parse().map_err(|_| ErrFromEnv::ParseEnvVar)?; + let write = write.parse().map_err(|_| ErrFromEnv::ParseEnvVar)?; // 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 @@ -145,9 +128,9 @@ impl Client { if check_fd(read) && check_fd(write) { drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); - Some(Ok(Client::from_fds(read, write))) + Ok(Some(Client::from_fds(read, write))) } else { - Some(Err(ErrFromEnv::InvalidDescriptor(read, write))) + Err(ErrFromEnv::InvalidDescriptor(read, write)) } } From 6e047a5fdcaa0b70340ea7077afc9e4db42e14ec Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 14:45:32 +0300 Subject: [PATCH 04/13] [temp] remove Option from from_pipe --- src/unix.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 4688cec..38d7cb7 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -89,7 +89,7 @@ impl Client { } pub unsafe fn open(s: &str) -> Result { - Ok(Self::from_fifo(s)?.unwrap_or(Self::from_pipe(s)?.ok_or(ErrFromEnv::ParseEnvVar)?)) + Ok(Self::from_fifo(s)?.unwrap_or(Self::from_pipe(s)?)) } /// `--jobserver-auth=fifo:PATH` @@ -110,7 +110,7 @@ impl Client { } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> Result, ErrFromEnv> { + unsafe fn from_pipe(s: &str) -> Result { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); let write = parts.next().ok_or(ErrFromEnv::ParseEnvVar)?; @@ -128,7 +128,7 @@ impl Client { if check_fd(read) && check_fd(write) { drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); - Ok(Some(Client::from_fds(read, write))) + Ok(Client::from_fds(read, write)) } else { Err(ErrFromEnv::InvalidDescriptor(read, write)) } From 621665f92b304bd79650b642dc54ce0c9ce0d962 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 14:49:20 +0300 Subject: [PATCH 05/13] [temp] switch to io::Error --- src/lib.rs | 2 +- src/unix.rs | 41 ++++++++++++++++++++++------------------- src/wasm.rs | 9 ++------- src/windows.rs | 15 +++------------ 4 files changed, 28 insertions(+), 39 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a775476..b6ee761 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -159,7 +159,7 @@ pub enum ErrFromEnv { /// Cannot connect following this process's environment. PlatformSpecific { /// Error. - err: imp::ErrFromEnv, + err: io::Error, /// Name of gotten env var. env: &'static str, /// Value of gotten env var. diff --git a/src/unix.rs b/src/unix.rs index 38d7cb7..49c0d00 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -25,13 +25,6 @@ pub struct Acquired { byte: u8, } -#[derive(Debug)] -pub enum ErrFromEnv { - ParseEnvVar, - OpenFile(String), - InvalidDescriptor(i32, i32), -} - impl Client { pub fn new(mut limit: usize) -> io::Result { let client = unsafe { Client::mk()? }; @@ -88,21 +81,21 @@ impl Client { Ok(Client::from_fds(pipes[0], pipes[1])) } - pub unsafe fn open(s: &str) -> Result { + pub unsafe fn open(s: &str) -> io::Result { Ok(Self::from_fifo(s)?.unwrap_or(Self::from_pipe(s)?)) } /// `--jobserver-auth=fifo:PATH` - fn from_fifo(s: &str) -> Result, ErrFromEnv> { + fn from_fifo(s: &str) -> io::Result> { let mut parts = s.splitn(2, ':'); if parts.next().unwrap() != "fifo" { return Ok(None); } - let path = Path::new(parts.next().ok_or(ErrFromEnv::ParseEnvVar)?); - let file = match OpenOptions::new().read(true).write(true).open(path) { - Ok(f) => f, - Err(e) => return Err(ErrFromEnv::OpenFile(e.to_string())), - }; + let path = Path::new(parts.next().ok_or(io::Error::new( + io::ErrorKind::InvalidInput, + "expected ':' after `fifo`", + ))?); + let file = OpenOptions::new().read(true).write(true).open(path)?; Ok(Some(Client::Fifo { file, path: path.into(), @@ -110,12 +103,19 @@ impl Client { } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> Result { + unsafe fn from_pipe(s: &str) -> io::Result { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); - let write = parts.next().ok_or(ErrFromEnv::ParseEnvVar)?; - let read = read.parse().map_err(|_| ErrFromEnv::ParseEnvVar)?; - let write = write.parse().map_err(|_| ErrFromEnv::ParseEnvVar)?; + let write = parts.next().ok_or(io::Error::new( + io::ErrorKind::InvalidInput, + "expected ',' in `auth=R,W`", + ))?; + let read = read + .parse() + .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; + let write = write + .parse() + .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; // 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 @@ -130,7 +130,10 @@ impl Client { drop(set_cloexec(write, true)); Ok(Client::from_fds(read, write)) } else { - Err(ErrFromEnv::InvalidDescriptor(read, write)) + Err(io::Error::new( + io::ErrorKind::InvalidData, + "invalid file descriptors", + )) } } diff --git a/src/wasm.rs b/src/wasm.rs index 749b994..a542fcf 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -17,11 +17,6 @@ struct Inner { #[derive(Debug)] pub struct Acquired(()); -#[derive(Debug)] -pub enum ErrFromEnv { - UnavailableOnTarget, -} - impl Client { pub fn new(limit: usize) -> io::Result { Ok(Client { @@ -32,8 +27,8 @@ impl Client { }) } - pub unsafe fn open(_s: &str) -> Result { - Err(ErrFromEnv::UnavailableOnTarget) + pub unsafe fn open(_s: &str) -> io::Result { + Err(io::ErrorKind::Unsupported.into()) } pub fn acquire(&self) -> io::Result { diff --git a/src/windows.rs b/src/windows.rs index d67a373..bea8c05 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -14,12 +14,6 @@ pub struct Client { #[derive(Debug)] pub struct Acquired; -#[derive(Debug)] -pub enum ErrFromEnv { - IsNotCString, - CannotAcquireSemaphore, -} - type BOOL = i32; type DWORD = u32; type HANDLE = *mut u8; @@ -133,15 +127,12 @@ impl Client { )) } - pub unsafe fn open(s: &str) -> Result { - let name = match CString::new(s) { - Ok(s) => s, - Err(_) => return Err(ErrFromEnv::IsNotCString), - }; + pub unsafe fn open(s: &str) -> io::Result { + let name = CString::new(s).map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; let sem = OpenSemaphoreA(SYNCHRONIZE | SEMAPHORE_MODIFY_STATE, FALSE, name.as_ptr()); if sem.is_null() { - Err(ErrFromEnv::CannotAcquireSemaphore) + Err(io::Error::last_os_error()) } else { Ok(Client { sem: Handle(sem), From 1cd2091d3b801624fdf3d37a79746d113a6a8378 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 15:04:52 +0300 Subject: [PATCH 06/13] [temp] Display and Error for ErrFromEnv --- src/lib.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index b6ee761..2180a4f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -167,6 +167,28 @@ pub enum ErrFromEnv { }, } +impl std::fmt::Display for ErrFromEnv { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ErrFromEnv::IsNotConfigured => { + write!(f, "couldn't find relevant environment variable") + } + ErrFromEnv::PlatformSpecific { err, env, var } => { + write!(f, "{err} ({env}={var}") + } + } + } +} + +impl std::error::Error for ErrFromEnv { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ErrFromEnv::IsNotConfigured => None, + ErrFromEnv::PlatformSpecific { err, .. } => Some(err), + } + } +} + impl Client { /// Creates a new jobserver initialized with the given parallelism limit. /// From 39b279011c339c935ed8c7624328dd3c331ec426 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 15:22:43 +0300 Subject: [PATCH 07/13] [temp] improve check_fd diagnostic --- src/unix.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 49c0d00..6233501 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -125,16 +125,11 @@ impl Client { // If we're called from `make` *without* the leading + on our rule // then we'll have `MAKEFLAGS` env vars but won't actually have // access to the file descriptors. - if check_fd(read) && check_fd(write) { - drop(set_cloexec(read, true)); - drop(set_cloexec(write, true)); - Ok(Client::from_fds(read, write)) - } else { - Err(io::Error::new( - io::ErrorKind::InvalidData, - "invalid file descriptors", - )) - } + check_fd(read)?; + check_fd(write)?; + drop(set_cloexec(read, true)); + drop(set_cloexec(write, true)); + Ok(Client::from_fds(read, write)) } unsafe fn from_fds(read: c_int, write: c_int) -> Client { @@ -383,25 +378,34 @@ impl Helper { } } -fn check_fd(fd: c_int) -> bool { +fn check_fd(fd: c_int) -> io::Result<()> { #[cfg(feature = "check_pipe")] unsafe { let mut stat = mem::zeroed(); - if libc::fstat(fd, &mut stat) == 0 { + if libc::fstat(fd, &mut stat) == -1 { + Err(io::Error::last_os_error()) + } else { // On android arm and i686 mode_t is u16 and st_mode is u32, // this generates a type mismatch when S_IFIFO (declared as mode_t) // is used in operations with st_mode, so we use this workaround // to get the value of S_IFIFO with the same type of st_mode. let mut s_ififo = stat.st_mode; s_ififo = libc::S_IFIFO as _; - stat.st_mode & s_ififo == s_ififo - } else { - false + if stat.st_mode & s_ififo == s_ififo { + return Ok(()); + } + Err(io::Error::last_os_error()) // } } #[cfg(not(feature = "check_pipe"))] unsafe { - libc::fcntl(fd, libc::F_GETFD) != -1 + match libc::fcntl(fd, libc::F_GETFD) { + r if r == -1 => Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("{fd} is not a pipe"), + )), + _ => Ok(()), + } } } From 85fc976d245b5ebc36469ee1836d5c20e25a4948 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 15:23:52 +0300 Subject: [PATCH 08/13] [temp] switch default to check_pipe --- Cargo.toml | 2 +- src/unix.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 51f3d05..f41f407 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ edition = "2018" libc = "0.2.50" [features] -check_pipe = [] +do_not_check_pipe = [] [dev-dependencies] futures = "0.1" diff --git a/src/unix.rs b/src/unix.rs index 6233501..aedde0e 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -379,7 +379,7 @@ impl Helper { } fn check_fd(fd: c_int) -> io::Result<()> { - #[cfg(feature = "check_pipe")] + #[cfg(not(feature = "do_not_check_pipe"))] unsafe { let mut stat = mem::zeroed(); if libc::fstat(fd, &mut stat) == -1 { @@ -397,7 +397,7 @@ fn check_fd(fd: c_int) -> io::Result<()> { Err(io::Error::last_os_error()) // } } - #[cfg(not(feature = "check_pipe"))] + #[cfg(feature = "do_not_check_pipe")] unsafe { match libc::fcntl(fd, libc::F_GETFD) { r if r == -1 => Err(io::Error::new( From a9003015d4a19abcde561dbe950510988da0fe91 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 15:36:38 +0300 Subject: [PATCH 09/13] [temp] fix --- src/unix.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/unix.rs b/src/unix.rs index aedde0e..702a047 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -82,7 +82,10 @@ impl Client { } pub unsafe fn open(s: &str) -> io::Result { - Ok(Self::from_fifo(s)?.unwrap_or(Self::from_pipe(s)?)) + if let Some(client) = Self::from_fifo(s)? { + return Ok(client); + } + Self::from_pipe(s) } /// `--jobserver-auth=fifo:PATH` From 9ca894f716a7bd45658537a6c3034c5921c41c4d Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 16:03:38 +0300 Subject: [PATCH 10/13] [temp] maybe better diagnostic --- src/unix.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 702a047..a7b5e54 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -85,7 +85,13 @@ impl Client { if let Some(client) = Self::from_fifo(s)? { return Ok(client); } - Self::from_pipe(s) + if let Some(client) = Self::from_pipe(s)? { + return Ok(client); + } + Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unrecognized format of environment variable", + )) } /// `--jobserver-auth=fifo:PATH` @@ -106,13 +112,13 @@ impl Client { } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> io::Result { + unsafe fn from_pipe(s: &str) -> io::Result> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); - let write = parts.next().ok_or(io::Error::new( - io::ErrorKind::InvalidInput, - "expected ',' in `auth=R,W`", - ))?; + let write = match parts.next() { + Some(w) => w, + None => return Ok(None), + }; let read = read .parse() .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; @@ -132,7 +138,7 @@ impl Client { check_fd(write)?; drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); - Ok(Client::from_fds(read, write)) + Ok(Some(Client::from_fds(read, write))) } unsafe fn from_fds(read: c_int, write: c_int) -> Client { From ff334c2531978f7801e2d2057a121e7545db3669 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Tue, 14 Mar 2023 18:04:35 +0300 Subject: [PATCH 11/13] [temp] changed ok_or to ok_or_else --- src/unix.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index a7b5e54..0a50b69 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -100,10 +100,9 @@ impl Client { if parts.next().unwrap() != "fifo" { return Ok(None); } - let path = Path::new(parts.next().ok_or(io::Error::new( - io::ErrorKind::InvalidInput, - "expected ':' after `fifo`", - ))?); + let path = Path::new(parts.next().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "expected ':' after `fifo`") + })?); let file = OpenOptions::new().read(true).write(true).open(path)?; Ok(Some(Client::Fifo { file, From 65bf01a944027bd461659049f1bd194e6b05e19b Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Thu, 23 Mar 2023 12:01:36 +0300 Subject: [PATCH 12/13] [temp] use new name 'from_env_ext' for new behavior --- src/lib.rs | 14 +++++++++++--- tests/client.rs | 6 +++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2180a4f..680c6b6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,8 +29,8 @@ //! //! // See API documentation for why this is `unsafe` //! let client = match unsafe { Client::from_env() } { -//! Ok(client) => client, -//! Err(_) => panic!("client not configured"), +//! Some(client) => client, +//! None => panic!("client not configured"), //! }; //! ``` //! @@ -269,7 +269,7 @@ impl Client { /// /// Note, though, that on Windows it should be safe to call this function /// any number of times. - pub unsafe fn from_env() -> Result { + pub unsafe fn from_env_ext() -> Result { let (env, var) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] .iter() .map(|&env| env::var(env).map(|var| (env, var))) @@ -289,6 +289,14 @@ impl Client { } } + /// Attempts to connect to the jobserver specified in this process's + /// environment. + /// + /// Wraps `from_env_ext` and discards error details. + pub unsafe fn from_env() -> Option { + Self::from_env_ext().ok() + } + /// Acquires a token from this jobserver client. /// /// This function will block the calling thread until a new token can be diff --git a/tests/client.rs b/tests/client.rs index bad343e..2516b8c 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -35,7 +35,7 @@ const TESTS: &[Test] = &[ make_args: &[], rule: &|me| me.to_string(), f: &|| { - assert!(unsafe { Client::from_env().is_err() }); + assert!(unsafe { Client::from_env().is_none() }); }, }, Test { @@ -43,7 +43,7 @@ const TESTS: &[Test] = &[ make_args: &[], rule: &|me| format!("+{}", me), f: &|| { - assert!(unsafe { Client::from_env().is_err() }); + assert!(unsafe { Client::from_env().is_none() }); }, }, Test { @@ -51,7 +51,7 @@ const TESTS: &[Test] = &[ make_args: &["-j2"], rule: &|me| format!("+{}", me), f: &|| { - assert!(unsafe { Client::from_env().is_ok() }); + assert!(unsafe { Client::from_env().is_some() }); }, }, Test { From c78d26de25c87eae278a0e15566abec58727a538 Mon Sep 17 00:00:00 2001 From: BelovDV <70999565+BelovDV@users.noreply.github.com> Date: Thu, 23 Mar 2023 12:15:52 +0300 Subject: [PATCH 13/13] [temp] switch 'check_pipe' to runtime option --- Cargo.toml | 3 --- src/lib.rs | 14 +++++++++----- src/unix.rs | 19 ++++++++----------- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f41f407..807edda 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,9 +14,6 @@ edition = "2018" [target.'cfg(unix)'.dependencies] libc = "0.2.50" -[features] -do_not_check_pipe = [] - [dev-dependencies] futures = "0.1" num_cpus = "1.0" diff --git a/src/lib.rs b/src/lib.rs index 680c6b6..5b680ee 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -250,8 +250,8 @@ impl Client { /// with `CLOEXEC` so they're not automatically inherited by spawned /// children. /// - /// There is a feature to configure, will this function check if provided - /// files are actually pipes. + /// On unix if `unix_check_is_pipe` enabled this function will check if + /// provided files are actually pipes. /// /// # Safety /// @@ -269,7 +269,7 @@ impl Client { /// /// Note, though, that on Windows it should be safe to call this function /// any number of times. - pub unsafe fn from_env_ext() -> Result { + pub unsafe fn from_env_ext(check_pipe: bool) -> Result { let (env, var) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] .iter() .map(|&env| env::var(env).map(|var| (env, var))) @@ -283,7 +283,11 @@ impl Client { .ok_or(ErrFromEnv::IsNotConfigured)?; let s = var[pos + arg.len()..].split(' ').next().unwrap(); - match imp::Client::open(s) { + #[cfg(unix)] + let imp_client = imp::Client::open(s, check_pipe); + #[cfg(not(unix))] + let imp_client = imp::Client::open(s); + match imp_client { Ok(c) => Ok(Client { inner: Arc::new(c) }), Err(err) => Err(ErrFromEnv::PlatformSpecific { err, env, var }), } @@ -294,7 +298,7 @@ impl Client { /// /// Wraps `from_env_ext` and discards error details. pub unsafe fn from_env() -> Option { - Self::from_env_ext().ok() + Self::from_env_ext(false).ok() } /// Acquires a token from this jobserver client. diff --git a/src/unix.rs b/src/unix.rs index 0a50b69..8991825 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -81,11 +81,11 @@ impl Client { Ok(Client::from_fds(pipes[0], pipes[1])) } - pub unsafe fn open(s: &str) -> io::Result { + pub unsafe fn open(s: &str, check_pipe: bool) -> io::Result { if let Some(client) = Self::from_fifo(s)? { return Ok(client); } - if let Some(client) = Self::from_pipe(s)? { + if let Some(client) = Self::from_pipe(s, check_pipe)? { return Ok(client); } Err(io::Error::new( @@ -111,7 +111,7 @@ impl Client { } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str) -> io::Result> { + unsafe fn from_pipe(s: &str, check_pipe: bool) -> io::Result> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); let write = match parts.next() { @@ -133,8 +133,8 @@ impl Client { // If we're called from `make` *without* the leading + on our rule // then we'll have `MAKEFLAGS` env vars but won't actually have // access to the file descriptors. - check_fd(read)?; - check_fd(write)?; + check_fd(read, check_pipe)?; + check_fd(write, check_pipe)?; drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); Ok(Some(Client::from_fds(read, write))) @@ -386,9 +386,8 @@ impl Helper { } } -fn check_fd(fd: c_int) -> io::Result<()> { - #[cfg(not(feature = "do_not_check_pipe"))] - unsafe { +unsafe fn check_fd(fd: c_int, check_pipe: bool) -> io::Result<()> { + if check_pipe { let mut stat = mem::zeroed(); if libc::fstat(fd, &mut stat) == -1 { Err(io::Error::last_os_error()) @@ -404,9 +403,7 @@ fn check_fd(fd: c_int) -> io::Result<()> { } Err(io::Error::last_os_error()) // } - } - #[cfg(feature = "do_not_check_pipe")] - unsafe { + } else { match libc::fcntl(fd, libc::F_GETFD) { r if r == -1 => Err(io::Error::new( io::ErrorKind::InvalidData,