Overhaul Nix's error types

For many of Nix's consumers it be convenient to easily convert a Nix
error into a std::io::Error.  That's currently not possible because of
the InvalidPath, InvalidUtf8, and UnsupportedOperation types that have
no equivalent in std::io::Error.

However, very few of Nix's public APIs actually return those unusual
errors.  So a more useful API would be for Nix's standard error type to
implement Into<std::io::Error>.

This commit makes Error a simple NewType around Errno.  For most
functions it's a drop-in replacement.  There are only three exceptions:

* clearenv now returns a bespoke error type.  It was the only Nix
  function whose error couldn't be cleanly mapped onto an Errno.

* sys::signal::signal now returns Error(Errno::ENOTSUP) instead of
  Error::UnsupportedOperation when the user passes an incompatible
  argument to `handler`.

* When a NixPath exceeds PATH_MAX, it will now return
  Error(Errno::ENAMETOOLONG) instead of Error::InvalidPath.

In the latter two cases there is now some abiguity about whether the
error code was generated by Nix or by the OS.  But I think the ambiguity
is worth it for the sake of being able to implement Into<io::Error>.

This commit also introduces Error::Sys() as a migration aid.  Previously
that as an enum variant.  Now it's a function, but it will work in many
of the same contexts as the original.

Fixes #1155

Author: asomers

src/dir.rs

@@ -84,7 +84,7 @@ impl AsRawFd for Dir {
 impl Drop for Dir {
     fn drop(&mut self) {
         let e = Errno::result(unsafe { libc::closedir(self.0.as_ptr()) });
-        if !std::thread::panicking() && e == Err(Error::Sys(Errno::EBADF)) {
+        if !std::thread::panicking() && e == Err(Error::from(Errno::EBADF)) {
             panic!("Closing an invalid file descriptor!");
         };
     }

src/env.rs

@@ -1,12 +1,24 @@
 use cfg_if::cfg_if;
-use crate::{Error, Result};
+use std::fmt;
+
+/// Indicates that [`clearenv`] failed for some unknown reason
+#[derive(Clone, Copy, Debug)]
+pub struct ClearEnvError;
+
+impl fmt::Display for ClearEnvError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "clearenv failed")
+    }
+}
+
+impl std::error::Error for ClearEnvError {}
 
 /// Clear the environment of all name-value pairs.
 ///
 /// On platforms where libc provides `clearenv()`, it will be used. libc's
 /// `clearenv()` is documented to return an error code but not set errno; if the
 /// return value indicates a failure, this function will return
-/// `Error::UnsupportedOperation`.
+/// [`ClearEnvError`].
 ///
 /// On platforms where libc does not provide `clearenv()`, a fallback
 /// implementation will be used that iterates over all environment variables and
@@ -25,7 +37,7 @@ use crate::{Error, Result};
 ///  `environ` is currently held. The latter is not an issue if the only other
 ///  environment access in the program is via `std::env`, but the requirement on
 ///  thread safety must still be upheld.
-pub unsafe fn clearenv() -> Result<()> {
+pub unsafe fn clearenv() -> std::result::Result<(), ClearEnvError> {
     let ret;
     cfg_if! {
         if #[cfg(any(target_os = "fuchsia",
@@ -48,6 +60,6 @@ pub unsafe fn clearenv() -> Result<()> {
     if ret == 0 {
         Ok(())
     } else {
-        Err(Error::UnsupportedOperation)
+        Err(ClearEnvError)
     }
 }

src/errno.rs

@@ -64,11 +64,21 @@ impl Errno {
         clear()
     }
 
+    pub(crate) fn result2<S: ErrnoSentinel + PartialEq<S>>(value: S)
+        -> std::result::Result<S, Self>
+    {
+        if value == S::sentinel() {
+            Err(Self::last())
+        } else {
+            Ok(value)
+        }
+    }
+
     /// Returns `Ok(value)` if it does not contain the sentinel value. This
     /// should not be used when `-1` is not the errno sentinel value.
     pub fn result<S: ErrnoSentinel + PartialEq<S>>(value: S) -> Result<S> {
         if value == S::sentinel() {
-            Err(Error::Sys(Self::last()))
+            Err(Error::from(Self::last()))
         } else {
             Ok(value)
         }

src/fcntl.rs

@@ -288,7 +288,7 @@ fn inner_readlink<P: ?Sized + NixPath>(dirfd: Option<RawFd>, path: &P) -> Result
                 Some(next_size) => try_size = next_size,
                 // It's absurd that this would happen, but handle it sanely
                 // anyway.
-                None => break Err(super::Error::Sys(Errno::ENAMETOOLONG)),
+                None => break Err(super::Error::from(Errno::ENAMETOOLONG)),
             }
         }
     }
@@ -646,6 +646,6 @@ pub fn posix_fallocate(fd: RawFd, offset: libc::off_t, len: libc::off_t) -> Resu
     match Errno::result(res) {
         Err(err) => Err(err),
         Ok(0) => Ok(()),
-        Ok(errno) => Err(crate::Error::Sys(Errno::from_i32(errno))),
+        Ok(errno) => Err(crate::Error::from(Errno::from_i32(errno))),
     }
 }

src/lib.rs

@@ -79,34 +79,29 @@ pub mod unistd;
 
 use libc::{c_char, PATH_MAX};
 
-use std::{error, fmt, ptr, result};
+use std::convert::TryFrom;
+use std::{error, fmt, io, ptr, result};
 use std::ffi::{CStr, OsStr};
 use std::os::unix::ffi::OsStrExt;
 use std::path::{Path, PathBuf};
 
-use errno::Errno;
+use errno::{Errno, ErrnoSentinel};
 
 /// Nix Result Type
 pub type Result<T> = result::Result<T, Error>;
 
-/// Nix Error Type
+/// Nix's main error type.
 ///
-/// The nix error type provides a common way of dealing with
-/// various system system/libc calls that might fail.  Each
-/// error has a corresponding errno (usually the one from the
-/// underlying OS) to which it can be mapped in addition to
-/// implementing other common traits.
+/// It's a wrapper around Errno.  As such, it's very interoperable with
+/// [`std::io::Error`], but it has the advantages of:
+/// * `Clone`
+/// * `Copy`
+/// * `Eq`
+/// * Small size
+/// * Represents all of the system's errnos, instead of just the most common
+/// ones.
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub enum Error {
-    Sys(Errno),
-    InvalidPath,
-    /// The operation involved a conversion to Rust's native String type, which failed because the
-    /// string did not contain all valid UTF-8.
-    InvalidUtf8,
-    /// The operation is not supported by Nix, in this instance either use the libc bindings or
-    /// consult the module documentation to see if there is a more appropriate interface available.
-    UnsupportedOperation,
-}
+pub struct Error(pub Errno);
 
 impl Error {
     /// Convert this `Error` to an [`Errno`](enum.Errno.html).
@@ -119,49 +114,95 @@ impl Error {
     /// let e = Error::from(Errno::EPERM);
     /// assert_eq!(Some(Errno::EPERM), e.as_errno());
     /// ```
+    #[deprecated(
+        since = "0.22.0",
+        note = "Use Error::into<Errno> instead"
+    )]
     pub fn as_errno(self) -> Option<Errno> {
-        if let Error::Sys(e) = self {
-            Some(e)
-        } else {
-            None
-        }
+        Some(self.0)
     }
 
     /// Create a nix Error from a given errno
+    #[deprecated(
+        since = "0.22.0",
+        note = "Use Error::from instead"
+    )]
     pub fn from_errno(errno: Errno) -> Error {
-        Error::Sys(errno)
+        Error::from(errno)
     }
 
     /// Get the current errno and convert it to a nix Error
     pub fn last() -> Error {
-        Error::Sys(Errno::last())
+        Error::from(Errno::last())
     }
 
     /// Create a new invalid argument error (`EINVAL`)
+    #[deprecated(
+        since = "0.22.0",
+        note = "Use Error::from(Errno::EINVAL) instead"
+    )]
     pub fn invalid_argument() -> Error {
-        Error::Sys(Errno::EINVAL)
+        Error::from(Errno::EINVAL)
+    }
+
+    /// Returns `Ok(value)` if it does not contain the sentinel value. This
+    /// should not be used when `-1` is not the errno sentinel value.
+    pub(crate) fn result<S: ErrnoSentinel + PartialEq<S>>(value: S)
+        -> std::result::Result<S, Error>
+    {
+        Errno::result2(value).map_err(Self::from)
     }
 
+    /// Backwards compatibility hack for Nix <= 0.21.0 users
+    ///
+    /// In older versions of Nix, `Error::Sys` was an enum variant.  Now it's a
+    /// function, which is compatible with most of the former use cases of the
+    /// enum variant.  But you should use `Error(Errno::...)` instead.
+    #[deprecated(
+        since = "0.22.0",
+        note = "Use Error(Errno::...) instead"
+    )]
+    #[allow(non_snake_case)]
+    #[inline]
+    pub fn Sys(errno: Errno) -> Error {
+        Error::from(errno)
+    }
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{:?}: {}", self.0, self.0.desc())
+    }
 }
 
+impl error::Error for Error {}
+
 impl From<Errno> for Error {
-    fn from(errno: Errno) -> Error { Error::from_errno(errno) }
+    fn from(errno: Errno) -> Self {
+        Self(errno)
+    }
 }
 
-impl From<std::string::FromUtf8Error> for Error {
-    fn from(_: std::string::FromUtf8Error) -> Error { Error::InvalidUtf8 }
+impl From<Error> for Errno {
+    fn from(error: Error) -> Self {
+        error.0
+    }
 }
 
-impl error::Error for Error {}
+impl TryFrom<io::Error> for Error {
+    type Error = io::Error;
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            Error::InvalidPath => write!(f, "Invalid path"),
-            Error::InvalidUtf8 => write!(f, "Invalid UTF-8 string"),
-            Error::UnsupportedOperation => write!(f, "Unsupported Operation"),
-            Error::Sys(errno) => write!(f, "{:?}: {}", errno, errno.desc()),
-        }
+    fn try_from(ioerror: io::Error) -> std::result::Result<Self, io::Error> {
+        ioerror.raw_os_error()
+            .map(Errno::from_i32)
+            .map(Error::from)
+            .ok_or(ioerror)
+    }
+}
+
+impl From<Error> for io::Error {
+    fn from(error: Error) -> Self {
+        Self::from_raw_os_error(error.0 as i32)
     }
 }
 
@@ -217,7 +258,7 @@ impl NixPath for CStr {
             where F: FnOnce(&CStr) -> T {
         // Equivalence with the [u8] impl.
         if self.len() >= PATH_MAX as usize {
-            return Err(Error::InvalidPath);
+            return Err(Error::from(Errno::ENAMETOOLONG))
         }
 
         Ok(f(self))
@@ -238,11 +279,11 @@ impl NixPath for [u8] {
         let mut buf = [0u8; PATH_MAX as usize];
 
         if self.len() >= PATH_MAX as usize {
-            return Err(Error::InvalidPath);
+            return Err(Error::from(Errno::ENAMETOOLONG))
         }
 
         match self.iter().position(|b| *b == 0) {
-            Some(_) => Err(Error::InvalidPath),
+            Some(_) => Err(Error::from(Errno::EINVAL)),
             None => {
                 unsafe {
                     // TODO: Replace with bytes::copy_memory. rust-lang/rust#24028

src/mount/bsd.rs

@@ -136,7 +136,7 @@ impl fmt::Display for NmountError {
 
 impl From<NmountError> for io::Error {
     fn from(err: NmountError) -> Self {
-        io::Error::from_raw_os_error(err.errno as i32)
+        err.errno.into()
     }
 }
 
@@ -381,7 +381,7 @@ impl<'a> Nmount<'a> {
                         Some(CStr::from_bytes_with_nul(sl).unwrap())
                     }
                 };
-                Err(NmountError::new(error.as_errno().unwrap(), errmsg))
+                Err(NmountError::new(error.into(), errmsg))
             }
         }
     }

src/pty.rs

@@ -70,21 +70,21 @@ impl Drop for PtyMaster {
         // condition, which can cause confusing errors for future I/O
         // operations.
         let e = unistd::close(self.0);
-        if e == Err(Error::Sys(Errno::EBADF)) {
+        if e == Err(Error(Errno::EBADF)) {
             panic!("Closing an invalid file descriptor!");
         };
     }
 }
 
 impl io::Read for PtyMaster {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        unistd::read(self.0, buf).map_err(|e| e.as_errno().unwrap().into())
+        unistd::read(self.0, buf).map_err(io::Error::from)
     }
 }
 
 impl io::Write for PtyMaster {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        unistd::write(self.0, buf).map_err(|e| e.as_errno().unwrap().into())
+        unistd::write(self.0, buf).map_err(io::Error::from)
     }
     fn flush(&mut self) -> io::Result<()> {
         Ok(())
@@ -99,7 +99,7 @@ impl io::Write for PtyMaster {
 #[inline]
 pub fn grantpt(fd: &PtyMaster) -> Result<()> {
     if unsafe { libc::grantpt(fd.as_raw_fd()) } < 0 {
-        return Err(Error::last());
+        return Err(Error::from(Errno::last()));
     }
 
     Ok(())
@@ -145,7 +145,7 @@ pub fn posix_openpt(flags: fcntl::OFlag) -> Result<PtyMaster> {
     };
 
     if fd < 0 {
-        return Err(Error::last());
+        return Err(Error::from(Errno::last()));
     }
 
     Ok(PtyMaster(fd))
@@ -171,7 +171,7 @@ pub fn posix_openpt(flags: fcntl::OFlag) -> Result<PtyMaster> {
 pub unsafe fn ptsname(fd: &PtyMaster) -> Result<String> {
     let name_ptr = libc::ptsname(fd.as_raw_fd());
     if name_ptr.is_null() {
-        return Err(Error::last());
+        return Err(Error::from(Errno::last()));
     }
 
     let name = CStr::from_ptr(name_ptr);
@@ -213,7 +213,7 @@ pub fn ptsname_r(fd: &PtyMaster) -> Result<String> {
 #[inline]
 pub fn unlockpt(fd: &PtyMaster) -> Result<()> {
     if unsafe { libc::unlockpt(fd.as_raw_fd()) } < 0 {
-        return Err(Error::last());
+        return Err(Error::from(Errno::last()));
     }
 
     Ok(())

src/sched.rs

@@ -68,7 +68,7 @@ mod sched_linux_like {
         /// `field` is the CPU id to test
         pub fn is_set(&self, field: usize) -> Result<bool> {
             if field >= CpuSet::count() {
-                Err(Error::Sys(Errno::EINVAL))
+                Err(Error::from(Errno::EINVAL))
             } else {
                 Ok(unsafe { libc::CPU_ISSET(field, &self.cpu_set) })
             }
@@ -78,7 +78,7 @@ mod sched_linux_like {
         /// `field` is the CPU id to add
         pub fn set(&mut self, field: usize) -> Result<()> {
             if field >= CpuSet::count() {
-                Err(Error::Sys(Errno::EINVAL))
+                Err(Error::from(Errno::EINVAL))
             } else {
                 unsafe { libc::CPU_SET(field, &mut self.cpu_set); }
                 Ok(())
@@ -89,7 +89,7 @@ mod sched_linux_like {
         /// `field` is the CPU id to remove
         pub fn unset(&mut self, field: usize) -> Result<()> {
             if field >= CpuSet::count() {
-                Err(Error::Sys(Errno::EINVAL))
+                Err(Error::from(Errno::EINVAL))
             } else {
                 unsafe { libc::CPU_CLR(field, &mut self.cpu_set);}
                 Ok(())