Refactor error handling

README.md

@@ -21,7 +21,7 @@ data/tests/rust-logo-128x128-blk.png: PNG image data, 128 x 128, 8-bit/color RGB
 
 You can implement something similar in Rust with the `magic` crate (see [examples/file-ish.rs](examples/file-ish.rs)):
 ```rust
-fn file_example() -> Result<(), magic::MagicError> {
+fn file_example() -> Result<(), Box<dyn std::error::Error>> {
     // Open a new configuration with flags
     let cookie = magic::Cookie::open(magic::CookieFlags::ERROR)?;
 

examples/file-ish.rs

@@ -12,7 +12,7 @@
 //! PNG image data, 128 x 128, 8-bit/color RGBA, non-interlaced
 //! ```
 
-fn main() -> Result<(), magic::MagicError> {
+fn main() -> Result<(), Box<dyn std::error::Error>> {
     // Open a new configuration with flags
     let cookie = magic::Cookie::open(magic::CookieFlags::ERROR)?;
 

src/ffi.rs

@@ -9,44 +9,29 @@
 
 use magic_sys as libmagic;
 
-#[non_exhaustive]
+/// Error for opened `magic_t` instance
 #[derive(thiserror::Error, Debug)]
-pub(crate) enum LibmagicError {
-    /// Error during `magic_open`
-    #[error("Error calling `magic_open`, errno: {errno}")]
-    Open { errno: std::io::Error },
-
-    /// Error for opened `magic_t` instance
-    #[error("Error for cookie call ({}): {}",
-        match .errno {
-            Some(errno) => format!("OS errno: {}", errno),
-            None => "no OS errno".to_string(),
-        },
-        .explanation.to_string_lossy()
-    )]
-    Cookie {
-        explanation: std::ffi::CString,
-        errno: Option<std::io::Error>,
-    },
-
-    /// Error during `magic_setflags`
-    #[error("Error calling `magic_setflags`, unsupported flags: {flags}")]
-    UnsupportedFlags { flags: libc::c_int },
-
-    /// `libmagic` did not behave according to its API
-    #[error("Error in `libmagic` behavior, violated its API: {description}")]
-    ApiViolation { description: String },
-}
-
-fn last_error(cookie: libmagic::magic_t) -> Option<LibmagicError> {
+#[error("magic cookie error ({}): {}",
+match .errno {
+    Some(errno) => format!("OS errno: {}", errno),
+    None => "no OS errno".to_string(),
+},
+.explanation.to_string_lossy()
+)]
+pub(crate) struct CookieError {
+    explanation: std::ffi::CString,
+    errno: Option<std::io::Error>,
+}
+
+fn last_error(cookie: libmagic::magic_t) -> Option<CookieError> {
     let error = unsafe { libmagic::magic_error(cookie) };
     let errno = unsafe { libmagic::magic_errno(cookie) };
 
     if error.is_null() {
         None
     } else {
         let c_str = unsafe { std::ffi::CStr::from_ptr(error) };
-        Some(LibmagicError::Cookie {
+        Some(CookieError {
             explanation: c_str.into(),
             errno: match errno {
                 0 => None,
@@ -56,21 +41,31 @@ fn last_error(cookie: libmagic::magic_t) -> Option<LibmagicError> {
     }
 }
 
-fn expect_error(cookie: libmagic::magic_t, description: String) -> LibmagicError {
+fn api_violation(cookie: libmagic::magic_t, description: String) -> ! {
+    panic!(
+        "`libmagic` API violation for magic cookie {:?}: {}",
+        cookie, description
+    );
+}
+
+fn expect_error(cookie: libmagic::magic_t, description: String) -> CookieError {
     match last_error(cookie) {
         Some(err) => err,
-        None => LibmagicError::ApiViolation { description },
+        _ => api_violation(cookie, description),
     }
 }
 
 pub(crate) fn close(cookie: libmagic::magic_t) {
     unsafe { libmagic::magic_close(cookie) }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error.
 pub(crate) fn file(
     cookie: libmagic::magic_t,
     filename: &std::ffi::CStr, // TODO: Support NULL
-) -> Result<std::ffi::CString, LibmagicError> {
+) -> Result<std::ffi::CString, CookieError> {
     let filename_ptr = filename.as_ptr();
     let res = unsafe { libmagic::magic_file(cookie, filename_ptr) };
 
@@ -85,10 +80,13 @@ pub(crate) fn file(
     }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error.
 pub(crate) fn buffer(
     cookie: libmagic::magic_t,
     buffer: &[u8],
-) -> Result<std::ffi::CString, LibmagicError> {
+) -> Result<std::ffi::CString, CookieError> {
     let buffer_ptr = buffer.as_ptr();
     let buffer_len = buffer.len() as libc::size_t;
     let res = unsafe { libmagic::magic_buffer(cookie, buffer_ptr, buffer_len) };
@@ -104,18 +102,27 @@ pub(crate) fn buffer(
     }
 }
 
-pub(crate) fn setflags(cookie: libmagic::magic_t, flags: libc::c_int) -> Result<(), LibmagicError> {
+pub(crate) fn setflags(cookie: libmagic::magic_t, flags: libc::c_int) -> Result<(), SetFlagsError> {
     let ret = unsafe { libmagic::magic_setflags(cookie, flags) };
     match ret {
-        -1 => Err(LibmagicError::UnsupportedFlags { flags }),
+        -1 => Err(SetFlagsError { flags }),
         _ => Ok(()),
     }
 }
 
+#[derive(thiserror::Error, Debug)]
+#[error("could not set magic cookie flags {}", .flags)]
+pub(crate) struct SetFlagsError {
+    flags: libc::c_int,
+}
+
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error or returning undefined data.
 pub(crate) fn check(
     cookie: libmagic::magic_t,
     filename: Option<&std::ffi::CStr>,
-) -> Result<(), LibmagicError> {
+) -> Result<(), CookieError> {
     let filename_ptr = filename.map_or_else(std::ptr::null, std::ffi::CStr::as_ptr);
     let res = unsafe { libmagic::magic_check(cookie, filename_ptr) };
 
@@ -125,16 +132,20 @@ pub(crate) fn check(
             cookie,
             "`magic_check()` did not set last error".to_string(),
         )),
-        res => Err(LibmagicError::ApiViolation {
-            description: format!("Expected 0 or -1 but `magic_check()` returned {}", res),
-        }),
+        res => api_violation(
+            cookie,
+            format!("expected 0 or -1 but `magic_check()` returned {}", res),
+        ),
     }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error or returning undefined data.
 pub(crate) fn compile(
     cookie: libmagic::magic_t,
     filename: Option<&std::ffi::CStr>,
-) -> Result<(), LibmagicError> {
+) -> Result<(), CookieError> {
     let filename_ptr = filename.map_or_else(std::ptr::null, std::ffi::CStr::as_ptr);
     let res = unsafe { libmagic::magic_compile(cookie, filename_ptr) };
 
@@ -144,16 +155,20 @@ pub(crate) fn compile(
             cookie,
             "`magic_compile()` did not set last error".to_string(),
         )),
-        res => Err(LibmagicError::ApiViolation {
-            description: format!("Expected 0 or -1 but `magic_compile()` returned {}", res),
-        }),
+        res => api_violation(
+            cookie,
+            format!("Expected 0 or -1 but `magic_compile()` returned {}", res),
+        ),
     }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error or returning undefined data.
 pub(crate) fn list(
     cookie: libmagic::magic_t,
     filename: Option<&std::ffi::CStr>,
-) -> Result<(), LibmagicError> {
+) -> Result<(), CookieError> {
     let filename_ptr = filename.map_or_else(std::ptr::null, std::ffi::CStr::as_ptr);
     let res = unsafe { libmagic::magic_list(cookie, filename_ptr) };
 
@@ -163,16 +178,20 @@ pub(crate) fn list(
             cookie,
             "`magic_list()` did not set last error".to_string(),
         )),
-        res => Err(LibmagicError::ApiViolation {
-            description: format!("Expected 0 or -1 but `magic_list()` returned {}", res),
-        }),
+        res => api_violation(
+            cookie,
+            format!("Expected 0 or -1 but `magic_list()` returned {}", res),
+        ),
     }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error or returning undefined data.
 pub(crate) fn load(
     cookie: libmagic::magic_t,
     filename: Option<&std::ffi::CStr>,
-) -> Result<(), LibmagicError> {
+) -> Result<(), CookieError> {
     let filename_ptr = filename.map_or_else(std::ptr::null, std::ffi::CStr::as_ptr);
     let res = unsafe { libmagic::magic_load(cookie, filename_ptr) };
 
@@ -182,16 +201,20 @@ pub(crate) fn load(
             cookie,
             "`magic_load()` did not set last error".to_string(),
         )),
-        res => Err(LibmagicError::ApiViolation {
-            description: format!("Expected 0 or -1 but `magic_load()` returned {}", res),
-        }),
+        res => api_violation(
+            cookie,
+            format!("Expected 0 or -1 but `magic_load()` returned {}", res),
+        ),
     }
 }
 
+/// # Panics
+///
+/// Panics if `libmagic` violates its API contract, e.g. by not setting the last error or returning undefined data.
 pub(crate) fn load_buffers(
     cookie: libmagic::magic_t,
     buffers: &[&[u8]],
-) -> Result<(), LibmagicError> {
+) -> Result<(), CookieError> {
     let mut ffi_buffers: Vec<*const u8> = Vec::with_capacity(buffers.len());
     let mut ffi_sizes: Vec<libc::size_t> = Vec::with_capacity(buffers.len());
     let ffi_nbuffers = buffers.len() as libc::size_t;
@@ -214,27 +237,44 @@ pub(crate) fn load_buffers(
             cookie,
             "`magic_load_buffers()` did not set last error".to_string(),
         )),
-        res => Err(LibmagicError::ApiViolation {
-            description: format!(
+        res => api_violation(
+            cookie,
+            format!(
                 "Expected 0 or -1 but `magic_load_buffers()` returned {}",
                 res
             ),
-        }),
+        ),
     }
 }
 
-pub(crate) fn open(flags: libc::c_int) -> Result<libmagic::magic_t, LibmagicError> {
+pub(crate) fn open(flags: libc::c_int) -> Result<libmagic::magic_t, OpenError> {
     let cookie = unsafe { libmagic::magic_open(flags) };
 
     if cookie.is_null() {
-        Err(LibmagicError::Open {
+        Err(OpenError {
+            flags,
+            // note that libmagic only really cares about MAGIC_PRESERVE_ATIME
+            // invalid bits in `flags` still result in a valid cookie pointer
             errno: std::io::Error::last_os_error(),
         })
     } else {
         Ok(cookie)
     }
 }
 
+#[derive(thiserror::Error, Debug)]
+#[error("could not open magic cookie with flags {}: {}", .flags, .errno)]
+pub(crate) struct OpenError {
+    flags: libc::c_int,
+    errno: std::io::Error,
+}
+
+impl OpenError {
+    pub fn errno(&self) -> &std::io::Error {
+        &self.errno
+    }
+}
+
 pub(crate) fn version() -> libc::c_int {
     unsafe { libmagic::magic_version() }
 }