From 41c0b3ee9e03de335147a71c8f42dd08892001b3 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Wed, 3 Jul 2024 18:41:22 +0200 Subject: [PATCH] Replace Error enum with struct To make it easier to add support for new error codes without breaking compatibility, this patch replaces the Error enum with a struct and associated constants. As a part of this change, it also enforces that Error is not used for return values indicating success, i. e. only for negative return values. --- CHANGELOG.md | 4 + src/fs.rs | 18 ++-- src/io.rs | 199 ++++++++++++++++++++++++++++----------------- src/object_safe.rs | 10 +-- src/tests.rs | 18 ++-- 5 files changed, 150 insertions(+), 99 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8a6522c..26f8d35a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,10 +28,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Replace all panicking `Path`/`PathBuf` conversions with fallible alternatives: - Return a `Result` from `Path::from_str_with_nul`. - Replace the `From<_>` implementations for `Path` and `PathBuf` with `TryFrom<_>`, except for `From<&Path> for PathBuf`. +- Refactor error types: + - Change `Error` enum to a struct with associated constants. + - Remove `Error::Success` and enforce negative values error codes for `Error`. ### Removed - Removed `Path::from_bytes_with_nul_unchecked`. Use `CStr::from_bytes_with_nul_unchecked` and `Path::from_cstr_unchecked` instead. +- Removed `From for littlefs2::io::Error`. [#47]: https://github.com/trussed-dev/littlefs2/pull/47 [#57]: https://github.com/trussed-dev/littlefs2/pull/57 diff --git a/src/fs.rs b/src/fs.rs index 2f8be1706..7c2064556 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1254,23 +1254,19 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { let path_slice = path.as_ref().as_bytes(); for i in 0..path_slice.len() { if path_slice[i] == b'/' { - let dir = PathBuf::try_from(&path_slice[..i])?; + let dir = PathBuf::try_from(&path_slice[..i]).map_err(|_| Error::IO)?; #[cfg(test)] println!("generated PathBuf dir {:?} using i = {}", &dir, i); - match self.create_dir(&dir) { - Ok(_) => {} - Err(io::Error::EntryAlreadyExisted) => {} - error => { - panic!("{:?}", &error); + if let Err(error) = self.create_dir(&dir) { + if error != Error::ENTRY_ALREADY_EXISTED { + return Err(error); } } } } - match self.create_dir(path) { - Ok(_) => {} - Err(io::Error::EntryAlreadyExisted) => {} - error => { - panic!("{:?}", &error); + if let Err(error) = self.create_dir(path) { + if error != Error::ENTRY_ALREADY_EXISTED { + return Err(error); } } Ok(()) diff --git a/src/io.rs b/src/io.rs index b8ad5fdc1..9b01c99db 100644 --- a/src/io.rs +++ b/src/io.rs @@ -2,6 +2,11 @@ pub mod prelude; +use core::{ + ffi::c_int, + fmt::{self, Debug, Formatter}, +}; + use littlefs2_sys as ll; /// The `Read` trait allows for reading bytes from a file. @@ -17,7 +22,7 @@ pub trait Read { Ok(()) } else { // TODO: Decide whether to add an equivalent of `ErrorKind::UnexpectedEof` - Err(Error::Io) + Err(Error::IO) } } } @@ -43,7 +48,7 @@ pub trait Write { match self.write(buf) { Ok(0) => { // failed to write whole buffer - return Err(Error::Io); + return Err(Error::IO); } Ok(n) => buf = &buf[n..], Err(e) => return Err(e), @@ -112,105 +117,151 @@ pub trait Seek { pub type Result = core::result::Result; -/// Definition of errors that might be returned by filesystem functionality. -#[derive(Clone, Copy, Debug, PartialEq)] -pub enum Error { - /// Error code was >=0, operation was successful. - Success, +/// The error type for filesystem operations. +/// +/// Specific error codes are available as associated constants of this type. +/// +/// ``` +/// # use littlefs2::io::Error; +/// +/// assert_eq!(Error::IO.code(), -5); +/// assert_eq!(Error::new(-5), Some(Error::IO)); +/// ``` +#[derive(Clone, Copy, PartialEq)] +pub struct Error { + code: c_int, +} + +impl Error { /// Input / output error occurred. - Io, + pub const IO: Self = Self::new_const(ll::lfs_error_LFS_ERR_IO); + /// File or filesystem was corrupt. - Corruption, + pub const CORRUPTION: Self = Self::new_const(ll::lfs_error_LFS_ERR_CORRUPT); + /// No entry found with that name. - NoSuchEntry, + pub const NO_SUCH_ENTRY: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOENT); + /// File or directory already exists. - EntryAlreadyExisted, + pub const ENTRY_ALREADY_EXISTED: Self = Self::new_const(ll::lfs_error_LFS_ERR_EXIST); + /// Path name is not a directory. - PathNotDir, + pub const PATH_NOT_DIR: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOTDIR); + /// Path specification is to a directory. - PathIsDir, + pub const PATH_IS_DIR: Self = Self::new_const(ll::lfs_error_LFS_ERR_ISDIR); + /// Directory was not empty. - DirNotEmpty, + pub const DIR_NOT_EMPTY: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOTEMPTY); + /// Bad file descriptor. - BadFileDescriptor, + pub const BAD_FILE_DESCRIPTOR: Self = Self::new_const(ll::lfs_error_LFS_ERR_BADF); + /// File is too big. - FileTooBig, + pub const FILE_TOO_BIG: Self = Self::new_const(ll::lfs_error_LFS_ERR_FBIG); + /// Incorrect value specified to function. - Invalid, + pub const INVALID: Self = Self::new_const(ll::lfs_error_LFS_ERR_INVAL); + /// No space left available for operation. - NoSpace, + pub const NO_SPACE: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOSPC); + /// No memory available for completing request. - NoMemory, + pub const NO_MEMORY: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOMEM); + /// No attribute or data available - NoAttribute, + pub const NO_ATTRIBUTE: Self = Self::new_const(ll::lfs_error_LFS_ERR_NOATTR); + /// Filename too long - FilenameTooLong, - /// Unknown error occurred, integer code specified. - Unknown(i32), -} + pub const FILENAME_TOO_LONG: Self = Self::new_const(ll::lfs_error_LFS_ERR_NAMETOOLONG); -impl From for Error { - fn from(_error: crate::path::Error) -> Self { - Error::Io + /// Construct an `Error` from an error code. + /// + /// Return values that are greater or equals to zero represent success. In this case, `None` + /// is returned. + pub const fn new(code: c_int) -> Option { + if code >= 0 { + None + } else { + Some(Self { code }) + } } -} -impl From for ll::lfs_error { - fn from(error: Error) -> Self { - match error { - Error::Success => ll::lfs_error_LFS_ERR_OK, - Error::Io => ll::lfs_error_LFS_ERR_IO, - Error::Corruption => ll::lfs_error_LFS_ERR_CORRUPT, - Error::NoSuchEntry => ll::lfs_error_LFS_ERR_NOENT, - Error::EntryAlreadyExisted => ll::lfs_error_LFS_ERR_EXIST, - Error::PathNotDir => ll::lfs_error_LFS_ERR_NOTDIR, - Error::PathIsDir => ll::lfs_error_LFS_ERR_ISDIR, - Error::DirNotEmpty => ll::lfs_error_LFS_ERR_NOTEMPTY, - Error::BadFileDescriptor => ll::lfs_error_LFS_ERR_BADF, - Error::FileTooBig => ll::lfs_error_LFS_ERR_FBIG, - Error::Invalid => ll::lfs_error_LFS_ERR_INVAL, - Error::NoSpace => ll::lfs_error_LFS_ERR_NOSPC, - Error::NoMemory => ll::lfs_error_LFS_ERR_NOMEM, - Error::NoAttribute => ll::lfs_error_LFS_ERR_NOATTR, - Error::FilenameTooLong => ll::lfs_error_LFS_ERR_NAMETOOLONG, - Error::Unknown(error_code) => error_code, + const fn new_const(code: c_int) -> Self { + if code >= 0 { + panic!("error code must be negative"); } + Self { code } + } + + /// Return the error code of this error. + pub const fn code(&self) -> c_int { + self.code + } + + const fn kind(&self) -> Option<&'static str> { + Some(match *self { + Self::IO => "Io", + Self::CORRUPTION => "Corruption", + Self::NO_SUCH_ENTRY => "NoSuchEntry", + Self::ENTRY_ALREADY_EXISTED => "EntryAlreadyExisted", + Self::PATH_NOT_DIR => "PathNotDir", + Self::PATH_IS_DIR => "PathIsDir", + Self::DIR_NOT_EMPTY => "DirNotEmpty", + Self::BAD_FILE_DESCRIPTOR => "BadFileDescriptor", + Self::FILE_TOO_BIG => "FileTooBig", + Self::INVALID => "Invalid", + Self::NO_SPACE => "NoSpace", + Self::NO_MEMORY => "NoMemory", + Self::NO_ATTRIBUTE => "NoAttribute", + Self::FILENAME_TOO_LONG => "FilenameTooLong", + _ => { + return None; + } + }) } } -impl From for Error { - fn from(error_code: i32) -> Error { - match error_code { - n if n >= 0 => Error::Success, - // negative codes - ll::lfs_error_LFS_ERR_IO => Error::Io, - ll::lfs_error_LFS_ERR_CORRUPT => Error::Corruption, - ll::lfs_error_LFS_ERR_NOENT => Error::NoSuchEntry, - ll::lfs_error_LFS_ERR_EXIST => Error::EntryAlreadyExisted, - ll::lfs_error_LFS_ERR_NOTDIR => Error::PathNotDir, - ll::lfs_error_LFS_ERR_ISDIR => Error::PathIsDir, - ll::lfs_error_LFS_ERR_NOTEMPTY => Error::DirNotEmpty, - ll::lfs_error_LFS_ERR_BADF => Error::BadFileDescriptor, - ll::lfs_error_LFS_ERR_FBIG => Error::FileTooBig, - ll::lfs_error_LFS_ERR_INVAL => Error::Invalid, - ll::lfs_error_LFS_ERR_NOSPC => Error::NoSpace, - ll::lfs_error_LFS_ERR_NOMEM => Error::NoMemory, - ll::lfs_error_LFS_ERR_NOATTR => Error::NoAttribute, - ll::lfs_error_LFS_ERR_NAMETOOLONG => Error::FilenameTooLong, - // positive codes should always indicate success - _ => Error::Unknown(error_code), - } +/// Prints the numeric error code and the name of the error (if known). +/// +/// ``` +/// # use littlefs2::io::Error; +/// +/// assert_eq!( +/// &format!("{:?}", Error::IO), +/// "Error { code: -5, kind: Some(\"Io\") }", +/// ); +/// assert_eq!( +/// &format!("{:?}", Error::new(-128).unwrap()), +/// "Error { code: -128, kind: None }", +/// ); +/// ``` +impl Debug for Error { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + // add pseudo-field kind to debug output to make errors easier to read + f.debug_struct("Error") + .field("code", &self.code) + .field("kind", &self.kind()) + .finish() + } +} + +impl From for c_int { + fn from(error: Error) -> Self { + error.code } } pub fn error_code_from(result: Result) -> ll::lfs_error { - result.err().unwrap_or(Error::Success).into() + result + .map(|_| ll::lfs_error_LFS_ERR_OK) + .unwrap_or_else(From::from) } pub fn result_from(return_value: T, error_code: ll::lfs_error) -> Result { - let error: Error = error_code.into(); - match error { - Error::Success => Ok(return_value), - _ => Err(error), + if let Some(error) = Error::new(error_code) { + Err(error) + } else { + Ok(return_value) } } diff --git a/src/object_safe.rs b/src/object_safe.rs index 625001f34..c50fc4cc5 100644 --- a/src/object_safe.rs +++ b/src/object_safe.rs @@ -243,7 +243,7 @@ impl dyn DynFilesystem + '_ { } pub fn create_file_and_then(&self, path: &Path, f: FileCallback<'_, R>) -> Result { - let mut result = Err(Error::Io); + let mut result = Err(Error::IO); self.create_file_and_then_unit(path, &mut |file| { result = Ok(f(file)?); Ok(()) @@ -252,7 +252,7 @@ impl dyn DynFilesystem + '_ { } pub fn open_file_and_then(&self, path: &Path, f: FileCallback<'_, R>) -> Result { - let mut result = Err(Error::Io); + let mut result = Err(Error::IO); self.open_file_and_then_unit(path, &mut |file| { result = Ok(f(file)?); Ok(()) @@ -266,7 +266,7 @@ impl dyn DynFilesystem + '_ { path: &Path, f: FileCallback<'_, R>, ) -> Result { - let mut result = Err(Error::Io); + let mut result = Err(Error::IO); self.open_file_with_options_and_then_unit(o, path, &mut |file| { result = Ok(f(file)?); Ok(()) @@ -275,7 +275,7 @@ impl dyn DynFilesystem + '_ { } pub fn read_dir_and_then(&self, path: &Path, f: DirEntriesCallback<'_, R>) -> Result { - let mut result = Err(Error::Io); + let mut result = Err(Error::IO); self.read_dir_and_then_unit(path, &mut |entries| { result = Ok(f(entries)?); Ok(()) @@ -372,7 +372,7 @@ impl DynStorage for S { impl dyn DynStorage + '_ { pub fn mount_and_then(&mut self, f: FilesystemCallback<'_, R>) -> Result { - let mut result = Err(Error::Io); + let mut result = Err(Error::IO); self.mount_and_then_unit(&mut |fs| { result = Ok(f(fs)?); Ok(()) diff --git a/src/tests.rs b/src/tests.rs index 1e32e5fcf..6feb1e0dc 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -56,7 +56,7 @@ fn format() { Filesystem::mount(&mut alloc, &mut storage) .map(drop) .unwrap_err(), - Error::Corruption + Error::CORRUPTION ); // should succeed assert!(Filesystem::format(&mut storage).is_ok()); @@ -200,7 +200,7 @@ fn test_create() { File::open_and_then(fs, b"/test_open.txt\0".try_into().unwrap(), |_| { Ok(()) }) .map(drop) .unwrap_err(), // "real" contains_err is experimental - Error::NoSuchEntry + Error::NO_SUCH_ENTRY ); assert!(!path!("/test_open.txt").exists(fs)); @@ -224,7 +224,7 @@ fn test_create() { // // cannot remove non-empty directories assert_eq!( fs.remove(b"/tmp\0".try_into().unwrap()).unwrap_err(), - Error::DirNotEmpty + Error::DIR_NOT_EMPTY ); let metadata = fs.metadata(b"/tmp\0".try_into().unwrap())?; @@ -414,10 +414,10 @@ fn remove_dir_all_where() { }) .unwrap(); assert!(fs.metadata(path!("test_dir/test_file")).unwrap().is_file()); - assert_eq!(fs.metadata(path!("test_file")), Err(Error::NoSuchEntry)); + assert_eq!(fs.metadata(path!("test_file")), Err(Error::NO_SUCH_ENTRY)); assert_eq!( fs.metadata(path!("test_dir/test_file2")), - Err(Error::NoSuchEntry) + Err(Error::NO_SUCH_ENTRY) ); Ok(()) }) @@ -474,15 +474,15 @@ fn test_iter_dirs() { let mut storage = RamStorage::new(&mut backend); Filesystem::format(&mut storage).unwrap(); Filesystem::mount_and_then(&mut storage, |fs| { - fs.create_dir(b"/tmp\0".try_into()?)?; + fs.create_dir(path!("/tmp").into())?; // TODO: we might want "multi-open" - fs.create_file_and_then(b"/tmp/file.a\0".try_into()?, |file| { + fs.create_file_and_then(path!("/tmp/file.a"), |file| { file.set_len(37)?; - fs.create_file_and_then(b"/tmp/file.b\0".try_into()?, |file| file.set_len(42)) + fs.create_file_and_then(path!("/tmp/file.b"), |file| file.set_len(42)) })?; - fs.read_dir_and_then(b"/tmp\0".try_into()?, |dir| { + fs.read_dir_and_then(path!("/tmp"), |dir| { let mut found_files: usize = 0; let mut sizes = [0usize; 4];