From 10639dcd83953c56ed902fe4f4e0ef4b2c3cca0a Mon Sep 17 00:00:00 2001 From: RoDmitry Date: Thu, 2 May 2024 13:39:17 +0000 Subject: [PATCH] Change `error-chain` to `thiserror` --- Cargo.toml | 3 +- examples/enable.rs | 2 +- src/lib.rs | 165 +++++++++++++++++++++++++++++----------- src/macros.rs | 8 +- src/pooladdr.rs | 12 +-- src/rule/endpoint.rs | 4 +- src/rule/gid.rs | 6 +- src/rule/interface.rs | 6 +- src/rule/ip.rs | 4 +- src/rule/mod.rs | 43 ++++++----- src/rule/port.rs | 24 +++--- src/rule/uid.rs | 6 +- src/transaction.rs | 12 +-- src/utils.rs | 9 ++- tests/anchors.rs | 4 +- tests/enable_disable.rs | 4 +- 16 files changed, 204 insertions(+), 108 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a067137..4b39f30 100644 --- a/Cargo.toml +++ b/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" diff --git a/examples/enable.rs b/examples/enable.rs index 9d76199..6768875 100644 --- a/examples/enable.rs +++ b/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(()) diff --git a/src/lib.rs b/src/lib.rs index 3d50d16..fb2701d 100644 --- a/src/lib.rs +++ b/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, + 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 for ConversionError { + fn from(source: ConversionErrorInner) -> Self { + Self { kind: None, source } + } +} + +impl From for Error { + fn from(source: ConversionErrorInner) -> Self { + Self::Conversion(ConversionError { kind: None, source }) + } +} + +pub type Result = ::std::result::Result; +pub type ConversionResult = ::std::result::Result; + +/// 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 { - 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 { - ensure!(cchars.iter().any(|&c| c == 0), "Not null terminated"); +fn compare_cstr_safe(s: &str, cchars: &[std::os::raw::c_char]) -> ConversionResult { + 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 { 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::() }; 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 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(&self, name: &str, kind: AnchorKind, f: F) -> Result 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) ); } diff --git a/src/macros.rs b/src/macros.rs index 0832b65..4b85906 100644 --- a/src/macros.rs +++ b/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<()> } diff --git a/src/pooladdr.rs b/src/pooladdr.rs index 1c3cfb9..6d7650f 100644 --- a/src/pooladdr.rs +++ b/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 for PoolAddr { } impl TryCopyTo 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 { + pub fn new(pool_addrs: &[PoolAddr]) -> ConversionResult { 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> { + fn init_pool(pool_addrs: &[PoolAddr]) -> ConversionResult> { let mut pool = Vec::with_capacity(pool_addrs.len()); for pool_addr in pool_addrs { let mut pf_pooladdr = unsafe { mem::zeroed::() }; diff --git a/src/rule/endpoint.rs b/src/rule/endpoint.rs index 51c901e..f7fac71 100644 --- a/src/rule/endpoint.rs +++ b/src/rule/endpoint.rs @@ -92,7 +92,9 @@ impl From for Endpoint { } impl TryCopyTo 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 })?; diff --git a/src/rule/gid.rs b/src/rule/gid.rs index 5bd9f91..ac27c4c 100644 --- a/src/rule/gid.rs +++ b/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> From for Gid { } impl TryCopyTo 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; diff --git a/src/rule/interface.rs b/src/rule/interface.rs index ccd2d49..0b9235b 100644 --- a/src/rule/interface.rs +++ b/src/rule/interface.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, Result}; +use crate::{conversion::TryCopyTo, ConversionResult}; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct InterfaceName(String); @@ -36,7 +36,9 @@ impl> From for Interface { } impl TryCopyTo<[i8]> for Interface { - fn try_copy_to(&self, dst: &mut [i8]) -> Result<()> { + type Result = ConversionResult<()>; + + fn try_copy_to(&self, dst: &mut [i8]) -> Self::Result { match *self { Interface::Any => "", Interface::Name(InterfaceName(ref name)) => &name[..], diff --git a/src/rule/ip.rs b/src/rule/ip.rs index faa4be6..4ad0ccf 100644 --- a/src/rule/ip.rs +++ b/src/rule/ip.rs @@ -10,7 +10,7 @@ use crate::{ conversion::CopyTo, ffi, pooladdr::{PoolAddr, PoolAddrList}, - AddrFamily, Result, + AddrFamily, ConversionResult, }; use ipnetwork::{IpNetwork, Ipv4Network, Ipv6Network}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; @@ -36,7 +36,7 @@ impl Ip { } /// Returns PoolAddrList initialized with receiver - pub fn to_pool_addr_list(&self) -> Result { + pub fn to_pool_addr_list(&self) -> ConversionResult { PoolAddrList::new(&[PoolAddr::from(*self)]) } } diff --git a/src/rule/mod.rs b/src/rule/mod.rs index bd541b5..57537dd 100644 --- a/src/rule/mod.rs +++ b/src/rule/mod.rs @@ -8,7 +8,8 @@ use crate::{ conversion::{CopyTo, TryCopyTo}, - ffi, ErrorKind, Result, ResultExt, + ffi, ConversionError, ConversionErrorInner, ConversionErrorKind, ConversionResult, Error, + Result, }; use derive_builder::Builder; use ipnetwork::IpNetwork; @@ -98,8 +99,7 @@ pub struct FilterRule { impl FilterRuleBuilder { pub fn build(&self) -> Result { - self.build_internal() - .map_err(|e| ErrorKind::InvalidRuleCombination(e).into()) + self.build_internal().map_err(Error::InvalidRuleCombination) } } @@ -128,14 +128,16 @@ impl FilterRule { "StatePolicy {:?} and protocol {:?} are incompatible", state_policy, proto ); - bail!(ErrorKind::InvalidRuleCombination(msg)); + Err(Error::InvalidRuleCombination(msg)) } } } } impl TryCopyTo for FilterRule { - fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Result<()> { + type Result = Result<()>; + + fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Self::Result { pf_rule.action = self.action.into(); pf_rule.direction = self.direction.into(); pf_rule.quick = self.quick as u8; @@ -148,7 +150,7 @@ impl TryCopyTo for FilterRule { self.interface .try_copy_to(&mut pf_rule.ifname) - .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?; + .map_err(|e| ConversionError::new(ConversionErrorKind::IncompatibleInterfaceName, e))?; pf_rule.proto = self.proto.into(); pf_rule.af = self.get_af()?.into(); @@ -197,8 +199,7 @@ pub struct RedirectRule { impl RedirectRuleBuilder { pub fn build(&self) -> Result { - self.build_internal() - .map_err(|e| ErrorKind::InvalidRuleCombination(e).into()) + self.build_internal().map_err(Error::InvalidRuleCombination) } } @@ -218,14 +219,16 @@ impl RedirectRule { } impl TryCopyTo for RedirectRule { - fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Result<()> { + type Result = Result<()>; + + fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Self::Result { pf_rule.action = self.action.into(); pf_rule.direction = self.direction.into(); pf_rule.quick = self.quick as u8; pf_rule.log = (&self.log).into(); self.interface .try_copy_to(&mut pf_rule.ifname) - .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?; + .map_err(|e| ConversionError::new(ConversionErrorKind::IncompatibleInterfaceName, e))?; pf_rule.proto = self.proto.into(); pf_rule.af = self.get_af()?.into(); @@ -246,7 +249,7 @@ fn compatible_af(af1: AddrFamily, af2: AddrFamily) -> Result { (AddrFamily::Any, af) => Ok(af), (af1, af2) => { let msg = format!("AddrFamily {} and {} are incompatible", af1, af2); - bail!(ErrorKind::InvalidRuleCombination(msg)); + Err(Error::InvalidRuleCombination(msg)) } } } @@ -480,19 +483,19 @@ impl CopyTo for Ipv6Addr { } impl> TryCopyTo<[i8]> for T { + type Result = ConversionResult<()>; + /// Safely copy a Rust string into a raw buffer. Returning an error if the string could not /// be copied to the buffer. - fn try_copy_to(&self, dst: &mut [i8]) -> Result<()> { + fn try_copy_to(&self, dst: &mut [i8]) -> Self::Result { let src_i8: &[i8] = unsafe { &*(self.as_ref().as_bytes() as *const _ as *const _) }; - ensure!( - src_i8.len() < dst.len(), - ErrorKind::InvalidArgument("String does not fit destination") - ); - ensure!( - !src_i8.contains(&0), - ErrorKind::InvalidArgument("String has null byte") - ); + if src_i8.len() >= dst.len() { + return Err(ConversionErrorInner::StrCopyNotFits); + } + if src_i8.contains(&0) { + return Err(ConversionErrorInner::StrCopyNullByte); + } dst[..src_i8.len()].copy_from_slice(src_i8); // Terminate ffi string with null byte diff --git a/src/rule/port.rs b/src/rule/port.rs index 388df93..2d6474e 100644 --- a/src/rule/port.rs +++ b/src/rule/port.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, ffi, ErrorKind, Result}; +use crate::{conversion::TryCopyTo, ffi, ConversionErrorInner, ConversionResult}; // Port range representation #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -29,7 +29,9 @@ impl From for Port { } impl TryCopyTo for Port { - fn try_copy_to(&self, pf_port_range: &mut ffi::pfvar::pf_port_range) -> Result<()> { + type Result = ConversionResult<()>; + + fn try_copy_to(&self, pf_port_range: &mut ffi::pfvar::pf_port_range) -> Self::Result { match *self { Port::Any => { pf_port_range.op = ffi::pfvar::PF_OP_NONE as u8; @@ -43,10 +45,9 @@ impl TryCopyTo for Port { pf_port_range.port[1] = 0; } Port::Range(start_port, end_port, modifier) => { - ensure!( - start_port <= end_port, - ErrorKind::InvalidArgument("Lower port is greater than upper port.") - ); + if start_port > end_port { + return Err(ConversionErrorInner::InvalidPortRange); + } pf_port_range.op = modifier.into(); // convert port range to network byte order pf_port_range.port[0] = start_port.to_be(); @@ -58,7 +59,9 @@ impl TryCopyTo for Port { } impl TryCopyTo for Port { - fn try_copy_to(&self, pf_pool: &mut ffi::pfvar::pf_pool) -> Result<()> { + type Result = ConversionResult<()>; + + fn try_copy_to(&self, pf_pool: &mut ffi::pfvar::pf_pool) -> Self::Result { match *self { Port::Any => { pf_pool.port_op = ffi::pfvar::PF_OP_NONE as u8; @@ -71,10 +74,9 @@ impl TryCopyTo for Port { pf_pool.proxy_port[1] = 0; } Port::Range(start_port, end_port, modifier) => { - ensure!( - start_port <= end_port, - ErrorKind::InvalidArgument("Lower port is greater than upper port.") - ); + if start_port > end_port { + return Err(ConversionErrorInner::InvalidPortRange); + } pf_pool.port_op = modifier.into(); pf_pool.proxy_port[0] = start_port; pf_pool.proxy_port[1] = end_port; diff --git a/src/rule/uid.rs b/src/rule/uid.rs index f9cec4d..2e91154 100644 --- a/src/rule/uid.rs +++ b/src/rule/uid.rs @@ -9,7 +9,7 @@ use crate::{ conversion::TryCopyTo, ffi::pfvar::{self, pf_rule_uid}, - Result, + ConversionResult, }; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -41,7 +41,9 @@ impl> From for Uid { } impl TryCopyTo for Uid { - fn try_copy_to(&self, pf_rule_uid: &mut pf_rule_uid) -> Result<()> { + type Result = ConversionResult<()>; + + fn try_copy_to(&self, pf_rule_uid: &mut pf_rule_uid) -> Self::Result { match self.0 { Id::Any => { pf_rule_uid.uid[0] = 0; diff --git a/src/transaction.rs b/src/transaction.rs index b753c10..086b4a6 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -7,8 +7,8 @@ // except according to those terms. use crate::{ - conversion::TryCopyTo, ffi, utils, ErrorKind, FilterRule, PoolAddrList, RedirectRule, Result, - ResultExt, RulesetKind, + conversion::TryCopyTo, ffi, utils, ConversionError, ConversionErrorKind, FilterRule, + PoolAddrList, RedirectRule, Result, RulesetKind, }; use std::{ collections::HashMap, @@ -112,7 +112,7 @@ impl Transaction { pfioc_rule.action = ffi::pfvar::PF_CHANGE_NONE as u32; 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)?; // request new address pool @@ -124,7 +124,7 @@ impl Transaction { // register pool address with firewall utils::add_pool_address(fd, pool_addr.clone(), pool_ticket)?; let pool_addr_list = PoolAddrList::new(&[pool_addr.clone()]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid route target"))?; + .map_err(|e| ConversionError::new(ConversionErrorKind::InvalidRouteTarget, e))?; pfioc_rule.rule.rpool.list = unsafe { pool_addr_list.to_palist() }; Some(pool_addr_list) @@ -146,7 +146,7 @@ impl Transaction { let mut pfioc_rule = unsafe { mem::zeroed::() }; 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)?; // register redirect address in newly created address pool @@ -186,7 +186,7 @@ impl Transaction { pfioc_trans_e.rs_num = ruleset_kind.into(); anchor .try_copy_to(&mut pfioc_trans_e.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + .map_err(|e| ConversionError::new(ConversionErrorKind::InvalidAnchorName, e))?; Ok(pfioc_trans_e) } } diff --git a/src/utils.rs b/src/utils.rs index 160bb31..dd57a4f 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -6,7 +6,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, ffi, AnchorKind, ErrorKind, PoolAddr, Result, ResultExt}; +use crate::{ + conversion::TryCopyTo, ffi, AnchorKind, ConversionError, ConversionErrorKind, Error, PoolAddr, + Result, +}; use std::{ fs::{File, OpenOptions}, mem, @@ -22,7 +25,7 @@ pub fn open_pf() -> Result { .read(true) .write(true) .open(PF_DEV_PATH) - .chain_err(|| ErrorKind::DeviceOpenError(PF_DEV_PATH)) + .map_err(|e| Error::DeviceOpen(PF_DEV_PATH, e)) } /// Add pool address using the pool ticket previously obtained via `get_pool_ticket()` @@ -50,7 +53,7 @@ pub fn get_ticket(fd: RawFd, anchor: &str, kind: AnchorKind) -> Result { pfioc_rule.rule.action = kind.into(); anchor .try_copy_to(&mut pfioc_rule.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + .map_err(|e| ConversionError::new(ConversionErrorKind::InvalidAnchorName, e))?; ioctl_guard!(ffi::pf_change_rule(fd, &mut pfioc_rule))?; Ok(pfioc_rule.ticket) } diff --git a/tests/anchors.rs b/tests/anchors.rs index b541ee2..3bb7970 100644 --- a/tests/anchors.rs +++ b/tests/anchors.rs @@ -30,7 +30,7 @@ test!(add_filter_anchor { assert_matches!( pf.add_anchor(&anchor_name, pfctl::AnchorKind::Filter), - Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _)) + Err(pfctl::Error::StateAlreadyActive(_)) ); assert_matches!(pf.try_add_anchor(&anchor_name, pfctl::AnchorKind::Filter), Ok(())); }); @@ -47,7 +47,7 @@ test!(remove_filter_anchor { assert_matches!( pf.remove_anchor(&anchor_name, pfctl::AnchorKind::Filter), - Err(pfctl::Error(pfctl::ErrorKind::AnchorDoesNotExist, _)) + Err(pfctl::Error::AnchorDoesNotExist) ); assert_matches!(pf.try_remove_anchor(&anchor_name, pfctl::AnchorKind::Filter), Ok(())); }); diff --git a/tests/enable_disable.rs b/tests/enable_disable.rs index a944a97..b493b5c 100644 --- a/tests/enable_disable.rs +++ b/tests/enable_disable.rs @@ -17,7 +17,7 @@ test!(enable_pf { assert_matches!(pfcli::disable_firewall(), Ok(())); assert_matches!(pf.enable(), Ok(())); assert_matches!(pfcli::is_enabled(), Ok(true)); - assert_matches!(pf.enable(), Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _))); + assert_matches!(pf.enable(), Err(pfctl::Error::StateAlreadyActive(_))); assert_matches!(pf.try_enable(), Ok(())); assert_matches!(pfcli::is_enabled(), Ok(true)); }); @@ -28,7 +28,7 @@ test!(disable_pf { assert_matches!(pfcli::enable_firewall(), Ok(())); assert_matches!(pf.disable(), Ok(())); assert_matches!(pfcli::is_enabled(), Ok(false)); - assert_matches!(pf.disable(), Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _))); + assert_matches!(pf.disable(), Err(pfctl::Error::StateAlreadyActive(_))); assert_matches!(pf.try_disable(), Ok(())); assert_matches!(pfcli::is_enabled(), Ok(false)); });