[diagnostic] extend from_env function

belovdv · belovdv · commit 23e8b7ee60fe · 2023-03-12T14:30:06.000+03:00
diff --git 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
@@ -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<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.
diff --git 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<Client> {
         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> {
-        Client::from_fifo(s).or_else(|| Client::from_pipe(s))
+    pub unsafe fn open(s: &str) -> Result<Client, ErrFromEnv> {
+        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<Client> {
+    fn from_fifo(s: &str) -> Option<Result<Client, ErrFromEnv>> {
         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<Client> {
+    unsafe fn from_pipe(s: &str) -> Option<Result<Client, ErrFromEnv>> {
         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
@@ -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<Client> {
         Ok(Client {
@@ -27,8 +32,8 @@ impl Client {
         })
     }
 
-    pub unsafe fn open(_s: &str) -> Option<Client> {
-        None
+    pub unsafe fn open(_s: &str) -> Result<Client, ErrFromEnv> {
+        Err(ErrFromEnv::UnavailableOnTarget)
     }
 
     pub fn acquire(&self) -> io::Result<Acquired> {
diff --git 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<Client> {
+    pub unsafe fn open(s: &str) -> Result<Client, ErrFromEnv> {
         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
@@ -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 {
