Merge pull request #54 from belovdv/err-extend-type

Error type for `from_env_ext` function

Author: petrochenkov

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<std::io::Error>),
+    Unsupported,
+}

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<Client, FromEnvError>,
+    /// 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<Client, ErrFromEnv> {
-        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<Client> {
-        Self::from_env_ext(false).ok()
+        Self::from_env_ext(false).client.ok()
     }
 
     /// Acquires a token from this jobserver client.

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,37 +82,41 @@ impl Client {
         Ok(Client::from_fds(pipes[0], pipes[1]))
     }
 
-    pub unsafe fn open(s: &str, check_pipe: bool) -> io::Result<Client> {
+    pub(crate) unsafe fn open(s: &str, check_pipe: bool) -> Result<Client, FromEnvErrorInner> {
         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<Option<Client>> {
+    fn from_fifo(s: &str) -> Result<Option<Client>, 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(),
         }))
     }
 
     /// `--jobserver-auth=R,W`
-    unsafe fn from_pipe(s: &str, check_pipe: bool) -> io::Result<Option<Client>> {
+    unsafe fn from_pipe(s: &str, check_pipe: bool) -> Result<Option<Client>, 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)
     }
 }