Change error-chain to thiserror

Cargo.toml

@@ -16,13 +16,14 @@ travis-ci = { repository = "mullvad/pfctl-rs" }
 
 [dependencies]
 errno = "0.2"
-error-chain = "0.12"
 ioctl-sys = "0.6.0"
 libc = "0.2.29"
 derive_builder = "0.9"
 ipnetwork = "0.16"
+thiserror = "1"
 
 [dev-dependencies]
 assert_matches = "1.1.0"
+error-chain = "0.12"
 uuid = { version = "0.8", features = ["v4"] }
 scopeguard = "1.0"

examples/enable.rs

@@ -21,7 +21,7 @@ fn run() -> Result<()> {
     // Try to enable the firewall. Equivalent to the CLI command "pfctl -e".
     match pf.enable() {
         Ok(_) => println!("Enabled PF"),
-        Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _)) => (),
+        Err(pfctl::Error::StateAlreadyActive(_)) => (),
         err => err.chain_err(|| "Unable to enable PF")?,
     }
     Ok(())

src/lib.rs

@@ -59,15 +59,14 @@
 
 #![deny(rust_2018_idioms)]
 
-#[macro_use]
-pub extern crate error_chain;
-
 use std::{
     ffi::CStr,
+    fmt::{Debug, Display},
     fs::File,
     mem,
     os::unix::io::{AsRawFd, RawFd},
 };
+use thiserror::Error;
 
 pub use ipnetwork;
 
@@ -92,40 +91,117 @@ pub use crate::ruleset::*;
 mod transaction;
 pub use crate::transaction::*;
 
-mod errors {
-    error_chain! {
-        errors {
-            DeviceOpenError(s: &'static str) {
-                description("Unable to open PF device file")
-                display("Unable to open PF device file at '{}'", s)
-            }
-            InvalidArgument(s: &'static str) {
-                display("Invalid argument: {}", s)
-            }
-            StateAlreadyActive {
-                description("Target state is already active")
-            }
-            InvalidRuleCombination(s: String) {
-                description("Rule contains incompatible values")
-                display("Incompatible values in rule: {}", s)
-            }
-            AnchorDoesNotExist {
-                display("Anchor does not exist")
+#[derive(Error, Debug)]
+#[non_exhaustive]
+pub enum Error {
+    #[error("Unable to open PF device file at {}", _0)]
+    DeviceOpen(&'static str, #[source] ::std::io::Error),
+
+    #[error("Target state is already active")]
+    StateAlreadyActive(#[source] ::std::io::Error),
+
+    #[error("Incompatible values in rule: {}", _0)]
+    InvalidRuleCombination(String),
+
+    #[error("Anchor does not exist")]
+    AnchorDoesNotExist,
+
+    #[error("Anchor does not exist")]
+    Conversion(#[from] ConversionError),
+
+    #[error("Ioctl Error")]
+    Ioctl(#[from] ::std::io::Error),
+}
+
+#[derive(Error, Debug)]
+#[non_exhaustive]
+pub enum ConversionErrorInner {
+    #[error("Lower port is greater than upper port.")]
+    InvalidPortRange,
+
+    #[error("String does not fit destination")]
+    StrCopyNotFits,
+
+    #[error("String has null byte")]
+    StrCopyNullByte,
+
+    #[error("Cstr not null terminated")]
+    CstrNotTerminated,
+}
+
+#[derive(Debug)]
+#[non_exhaustive]
+pub enum ConversionErrorKind {
+    InvalidAnchorName,
+    IncompatibleInterfaceName,
+    InvalidRouteTarget,
+}
+
+impl Display for ConversionErrorKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ConversionErrorKind::InvalidAnchorName => write!(f, "Invalid anchor name"),
+            ConversionErrorKind::IncompatibleInterfaceName => {
+                write!(f, "Incompatible interface name")
             }
+            ConversionErrorKind::InvalidRouteTarget => write!(f, "Invalid route target"),
+        }
+    }
+}
+
+#[derive(Debug)]
+pub struct ConversionError {
+    pub kind: Option<ConversionErrorKind>,
+    pub source: ConversionErrorInner,
+}
+
+impl Display for ConversionError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if let Some(kind) = self.kind.as_ref() {
+            Display::fmt(kind, f)
+        } else {
+            Display::fmt(&self.source, f)
         }
-        foreign_links {
-            IoctlError(::std::io::Error);
+    }
+}
+
+impl std::error::Error for ConversionError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        self.kind.as_ref()?;
+        Some(&self.source)
+    }
+}
+
+impl ConversionError {
+    fn new(kind: ConversionErrorKind, err: ConversionErrorInner) -> Self {
+        Self {
+            kind: Some(kind),
+            source: err,
         }
     }
 }
-pub use crate::errors::*;
 
-/// Returns the given input result, except if it is an `Err` matching the given `ErrorKind`,
+impl From<ConversionErrorInner> for ConversionError {
+    fn from(source: ConversionErrorInner) -> Self {
+        Self { kind: None, source }
+    }
+}
+
+impl From<ConversionErrorInner> for Error {
+    fn from(source: ConversionErrorInner) -> Self {
+        Self::Conversion(ConversionError { kind: None, source })
+    }
+}
+
+pub type Result<T> = ::std::result::Result<T, Error>;
+pub type ConversionResult<T> = ::std::result::Result<T, ConversionErrorInner>;
+
+/// Returns the given input result, except if it is an `Err` matching the given `Error`,
 /// then it returns `Ok(())` instead, so the error is ignored.
 macro_rules! ignore_error_kind {
     ($result:expr, $kind:pat) => {
         match $result {
-            Err($crate::Error($kind, _)) => Ok(()),
+            Err($kind) => Ok(()),
             result => result,
         }
     };
@@ -141,14 +217,18 @@ mod conversion {
 
     /// Internal trait for all types that can try to write their value into another type.
     pub trait TryCopyTo<T: ?Sized> {
-        fn try_copy_to(&self, dst: &mut T) -> crate::Result<()>;
+        type Result;
+
+        fn try_copy_to(&self, dst: &mut T) -> Self::Result;
     }
 }
 use crate::conversion::*;
 
 /// Internal function to safely compare Rust string with raw C string slice
-fn compare_cstr_safe(s: &str, cchars: &[std::os::raw::c_char]) -> Result<bool> {
-    ensure!(cchars.iter().any(|&c| c == 0), "Not null terminated");
+fn compare_cstr_safe(s: &str, cchars: &[std::os::raw::c_char]) -> ConversionResult<bool> {
+    if !(cchars.iter().any(|&c| c == 0)) {
+        return Err(ConversionErrorInner::CstrNotTerminated);
+    }
     let cs = unsafe { CStr::from_ptr(cchars.as_ptr()) };
     Ok(s.as_bytes() == cs.to_bytes())
 }
@@ -174,7 +254,7 @@ impl PfCtl {
     /// Same as `enable`, but `StateAlreadyActive` errors are supressed and exchanged for
     /// `Ok(())`.
     pub fn try_enable(&mut self) -> Result<()> {
-        ignore_error_kind!(self.enable(), ErrorKind::StateAlreadyActive)
+        ignore_error_kind!(self.enable(), Error::StateAlreadyActive(_))
     }
 
     /// Tries to disable PF. If the firewall is already disabled it will return an
@@ -186,7 +266,7 @@ impl PfCtl {
     /// Same as `disable`, but `StateAlreadyActive` errors are supressed and exchanged for
     /// `Ok(())`.
     pub fn try_disable(&mut self) -> Result<()> {
-        ignore_error_kind!(self.disable(), ErrorKind::StateAlreadyActive)
+        ignore_error_kind!(self.disable(), Error::StateAlreadyActive(_))
     }
 
     /// Tries to determine if PF is enabled or not.
@@ -201,7 +281,7 @@ impl PfCtl {
 
         pfioc_rule.rule.action = kind.into();
         name.try_copy_to(&mut pfioc_rule.anchor_call[..])
-            .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?;
+            .map_err(|e| ConversionError::new(ConversionErrorKind::InvalidAnchorName, e))?;
 
         ioctl_guard!(ffi::pf_insert_rule(self.fd(), &mut pfioc_rule))?;
         Ok(())
@@ -210,7 +290,7 @@ impl PfCtl {
     /// Same as `add_anchor`, but `StateAlreadyActive` errors are supressed and exchanged for
     /// `Ok(())`.
     pub fn try_add_anchor(&mut self, name: &str, kind: AnchorKind) -> Result<()> {
-        ignore_error_kind!(self.add_anchor(name, kind), ErrorKind::StateAlreadyActive)
+        ignore_error_kind!(self.add_anchor(name, kind), Error::StateAlreadyActive(_))
     }
 
     pub fn remove_anchor(&mut self, name: &str, kind: AnchorKind) -> Result<()> {
@@ -222,10 +302,7 @@ impl PfCtl {
     /// Same as `remove_anchor`, but `AnchorDoesNotExist` errors are supressed and exchanged for
     /// `Ok(())`.
     pub fn try_remove_anchor(&mut self, name: &str, kind: AnchorKind) -> Result<()> {
-        ignore_error_kind!(
-            self.remove_anchor(name, kind),
-            ErrorKind::AnchorDoesNotExist
-        )
+        ignore_error_kind!(self.remove_anchor(name, kind), Error::AnchorDoesNotExist)
     }
 
     // TODO(linus): Make more generic. No hardcoded ADD_TAIL etc.
@@ -236,7 +313,7 @@ impl PfCtl {
         pfioc_rule.ticket = utils::get_ticket(self.fd(), &anchor, AnchorKind::Filter)?;
         anchor
             .try_copy_to(&mut pfioc_rule.anchor[..])
-            .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?;
+            .map_err(|e| ConversionError::new(ConversionErrorKind::InvalidAnchorName, e))?;
         rule.try_copy_to(&mut pfioc_rule.rule)?;
 
         pfioc_rule.action = ffi::pfvar::PF_CHANGE_ADD_TAIL as u32;
@@ -287,7 +364,7 @@ impl PfCtl {
 
     /// Clear states created by rules in anchor.
     /// Returns total number of removed states upon success, otherwise
-    /// ErrorKind::AnchorDoesNotExist if anchor does not exist.
+    /// Error::AnchorDoesNotExist if anchor does not exist.
     pub fn clear_states(&mut self, anchor_name: &str, kind: AnchorKind) -> Result<u32> {
         let pfsync_states = self.get_states()?;
         if !pfsync_states.is_empty() {
@@ -317,7 +394,7 @@ impl PfCtl {
         let mut pfioc_state_kill = unsafe { mem::zeroed::<ffi::pfvar::pfioc_state_kill>() };
         interface
             .try_copy_to(&mut pfioc_state_kill.psk_ifname)
-            .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?;
+            .map_err(|e| ConversionError::new(ConversionErrorKind::IncompatibleInterfaceName, e))?;
         ioctl_guard!(ffi::pf_clear_states(self.fd(), &mut pfioc_state_kill))?;
         // psk_af holds the number of killed states
         Ok(pfioc_state_kill.psk_af as u32)
@@ -342,7 +419,7 @@ impl PfCtl {
     /// The return value from closure is transparently passed to the caller.
     ///
     /// - Returns Result<R> from call to closure on match.
-    /// - Returns `ErrorKind::AnchorDoesNotExist` on mismatch, the closure is not called in that
+    /// - Returns `Error::AnchorDoesNotExist` on mismatch, the closure is not called in that
     /// case.
     fn with_anchor_rule<F, R>(&self, name: &str, kind: AnchorKind, f: F) -> Result<R>
     where
@@ -359,7 +436,7 @@ impl PfCtl {
                 return f(pfioc_rule);
             }
         }
-        bail!(ErrorKind::AnchorDoesNotExist);
+        Err(Error::AnchorDoesNotExist)
     }
 
     /// Returns global number of states created by all stateful rules (see keep_state)
@@ -419,7 +496,7 @@ mod tests {
         let cchars: &[i8] = unsafe { mem::transmute(cstr.as_bytes()) };
         assert_matches!(
             compare_cstr_safe("Hello", cchars),
-            Err(ref e) if e.description() == "Not null terminated"
+            Err(ConversionErrorInner::CstrNotTerminated)
         );
     }
 

src/macros.rs

@@ -16,12 +16,12 @@ macro_rules! ioctl_guard {
     ($func:expr, $already_active:expr) => {
         if unsafe { $func } == $crate::macros::IOCTL_ERROR {
             let ::errno::Errno(error_code) = ::errno::errno();
-            let io_error = ::std::io::Error::from_raw_os_error(error_code);
-            let mut err = Err($crate::ErrorKind::IoctlError(io_error).into());
+            let err = ::std::io::Error::from_raw_os_error(error_code);
             if error_code == $already_active {
-                err = err.chain_err(|| $crate::ErrorKind::StateAlreadyActive);
+                Err($crate::Error::StateAlreadyActive(err).into())
+            } else {
+                Err($crate::Error::from(err).into())
             }
-            err
         } else {
             Ok(()) as $crate::Result<()>
         }

src/pooladdr.rs

@@ -8,9 +8,9 @@
 
 use crate::{
     conversion::{CopyTo, TryCopyTo},
-    ffi, Interface, Ip, Result,
+    ffi, ConversionResult, Interface, Ip,
 };
-use std::{mem, ptr, vec::Vec};
+use std::{mem, ptr};
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct PoolAddr {
@@ -46,7 +46,9 @@ impl From<Ip> for PoolAddr {
 }
 
 impl TryCopyTo<ffi::pfvar::pf_pooladdr> for PoolAddr {
-    fn try_copy_to(&self, pf_pooladdr: &mut ffi::pfvar::pf_pooladdr) -> Result<()> {
+    type Result = ConversionResult<()>;
+
+    fn try_copy_to(&self, pf_pooladdr: &mut ffi::pfvar::pf_pooladdr) -> Self::Result {
         self.interface.try_copy_to(&mut pf_pooladdr.ifname)?;
         self.ip.copy_to(&mut pf_pooladdr.addr);
         Ok(())
@@ -67,7 +69,7 @@ pub struct PoolAddrList {
 }
 
 impl PoolAddrList {
-    pub fn new(pool_addrs: &[PoolAddr]) -> Result<Self> {
+    pub fn new(pool_addrs: &[PoolAddr]) -> ConversionResult<Self> {
         let mut pool = Self::init_pool(pool_addrs)?;
         Self::link_elements(&mut pool);
         let list = Self::create_palist(&mut pool);
@@ -84,7 +86,7 @@ impl PoolAddrList {
         self.list
     }
 
-    fn init_pool(pool_addrs: &[PoolAddr]) -> Result<Vec<ffi::pfvar::pf_pooladdr>> {
+    fn init_pool(pool_addrs: &[PoolAddr]) -> ConversionResult<Vec<ffi::pfvar::pf_pooladdr>> {
         let mut pool = Vec::with_capacity(pool_addrs.len());
         for pool_addr in pool_addrs {
             let mut pf_pooladdr = unsafe { mem::zeroed::<ffi::pfvar::pf_pooladdr>() };

src/rule/endpoint.rs

@@ -92,7 +92,9 @@ impl From<SocketAddr> for Endpoint {
 }
 
 impl TryCopyTo<ffi::pfvar::pf_rule_addr> for Endpoint {
-    fn try_copy_to(&self, pf_rule_addr: &mut ffi::pfvar::pf_rule_addr) -> Result<()> {
+    type Result = Result<()>;
+
+    fn try_copy_to(&self, pf_rule_addr: &mut ffi::pfvar::pf_rule_addr) -> Self::Result {
         self.ip.copy_to(&mut pf_rule_addr.addr);
         self.port
             .try_copy_to(unsafe { &mut pf_rule_addr.xport.range })?;

src/rule/gid.rs

@@ -6,7 +6,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-pub use super::uid::{Id, IdUnaryModifier};
+pub use super::uid::Id;
 use crate::{
     conversion::TryCopyTo,
     ffi::pfvar::{pf_rule_gid, PF_OP_NONE},
@@ -29,7 +29,9 @@ impl<T: Into<Id>> From<T> for Gid {
 }
 
 impl TryCopyTo<pf_rule_gid> for Gid {
-    fn try_copy_to(&self, pf_rule_gid: &mut pf_rule_gid) -> Result<()> {
+    type Result = Result<()>;
+
+    fn try_copy_to(&self, pf_rule_gid: &mut pf_rule_gid) -> Self::Result {
         match self.0 {
             Id::Any => {
                 pf_rule_gid.gid[0] = 0;