From 3c0739ea44074da9e4ab19427680721acb2884b2 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Wed, 29 Mar 2023 15:47:43 +0400 Subject: [PATCH] error type for `from_env_ext` function --- src/error.rs | 78 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 105 ++++++++++++++++++++++++++----------------------- src/unix.rs | 75 ++++++++++++++++++++++------------- src/wasm.rs | 5 ++- src/windows.rs | 13 ++++-- 5 files changed, 195 insertions(+), 81 deletions(-) create mode 100644 src/error.rs diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 0000000..906cd8b --- /dev/null +++ b/src/error.rs @@ -0,0 +1,78 @@ +#[cfg(unix)] +type RawFd = std::os::fd::RawFd; +#[cfg(not(unix))] +type RawFd = std::convert::Infallible; + +/// Error type for `from_env_ext` function. +#[derive(Debug)] +pub struct FromEnvError { + pub(crate) inner: FromEnvErrorInner, +} + +/// Kind of an error returned from `from_env_ext` function. +#[derive(Debug)] +#[non_exhaustive] +pub enum FromEnvErrorKind { + /// There is no environment variable that describes jobserver to inherit. + NoEnvVar, + /// Cannot parse jobserver environment variable value, incorrect format. + CannotParse, + /// Cannot open path or name from the jobserver environment variable value. + CannotOpenPath, + /// Cannot open file descriptor from the jobserver environment variable value. + CannotOpenFd, + /// File descriptor from the jobserver environment variable value is not a pipe. + NotAPipe, + /// Jobserver inheritance is not supported on this platform. + Unsupported, +} + +impl FromEnvError { + /// Get the error kind. + pub fn kind(&self) -> FromEnvErrorKind { + match self.inner { + FromEnvErrorInner::NoEnvVar => FromEnvErrorKind::NoEnvVar, + FromEnvErrorInner::CannotParse(_) => FromEnvErrorKind::CannotParse, + FromEnvErrorInner::CannotOpenPath(..) => FromEnvErrorKind::CannotOpenPath, + FromEnvErrorInner::CannotOpenFd(..) => FromEnvErrorKind::CannotOpenFd, + FromEnvErrorInner::NotAPipe(..) => FromEnvErrorKind::NotAPipe, + FromEnvErrorInner::Unsupported => FromEnvErrorKind::Unsupported, + } + } +} + +impl std::fmt::Display for FromEnvError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.inner { + FromEnvErrorInner::NoEnvVar => write!(f, "there is no environment variable that describes jobserver to inherit"), + FromEnvErrorInner::CannotParse(s) => write!(f, "cannot parse jobserver environment variable value: {s}"), + FromEnvErrorInner::CannotOpenPath(s, err) => write!(f, "cannot open path or name {s} from the jobserver environment variable value: {err}"), + FromEnvErrorInner::CannotOpenFd(fd, err) => write!(f, "cannot open file descriptor {fd} from the jobserver environment variable value: {err}"), + 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"), + } + } +} +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) + } + _ => None, + } + } +} + +#[allow(dead_code)] +#[derive(Debug)] +pub(crate) enum FromEnvErrorInner { + NoEnvVar, + CannotParse(String), + CannotOpenPath(String, std::io::Error), + CannotOpenFd(RawFd, std::io::Error), + NotAPipe(RawFd, Option), + Unsupported, +} diff --git a/src/lib.rs b/src/lib.rs index 5b680ee..9f2a469 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,10 +82,12 @@ #![doc(html_root_url = "https://docs.rs/jobserver/0.1")] use std::env; +use std::ffi::OsString; use std::io; use std::process::Command; use std::sync::{Arc, Condvar, Mutex, MutexGuard}; +mod error; #[cfg(unix)] #[path = "unix.rs"] mod imp; @@ -151,40 +153,30 @@ struct HelperInner { consumer_done: bool, } -/// Error type for `from_env` function. +use error::FromEnvErrorInner; +pub use error::{FromEnvError, FromEnvErrorKind}; + +/// Return type for `from_env_ext` 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: io::Error, - /// Name of gotten env var. - env: &'static str, - /// Value of gotten env var. - var: String, - }, +pub struct FromEnv { + /// Result of trying to get jobserver client from env. + pub client: Result, + /// Name and value of the environment variable. + /// `None` if no relevant environment variable is found. + pub var: Option<(&'static str, OsString)>, } -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 FromEnv { + fn new_ok(client: Client, var_name: &'static str, var_value: OsString) -> FromEnv { + FromEnv { + client: Ok(client), + var: Some((var_name, var_value)), } } -} - -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), + fn new_err(kind: FromEnvErrorInner, var_name: &'static str, var_value: OsString) -> FromEnv { + FromEnv { + client: Err(FromEnvError { inner: kind }), + var: Some((var_name, var_value)), } } } @@ -234,10 +226,10 @@ impl Client { /// /// # Return value /// + /// `FromEnv` contains result and relevant environment variable. /// If a jobserver was found in the environment and it looks correct then - /// `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. + /// result with the connected client will be returned. In other cases + /// result will contain `Err(FromEnvErr)`. /// /// Note that on Unix the `Client` returned **takes ownership of the file /// descriptors specified in the environment**. Jobservers on Unix are @@ -250,8 +242,8 @@ impl Client { /// with `CLOEXEC` so they're not automatically inherited by spawned /// children. /// - /// On unix if `unix_check_is_pipe` enabled this function will check if - /// provided files are actually pipes. + /// On unix if `check_pipe` enabled this function will check if provided + /// files are actually pipes. /// /// # Safety /// @@ -269,27 +261,42 @@ 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(check_pipe: bool) -> Result { - let (env, var) = ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] + pub unsafe fn from_env_ext(check_pipe: bool) -> FromEnv { + let (env, var_os) = match ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"] .iter() - .map(|&env| env::var(env).map(|var| (env, var))) - .find_map(|p| p.ok()) - .ok_or(ErrFromEnv::IsNotConfigured)?; + .map(|&env| env::var_os(env).map(|var| (env, var))) + .find_map(|p| p) + { + Some((env, var_os)) => (env, var_os), + None => return FromEnv::new_err(FromEnvErrorInner::NoEnvVar, "", Default::default()), + }; + + let var = match var_os.to_str() { + Some(var) => var, + None => { + let err = FromEnvErrorInner::CannotParse("not valid UTF-8".to_string()); + return FromEnv::new_err(err, env, var_os); + } + }; - let (arg, pos) = ["--jobserver-fds=", "--jobserver-auth="] + let (arg, pos) = match ["--jobserver-fds=", "--jobserver-auth="] .iter() .map(|&arg| var.find(arg).map(|pos| (arg, pos))) .find_map(|pos| pos) - .ok_or(ErrFromEnv::IsNotConfigured)?; + { + Some((arg, pos)) => (arg, pos), + None => { + let err = FromEnvErrorInner::CannotParse( + "expected `--jobserver-fds=` or `--jobserver-auth=`".to_string(), + ); + return FromEnv::new_err(err, env, var_os); + } + }; let s = var[pos + arg.len()..].split(' ').next().unwrap(); - #[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 }), + match imp::Client::open(s, check_pipe) { + Ok(c) => FromEnv::new_ok(Client { inner: Arc::new(c) }, env, var_os), + Err(err) => FromEnv::new_err(err, env, var_os), } } @@ -298,7 +305,7 @@ impl Client { /// /// Wraps `from_env_ext` and discards error details. pub unsafe fn from_env() -> Option { - Self::from_env_ext(false).ok() + Self::from_env_ext(false).client.ok() } /// Acquires a token from this jobserver client. diff --git a/src/unix.rs b/src/unix.rs index 8991825..ee1e923 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -1,5 +1,6 @@ use libc::c_int; +use crate::FromEnvErrorInner; use std::fs::{File, OpenOptions}; use std::io::{self, Read, Write}; use std::mem; @@ -81,29 +82,33 @@ impl Client { Ok(Client::from_fds(pipes[0], pipes[1])) } - pub unsafe fn open(s: &str, check_pipe: bool) -> io::Result { + pub(crate) unsafe fn open(s: &str, check_pipe: bool) -> Result { if let Some(client) = Self::from_fifo(s)? { return Ok(client); } if let Some(client) = Self::from_pipe(s, check_pipe)? { return Ok(client); } - Err(io::Error::new( - io::ErrorKind::InvalidInput, - "unrecognized format of environment variable", - )) + Err(FromEnvErrorInner::CannotParse(format!( + "expected `fifo:PATH` or `R,W`, found `{s}`" + ))) } /// `--jobserver-auth=fifo:PATH` - fn from_fifo(s: &str) -> io::Result> { + fn from_fifo(s: &str) -> Result, FromEnvErrorInner> { let mut parts = s.splitn(2, ':'); if parts.next().unwrap() != "fifo" { return Ok(None); } - 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)?; + let path_str = parts.next().ok_or_else(|| { + 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(), @@ -111,7 +116,7 @@ impl Client { } /// `--jobserver-auth=R,W` - unsafe fn from_pipe(s: &str, check_pipe: bool) -> io::Result> { + unsafe fn from_pipe(s: &str, check_pipe: bool) -> Result, FromEnvErrorInner> { let mut parts = s.splitn(2, ','); let read = parts.next().unwrap(); let write = match parts.next() { @@ -120,21 +125,30 @@ impl Client { }; let read = read .parse() - .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; + .map_err(|e| FromEnvErrorInner::CannotParse(format!("cannot parse `read` fd: {e}")))?; let write = write .parse() - .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; + .map_err(|e| FromEnvErrorInner::CannotParse(format!("cannot parse `write` fd: {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 - // instances of a pipe if feature enabled or valid files otherwise - // before we return the client. + // valid files and instances of a pipe if feature enabled 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. - check_fd(read, check_pipe)?; - check_fd(write, check_pipe)?; + // + // `NotAPipe` is a worse error, return it if it's reported for any of the two fds. + match (fd_check(read, check_pipe), fd_check(write, check_pipe)) { + (read_err @ Err(FromEnvErrorInner::NotAPipe(..)), _) => read_err?, + (_, write_err @ Err(FromEnvErrorInner::NotAPipe(..))) => write_err?, + (read_err, write_err) => { + read_err?; + write_err?; + } + } + drop(set_cloexec(read, true)); drop(set_cloexec(write, true)); Ok(Some(Client::from_fds(read, write))) @@ -386,31 +400,38 @@ impl Helper { } } -unsafe fn check_fd(fd: c_int, check_pipe: bool) -> io::Result<()> { +unsafe fn fcntl_check(fd: c_int) -> Result<(), FromEnvErrorInner> { + match libc::fcntl(fd, libc::F_GETFD) { + -1 => Err(FromEnvErrorInner::CannotOpenFd( + fd, + io::Error::last_os_error(), + )), + _ => Ok(()), + } +} + +unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner> { if check_pipe { let mut stat = mem::zeroed(); if libc::fstat(fd, &mut stat) == -1 { - Err(io::Error::last_os_error()) + let last_os_error = io::Error::last_os_error(); + fcntl_check(fd)?; + Err(FromEnvErrorInner::NotAPipe(fd, Some(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. + #[allow(unused_assignments)] let mut s_ififo = stat.st_mode; s_ififo = libc::S_IFIFO as _; if stat.st_mode & s_ififo == s_ififo { return Ok(()); } - Err(io::Error::last_os_error()) // + Err(FromEnvErrorInner::NotAPipe(fd, None)) } } else { - 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(()), - } + fcntl_check(fd) } } diff --git a/src/wasm.rs b/src/wasm.rs index a542fcf..fe92d72 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -1,3 +1,4 @@ +use crate::FromEnvErrorInner; use std::io; use std::process::Command; use std::sync::{Arc, Condvar, Mutex}; @@ -27,8 +28,8 @@ impl Client { }) } - pub unsafe fn open(_s: &str) -> io::Result { - Err(io::ErrorKind::Unsupported.into()) + pub(crate) unsafe fn open(_s: &str, _check_pipe: bool) -> Result { + Err(FromEnvErrorInner::Unsupported) } pub fn acquire(&self) -> io::Result { diff --git a/src/windows.rs b/src/windows.rs index bea8c05..6fae7b2 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -1,3 +1,4 @@ +use crate::FromEnvErrorInner; use std::ffi::CString; use std::io; use std::process::Command; @@ -127,12 +128,18 @@ impl Client { )) } - pub unsafe fn open(s: &str) -> io::Result { - let name = CString::new(s).map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; + pub(crate) unsafe fn open(s: &str, _check_pipe: bool) -> Result { + let name = match CString::new(s) { + Ok(s) => s, + Err(e) => return Err(FromEnvErrorInner::CannotParse(e.to_string())), + }; let sem = OpenSemaphoreA(SYNCHRONIZE | SEMAPHORE_MODIFY_STATE, FALSE, name.as_ptr()); if sem.is_null() { - Err(io::Error::last_os_error()) + Err(FromEnvErrorInner::CannotOpenPath( + s.to_string(), + io::Error::last_os_error(), + )) } else { Ok(Client { sem: Handle(sem),