Diagnostic: extend from_env function

Cargo.toml

@@ -14,6 +14,9 @@ edition = "2018"
 [target.'cfg(unix)'.dependencies]
 libc = "0.2.50"
 
+[features]
+do_not_check_pipe = []
+
 [dev-dependencies]
 futures = "0.1"
 num_cpus = "1.0"

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,44 @@ 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: io::Error,
+        /// Name of gotten env var.
+        env: &'static str,
+        /// Value of gotten env var.
+        var: String,
+    },
+}
+
+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.
     ///
@@ -197,8 +235,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 +250,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 +269,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<Client> {
-        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<Client, ErrFromEnv> {
+        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.

src/unix.rs

@@ -81,62 +81,64 @@ impl Client {
         Ok(Client::from_fds(pipes[0], pipes[1]))
     }
 
-    pub unsafe fn open(s: &str) -> Option<Client> {
-        Client::from_fifo(s).or_else(|| Client::from_pipe(s))
+    pub unsafe fn open(s: &str) -> io::Result<Client> {
+        if let Some(client) = Self::from_fifo(s)? {
+            return Ok(client);
+        }
+        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`
-    fn from_fifo(s: &str) -> Option<Client> {
+    fn from_fifo(s: &str) -> io::Result<Option<Client>> {
         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 None,
-        };
-        let file = match OpenOptions::new().read(true).write(true).open(path) {
-            Ok(f) => f,
-            Err(_) => return None,
-        };
-        Some(Client::Fifo {
+        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(),
-        })
+        }))
     }
 
     /// `--jobserver-auth=R,W`
-    unsafe fn from_pipe(s: &str) -> Option<Client> {
+    unsafe fn from_pipe(s: &str) -> io::Result<Option<Client>> {
         let mut parts = s.splitn(2, ',');
         let read = parts.next().unwrap();
         let write = match parts.next() {
-            Some(s) => s,
-            None => return None,
-        };
-
-        let read = match read.parse() {
-            Ok(n) => n,
-            Err(_) => return None,
-        };
-        let write = match write.parse() {
-            Ok(n) => n,
-            Err(_) => return None,
+            Some(w) => w,
+            None => return Ok(None),
         };
+        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
-        // 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) {
-            drop(set_cloexec(read, true));
-            drop(set_cloexec(write, true));
-            Some(Client::from_fds(read, write))
-        } else {
-            None
-        }
+        check_fd(read)?;
+        check_fd(write)?;
+        drop(set_cloexec(read, true));
+        drop(set_cloexec(write, true));
+        Ok(Some(Client::from_fds(read, write)))
     }
 
     unsafe fn from_fds(read: c_int, write: c_int) -> Client {
@@ -207,7 +209,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 +328,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 +387,35 @@ impl Helper {
     }
 }
 
-fn is_valid_fd(fd: c_int) -> bool {
-    unsafe { libc::fcntl(fd, libc::F_GETFD) != -1 }
+fn check_fd(fd: c_int) -> io::Result<()> {
+    #[cfg(not(feature = "do_not_check_pipe"))]
+    unsafe {
+        let mut stat = mem::zeroed();
+        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 _;
+            if stat.st_mode & s_ififo == s_ififo {
+                return Ok(());
+            }
+            Err(io::Error::last_os_error()) //
+        }
+    }
+    #[cfg(feature = "do_not_check_pipe")]
+    unsafe {
+        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(()),
+        }
+    }
 }
 
 fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> {

src/wasm.rs

@@ -27,8 +27,8 @@ impl Client {
         })
     }
 
-    pub unsafe fn open(_s: &str) -> Option<Client> {
-        None
+    pub unsafe fn open(_s: &str) -> io::Result<Client> {
+        Err(io::ErrorKind::Unsupported.into())
     }
 
     pub fn acquire(&self) -> io::Result<Acquired> {

src/windows.rs

@@ -127,17 +127,14 @@ impl Client {
         ))
     }
 
-    pub unsafe fn open(s: &str) -> Option<Client> {
-        let name = match CString::new(s) {
-            Ok(s) => s,
-            Err(_) => return None,
-        };
+    pub unsafe fn open(s: &str) -> io::Result<Client> {
+        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() {
-            None
+            Err(io::Error::last_os_error())
         } else {
-            Some(Client {
+            Ok(Client {
                 sem: Handle(sem),
                 name: s.to_string(),
             })

tests/client.rs

@@ -35,23 +35,23 @@ 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 {
         name: "no j args with plus",
         make_args: &[],
         rule: &|me| format!("+{}", me),
         f: &|| {
-            assert!(unsafe { Client::from_env().is_none() });
+            assert!(unsafe { Client::from_env().is_err() });
         },
     },
     Test {
         name: "j args with plus",
         make_args: &["-j2"],
         rule: &|me| format!("+{}", me),
         f: &|| {
-            assert!(unsafe { Client::from_env().is_some() });
+            assert!(unsafe { Client::from_env().is_ok() });
         },
     },
     Test {