From c2bca00ac47d59e7a4f6d7696d470ad1d4a8b1be Mon Sep 17 00:00:00 2001 From: Walker Crouse Date: Tue, 20 Aug 2024 10:09:58 -0400 Subject: [PATCH 1/2] define better errors with thiserror Signed-off-by: Walker Crouse --- Cargo.lock | 21 +++++++++++ zeroconf/Cargo.toml | 1 + zeroconf/src/bonjour/bonjour_util.rs | 23 ++++++------ zeroconf/src/bonjour/browser.rs | 53 +++++++++++++++------------- zeroconf/src/bonjour/service.rs | 10 ++++-- zeroconf/src/error.rs | 44 ++++++++++------------- zeroconf/src/ffi/mod.rs | 7 ++-- zeroconf/src/lib.rs | 1 + zeroconf/src/service_type.rs | 10 +++--- 9 files changed, 101 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d12a50f..cc5834a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -634,6 +634,26 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "thiserror" +version = "1.0.63" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.63" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.72", +] + [[package]] name = "unicode-ident" version = "1.0.12" @@ -756,6 +776,7 @@ dependencies = [ "maplit", "serde", "serde_json", + "thiserror", "zeroconf-macros", ] diff --git a/zeroconf/Cargo.toml b/zeroconf/Cargo.toml index 8bdb89d..72701a3 100644 --- a/zeroconf/Cargo.toml +++ b/zeroconf/Cargo.toml @@ -26,6 +26,7 @@ derive-new = "0.5.9" log = "0.4.20" libc = "0.2.148" zeroconf-macros = { path = "../zeroconf-macros", version = "0.1.4" } +thiserror = "1.0.63" [dev-dependencies] env_logger = "0.10.0" diff --git a/zeroconf/src/bonjour/bonjour_util.rs b/zeroconf/src/bonjour/bonjour_util.rs index 511ccaf..bab39b4 100644 --- a/zeroconf/src/bonjour/bonjour_util.rs +++ b/zeroconf/src/bonjour/bonjour_util.rs @@ -3,7 +3,9 @@ use std::{ffi::CString, str::FromStr}; use super::constants; -use crate::{check_valid_characters, lstrip_underscore, NetworkInterface, Result, ServiceType}; +use crate::{ + check_valid_characters, lstrip_underscore, Error, NetworkInterface, Result, ServiceType, +}; use bonjour_sys::DNSServiceErrorType; /// Normalizes the specified domain `&str` to conform to a standard enforced by this crate. @@ -46,7 +48,10 @@ pub fn sys_exec DNSServiceErrorType>(func: F, message: &str) -> R let err = func(); if err < 0 { - Err(format!("{} (code: {})", message, err).into()) + Err(Error::MdnsSystemError { + code: err, + message: message.into(), + }) } else { Ok(()) } @@ -106,10 +111,9 @@ mod tests { #[test] fn parse_regtype_failure_invalid_regtype() { - assert_eq!( - parse_regtype("foobar"), - Err("invalid name and protocol".into()) - ); + let expected = Error::InvalidServiceType("invalid name and protocol".into()); + let result = parse_regtype("foobar"); + assert_eq!(result, Err(expected)); } #[test] @@ -132,10 +136,9 @@ mod tests { #[test] fn sys_exec_returns_error() { - assert_eq!( - sys_exec(|| -42, "uh oh spaghetti-o"), - Err("uh oh spaghetti-o (code: -42)".into()) - ); + let expected = Error::InvalidServiceType("uh oh spaghetti-o (code: -42)".into()); + let result = sys_exec(|| -42, "uh oh spaghetti-o"); + assert_eq!(result, Err(expected)); } #[test] diff --git a/zeroconf/src/bonjour/browser.rs b/zeroconf/src/bonjour/browser.rs index d410b3f..6ccdf97 100644 --- a/zeroconf/src/bonjour/browser.rs +++ b/zeroconf/src/bonjour/browser.rs @@ -6,7 +6,7 @@ use super::service_ref::{ use super::txt_record_ref::ManagedTXTRecordRef; use super::{bonjour_util, constants}; use crate::ffi::{c_str, AsRaw, FromRaw}; -use crate::prelude::*; +use crate::{prelude::*, Error}; use crate::{EventLoop, NetworkInterface, Result, ServiceType, TxtRecord}; use crate::{ServiceDiscoveredCallback, ServiceDiscovery}; #[cfg(target_vendor = "pc")] @@ -78,7 +78,8 @@ impl TMdnsBrowser for BonjourMdnsBrowser { .domain(ptr::null_mut()) .callback(Some(browse_callback)) .context(self.context.as_raw()) - .build()?; + .build() + .map_err(Error::BrowserError)?; unsafe { service_lock.browse_services(browse_params)? }; @@ -143,7 +144,10 @@ unsafe fn handle_browse( interface_index: u32, ) -> Result<()> { if error != 0 { - return Err(format!("browse_callback() reported error (code: {})", error).into()); + return Err(Error::MdnsSystemError { + code: error, + message: "browse_callback() reported error".into(), + }); } ctx.resolved_name = Some(c_str::copy_raw(name)); @@ -159,7 +163,8 @@ unsafe fn handle_browse( .domain(domain) .callback(Some(resolve_callback)) .context(ctx.as_raw()) - .build()?, + .build() + .map_err(Error::BrowserError)?, ) } @@ -202,7 +207,10 @@ unsafe fn handle_resolve( txt_record: *const c_uchar, ) -> Result<()> { if error != 0 { - return Err(format!("error reported by resolve_callback: (code: {})", error).into()); + return Err(Error::MdnsSystemError { + code: error, + message: "resolve_callback() reported error".into(), + }); } ctx.resolved_port = port; @@ -223,7 +231,8 @@ unsafe fn handle_resolve( .hostname(host_target) .callback(Some(get_address_info_callback)) .context(ctx.as_raw()) - .build()?, + .build() + .map_err(Error::BrowserError)?, ) } @@ -255,11 +264,10 @@ unsafe fn handle_get_address_info( } if error != 0 { - return Err(format!( - "get_address_info_callback() reported error (code: {})", - error - ) - .into()); + return Err(Error::MdnsSystemError { + code: error, + message: "get_address_info_callback() reported error".into(), + }); } // on macOS the bytes are swapped for the port @@ -285,22 +293,17 @@ unsafe fn handle_get_address_info( let hostname = c_str::copy_raw(hostname); - let domain = bonjour_util::normalize_domain( - &ctx.resolved_domain - .take() - .ok_or("could not get domain from BonjourBrowserContext")?, - ); + let domain = bonjour_util::normalize_domain(&ctx.resolved_domain.take().ok_or( + Error::BrowserError("could not get domain from BonjourBrowserContext".into()), + )?); - let kind = bonjour_util::normalize_domain( - &ctx.resolved_kind - .take() - .ok_or("could not get kind from BonjourBrowserContext")?, - ); + let kind = bonjour_util::normalize_domain(&ctx.resolved_kind.take().ok_or( + Error::BrowserError("could not get kind from BonjourBrowserContext".into()), + )?); - let name = ctx - .resolved_name - .take() - .ok_or("could not get name from BonjourBrowserContext")?; + let name = ctx.resolved_name.take().ok_or(Error::BrowserError( + "could not get name from BonjourBrowserContext".into(), + ))?; let result = ServiceDiscovery::builder() .name(name) diff --git a/zeroconf/src/bonjour/service.rs b/zeroconf/src/bonjour/service.rs index 014d6f7..4c821f7 100644 --- a/zeroconf/src/bonjour/service.rs +++ b/zeroconf/src/bonjour/service.rs @@ -4,7 +4,7 @@ use super::service_ref::{ManagedDNSServiceRef, RegisterServiceParams}; use super::{bonjour_util, constants}; use crate::ffi::c_str::{self, AsCChars}; use crate::ffi::{AsRaw, FromRaw, UnwrapOrNull}; -use crate::prelude::*; +use crate::{prelude::*, Error}; use crate::{ EventLoop, NetworkInterface, Result, ServiceRegisteredCallback, ServiceRegistration, ServiceType, TxtRecord, @@ -129,7 +129,8 @@ impl TMdnsService for BonjourMdnsService { .txt_record(txt_record) .callback(Some(register_callback)) .context(self.context.as_raw()) - .build()?; + .build() + .map_err(Error::ServiceError)?; unsafe { service_lock.register_service(register_params)? }; @@ -185,7 +186,10 @@ unsafe fn handle_register( regtype: *const c_char, ) -> Result<()> { if error != 0 { - return Err(format!("register_callback() reported error (code: {0})", error).into()); + return Err(Error::MdnsSystemError { + code: error, + message: "register_callback() reported error".to_string(), + }); } let domain = bonjour_util::normalize_domain(c_str::raw_to_str(domain)); diff --git a/zeroconf/src/error.rs b/zeroconf/src/error.rs index 22fd9bf..6451bd5 100644 --- a/zeroconf/src/error.rs +++ b/zeroconf/src/error.rs @@ -1,29 +1,23 @@ //! Utilities regarding error handling -use std::fmt; +use thiserror::Error; -/// For when something goes wrong when interfacing with mDNS implementations -#[derive(new, Debug, Clone, PartialEq, Eq)] -pub struct Error { - description: String, -} - -impl std::error::Error for Error {} - -impl fmt::Display for Error { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.description) - } -} - -impl From<&str> for Error { - fn from(s: &str) -> Self { - Error::from(s.to_string()) - } -} - -impl From for Error { - fn from(s: String) -> Self { - Error::new(s) - } +/// Error type for the zeroconf crate +#[derive(Error, Debug, PartialEq)] +pub enum Error { + /// An instance of `crate::ServiceType` could not be created due to an invalid format + #[error("Invalid ServiceType format: {0}")] + InvalidServiceType(String), + /// An error occurred in the underlying mDNS system (Avahi/Bonjour) + #[error("{message} (code: {code})")] + MdnsSystemError { code: i32, message: String }, + /// An error occurred in the underlying system (ABI) + #[error("{message} (code: {code})")] + SystemError { code: i32, message: String }, + /// An error occurred in an instance of an `crate::MdnsBrowser` + #[error("{0}")] + BrowserError(String), + /// An error occurred in an instance of an `crate::MdnsService` + #[error("{0}")] + ServiceError(String), } diff --git a/zeroconf/src/ffi/mod.rs b/zeroconf/src/ffi/mod.rs index acd3147..754bf4e 100644 --- a/zeroconf/src/ffi/mod.rs +++ b/zeroconf/src/ffi/mod.rs @@ -53,7 +53,7 @@ impl UnwrapMutOrNull for Option<*mut T> { #[cfg(target_vendor = "apple")] pub(crate) mod bonjour { - use crate::Result; + use crate::{Error, Result}; use libc::{fd_set, suseconds_t, time_t, timeval}; use std::time::Duration; use std::{mem, ptr}; @@ -82,7 +82,10 @@ pub(crate) mod bonjour { ); if result < 0 { - Err("select(): returned error status".into()) + Err(Error::SystemError { + code: result, + message: "select(): returned error status".to_string(), + }) } else { Ok(result as u32) } diff --git a/zeroconf/src/lib.rs b/zeroconf/src/lib.rs index ad88d21..710aaeb 100644 --- a/zeroconf/src/lib.rs +++ b/zeroconf/src/lib.rs @@ -229,6 +229,7 @@ pub mod avahi; pub mod bonjour; pub use browser::{ServiceDiscoveredCallback, ServiceDiscovery}; +pub use error::Error; pub use interface::*; pub use service::{ServiceRegisteredCallback, ServiceRegistration}; pub use service_type::*; diff --git a/zeroconf/src/service_type.rs b/zeroconf/src/service_type.rs index 7d1be22..14645df 100644 --- a/zeroconf/src/service_type.rs +++ b/zeroconf/src/service_type.rs @@ -44,7 +44,8 @@ impl FromStr for ServiceType { let parts = s.split('.').collect::>(); if parts.len() != 2 { - return Err("invalid name and protocol".into()); + let msg = "invalid name and protocol"; + return Err(Error::InvalidServiceType(msg.into())); } let name = lstrip_underscore(check_valid_characters(parts[0])?); @@ -56,11 +57,12 @@ impl FromStr for ServiceType { pub fn check_valid_characters(part: &str) -> Result<&str> { if part.contains('.') { - Err("invalid character: .".into()) + let msg = "invalid character: ."; + Err(Error::InvalidServiceType(msg.into())) } else if part.contains(',') { - Err("invalid character: ,".into()) + Err(Error::InvalidServiceType("invalid character: ,".into())) } else if part.is_empty() { - Err("cannot be empty".into()) + Err(Error::InvalidServiceType("cannot be empty".into())) } else { Ok(part) } From e72794f47d7a23395f57c1d2b8d1b1244efde757 Mon Sep 17 00:00:00 2001 From: Walker Crouse Date: Tue, 20 Aug 2024 23:39:21 -0400 Subject: [PATCH 2/2] unit tests Signed-off-by: Walker Crouse --- zeroconf/src/error.rs | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/zeroconf/src/error.rs b/zeroconf/src/error.rs index 6451bd5..953c667 100644 --- a/zeroconf/src/error.rs +++ b/zeroconf/src/error.rs @@ -21,3 +21,47 @@ pub enum Error { #[error("{0}")] ServiceError(String), } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_invalid_service_type_display() { + let error = Error::InvalidServiceType("invalid name and protocol".into()); + assert_eq!( + error.to_string(), + "Invalid ServiceType format: invalid name and protocol" + ); + } + + #[test] + fn test_mdns_system_error_display() { + let error = Error::MdnsSystemError { + code: -42, + message: "uh oh spaghetti-o".into(), + }; + assert_eq!(error.to_string(), "uh oh spaghetti-o (code: -42)"); + } + + #[test] + fn test_system_error_display() { + let error = Error::SystemError { + code: -42, + message: "uh oh spaghetti-o".into(), + }; + assert_eq!(error.to_string(), "uh oh spaghetti-o (code: -42)"); + } + + #[test] + fn test_browser_error_display() { + let error = Error::BrowserError("uh oh spaghetti-o".into()); + assert_eq!(error.to_string(), "uh oh spaghetti-o"); + } + + #[test] + fn test_service_error_display() { + let error = Error::ServiceError("uh oh spaghetti-o".into()); + assert_eq!(error.to_string(), "uh oh spaghetti-o"); + } +}