From 072d957ee552cf54b26f4dcfc5ef4f1c8ada92fb Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 6 May 2023 13:42:04 +0200 Subject: [PATCH 1/3] move file_system.rs to dedicated sub module --- uefi/src/fs/{file_system.rs => file_system/fs.rs} | 2 +- uefi/src/fs/file_system/mod.rs | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) rename uefi/src/fs/{file_system.rs => file_system/fs.rs} (99%) create mode 100644 uefi/src/fs/file_system/mod.rs diff --git a/uefi/src/fs/file_system.rs b/uefi/src/fs/file_system/fs.rs similarity index 99% rename from uefi/src/fs/file_system.rs rename to uefi/src/fs/file_system/fs.rs index 41dd78c15..2bb0db497 100644 --- a/uefi/src/fs/file_system.rs +++ b/uefi/src/fs/file_system/fs.rs @@ -1,6 +1,6 @@ //! Module for [`FileSystem`]. -use super::*; +use super::super::*; use crate::fs::path::{validate_path, PathError}; use crate::proto::media::file::{FileAttribute, FileInfo, FileType}; use crate::table::boot::ScopedProtocol; diff --git a/uefi/src/fs/file_system/mod.rs b/uefi/src/fs/file_system/mod.rs new file mode 100644 index 000000000..27b83bcd2 --- /dev/null +++ b/uefi/src/fs/file_system/mod.rs @@ -0,0 +1,3 @@ +mod fs; + +pub use fs::*; From 152b5d2f5bbf6c5a6f56de5d2cf0c47132c5027d Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 6 May 2023 13:31:08 +0200 Subject: [PATCH 2/3] error: add convenient factory method to_err_without_payload --- uefi/src/result/error.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/uefi/src/result/error.rs b/uefi/src/result/error.rs index 8a88cc1b0..00b478f7c 100644 --- a/uefi/src/result/error.rs +++ b/uefi/src/result/error.rs @@ -54,5 +54,19 @@ impl Display for Error { } } +impl Error { + /// Transforms the generic payload of an error to `()`. This is useful if + /// you want + /// - to retain the erroneous status code, + /// - do not care about the payload, and + /// - refrain from generic type complexity in a higher API level. + pub fn to_err_without_payload(&self) -> Error<()> { + Error { + status: self.status, + data: (), + } + } +} + #[cfg(feature = "unstable")] impl core::error::Error for Error {} From ee593a7babea50c60c9d1e301b4b2657983e8598 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 6 May 2023 13:47:30 +0200 Subject: [PATCH 3/3] fs: add new Error type + naming adjustments --- uefi-test-runner/src/fs/mod.rs | 17 ++- uefi/src/fs/file_system/error.rs | 82 +++++++++++++ uefi/src/fs/file_system/fs.rs | 202 +++++++++++++++---------------- uefi/src/fs/file_system/mod.rs | 2 + uefi/src/fs/mod.rs | 2 +- uefi/src/fs/path/mod.rs | 4 +- uefi/src/fs/path/path.rs | 3 +- uefi/src/fs/path/pathbuf.rs | 2 +- 8 files changed, 195 insertions(+), 119 deletions(-) create mode 100644 uefi/src/fs/file_system/error.rs diff --git a/uefi-test-runner/src/fs/mod.rs b/uefi-test-runner/src/fs/mod.rs index daf895e32..cb47146e8 100644 --- a/uefi-test-runner/src/fs/mod.rs +++ b/uefi-test-runner/src/fs/mod.rs @@ -2,14 +2,14 @@ use alloc::string::{String, ToString}; use alloc::vec::Vec; -use uefi::cstr16; -use uefi::fs::{FileSystem, FileSystemError, PathBuf}; +use uefi::fs::{FileSystem, FileSystemIOErrorContext, IoError, PathBuf}; use uefi::proto::media::fs::SimpleFileSystem; use uefi::table::boot::ScopedProtocol; +use uefi::{cstr16, fs, Status}; /// Tests functionality from the `uefi::fs` module. This test relies on a /// working File System Protocol, which is tested at a dedicated place. -pub fn test(sfs: ScopedProtocol) -> Result<(), FileSystemError> { +pub fn test(sfs: ScopedProtocol) -> Result<(), fs::Error> { let mut fs = FileSystem::new(sfs); // test create dir @@ -24,9 +24,14 @@ pub fn test(sfs: ScopedProtocol) -> Result<(), FileSystemError let read = String::from_utf8(read).expect("Should be valid utf8"); assert_eq!(read.as_str(), data_to_write); - // test copy from non-existent file + // test copy from non-existent file: does the error type work as expected? let err = fs.copy(cstr16!("not_found"), cstr16!("abc")); - assert!(matches!(err, Err(FileSystemError::OpenError { .. }))); + let expected_err = fs::Error::Io(IoError { + path: PathBuf::from(cstr16!("not_found")), + context: FileSystemIOErrorContext::OpenError, + uefi_error: uefi::Error::new(Status::NOT_FOUND, ()), + }); + assert_eq!(err, Err(expected_err)); // test rename file + path buf replaces / with \ fs.rename( @@ -35,7 +40,7 @@ pub fn test(sfs: ScopedProtocol) -> Result<(), FileSystemError )?; // file should not be available after rename let err = fs.read(cstr16!("foo_dir\\foo_cpy")); - assert!(matches!(err, Err(FileSystemError::OpenError { .. }))); + assert!(err.is_err()); // test read dir on a sub dir let entries = fs diff --git a/uefi/src/fs/file_system/error.rs b/uefi/src/fs/file_system/error.rs new file mode 100644 index 000000000..f25086539 --- /dev/null +++ b/uefi/src/fs/file_system/error.rs @@ -0,0 +1,82 @@ +use crate::fs::{PathBuf, PathError}; +use alloc::string::FromUtf8Error; +use core::fmt::Debug; +use derive_more::Display; + +/// All errors that can happen when working with the [`FileSystem`]. +/// +/// [`FileSystem`]: super::FileSystem +#[derive(Debug, Clone, Display, PartialEq, Eq)] +pub enum Error { + /// IO (low-level UEFI-errors) errors. See [`IoError`]. + Io(IoError), + /// Path-related errors. See [`PathError`]. + Path(PathError), + /// Can't parse file content as UTF-8. See [`FromUtf8Error`]. + Utf8Encoding(FromUtf8Error), +} + +/// UEFI-error with context when working with the underlying UEFI file protocol. +#[derive(Debug, Clone, Display, PartialEq, Eq)] +#[display(fmt = "IoError({},{})", context, path)] +pub struct IoError { + /// The path that led to the error. + pub path: PathBuf, + /// The context in which the path was used. + pub context: FileSystemIOErrorContext, + /// The underlying UEFI error. + pub uefi_error: crate::Error, +} + +/// Enum that further specifies the context in that a [`Error`] +/// occurred. +#[derive(Debug, Clone, Display, PartialEq, Eq)] +pub enum FileSystemIOErrorContext { + /// Can't delete the directory. + CantDeleteDirectory, + /// Can't delete the file. + CantDeleteFile, + /// Error flushing file. + FlushFailure, + /// Can't open the root directory of the underlying volume. + CantOpenVolume, + /// Error while reading the metadata of the file. + Metadata, + /// Could not open the given path. One possible reason is that the file does + /// not exist. + OpenError, + /// Error reading file. + ReadFailure, + /// Error writing bytes. + WriteFailure, + /// The path exists but does not correspond to a directory when a directory + /// was expected. + NotADirectory, + /// The path exists but does not correspond to a file when a file was + /// expected. + NotAFile, +} + +impl From for Error { + fn from(value: PathError) -> Self { + Self::Path(value) + } +} + +#[cfg(feature = "unstable")] +impl core::error::Error for Error { + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { + match self { + Error::Io(err) => Some(err), + Error::Path(err) => Some(err), + Error::Utf8Encoding(err) => Some(err), + } + } +} + +#[cfg(feature = "unstable")] +impl core::error::Error for IoError { + fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { + Some(&self.uefi_error) + } +} diff --git a/uefi/src/fs/file_system/fs.rs b/uefi/src/fs/file_system/fs.rs index 2bb0db497..c729fd725 100644 --- a/uefi/src/fs/file_system/fs.rs +++ b/uefi/src/fs/file_system/fs.rs @@ -1,79 +1,19 @@ //! Module for [`FileSystem`]. -use super::super::*; -use crate::fs::path::{validate_path, PathError}; -use crate::proto::media::file::{FileAttribute, FileInfo, FileType}; +use crate::fs::{Path, PathBuf, UefiDirectoryIter, SEPARATOR_STR, *}; use crate::table::boot::ScopedProtocol; +use crate::Status; use alloc::boxed::Box; -use alloc::string::{FromUtf8Error, String, ToString}; +use alloc::string::String; use alloc::vec; use alloc::vec::Vec; use core::fmt; use core::fmt::{Debug, Formatter}; use core::ops::Deref; -use derive_more::Display; use log::debug; -/// All errors that can happen when working with the [`FileSystem`]. -#[derive(Debug, Clone, Display, PartialEq, Eq)] -pub enum FileSystemError { - /// Can't open the root directory of the underlying volume. - CantOpenVolume, - /// The path is invalid because of the underlying [`PathError`]. - /// - /// [`PathError`]: path::PathError - IllegalPath(PathError), - /// The file or directory was not found in the underlying volume. - FileNotFound(String), - /// The path is existent but does not correspond to a directory when a - /// directory was expected. - NotADirectory(String), - /// The path is existent but does not correspond to a file when a file was - /// expected. - NotAFile(String), - /// Can't delete the file. - CantDeleteFile(String), - /// Can't delete the directory. - CantDeleteDirectory(String), - /// Error writing bytes. - WriteFailure, - /// Error flushing file. - FlushFailure, - /// Error reading file. - ReadFailure, - /// Can't parse file content as UTF-8. - Utf8Error(FromUtf8Error), - /// Could not open the given path. Carries the path that could not be opened - /// and the underlying UEFI error. - #[display(fmt = "{path:?}")] - OpenError { - /// Path that caused the failure. - path: String, - /// More detailed failure description. - error: crate::Error, - }, -} - -#[cfg(feature = "unstable")] -impl core::error::Error for FileSystemError { - fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { - match self { - FileSystemError::IllegalPath(e) => Some(e), - FileSystemError::Utf8Error(e) => Some(e), - FileSystemError::OpenError { path: _path, error } => Some(error), - _ => None, - } - } -} - -impl From for FileSystemError { - fn from(err: PathError) -> Self { - Self::IllegalPath(err) - } -} - /// Return type for public [`FileSystem`] operations. -pub type FileSystemResult = Result; +pub type FileSystemResult = Result; /// High-level file-system abstraction for UEFI volumes with an API that is /// close to `std::fs`. It acts as convenient accessor around the @@ -143,11 +83,11 @@ impl<'a> FileSystem<'a> { let path = path.as_ref(); let mut file = self.open(path, UefiFileMode::Read, false)?; file.get_boxed_info().map_err(|err| { - log::trace!("failed to fetch file info: {err:#?}"); - FileSystemError::OpenError { - path: path.to_cstr16().to_string(), - error: err, - } + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::Metadata, + uefi_error: err, + }) }) } @@ -158,18 +98,29 @@ impl<'a> FileSystem<'a> { let mut file = self .open(path, UefiFileMode::Read, false)? .into_regular_file() - .ok_or(FileSystemError::NotAFile(path.to_cstr16().to_string()))?; - let info = file - .get_boxed_info::() - .map_err(|err| FileSystemError::OpenError { - path: path.to_cstr16().to_string(), - error: err, - })?; + .ok_or(Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::NotAFile, + // We do not have a real UEFI error here as we have a logical + // problem. + uefi_error: Status::INVALID_PARAMETER.into(), + }))?; + + let info = file.get_boxed_info::().map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::Metadata, + uefi_error: err, + }) + })?; let mut vec = vec![0; info.file_size() as usize]; - let read_bytes = file.read(vec.as_mut_slice()).map_err(|e| { - log::error!("reading failed: {e:?}"); - FileSystemError::ReadFailure + let read_bytes = file.read(vec.as_mut_slice()).map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::ReadFailure, + uefi_error: err.to_err_without_payload(), + }) })?; // we read the whole file at once! @@ -186,13 +137,19 @@ impl<'a> FileSystem<'a> { let dir = self .open(path, UefiFileMode::Read, false)? .into_directory() - .ok_or(FileSystemError::NotADirectory(path.to_cstr16().to_string()))?; + .ok_or(Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::NotADirectory, + // We do not have a real UEFI error here as we have a logical + // problem. + uefi_error: Status::INVALID_PARAMETER.into(), + }))?; Ok(UefiDirectoryIter::new(dir)) } - /// Read the entire contents of a file into a string. + /// Read the entire contents of a file into a Rust string. pub fn read_to_string(&mut self, path: impl AsRef) -> FileSystemResult { - String::from_utf8(self.read(path)?).map_err(FileSystemError::Utf8Error) + String::from_utf8(self.read(path)?).map_err(Error::Utf8Encoding) } /// Removes an empty directory. @@ -205,12 +162,21 @@ impl<'a> FileSystem<'a> { .unwrap(); match file { - FileType::Dir(dir) => dir.delete().map_err(|e| { - log::error!("error removing dir: {e:?}"); - FileSystemError::CantDeleteDirectory(path.to_cstr16().to_string()) + UefiFileType::Dir(dir) => dir.delete().map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::CantDeleteDirectory, + uefi_error: err, + }) }), - FileType::Regular(_) => { - Err(FileSystemError::NotADirectory(path.to_cstr16().to_string())) + UefiFileType::Regular(_) => { + Err(Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::NotADirectory, + // We do not have a real UEFI error here as we have a logical + // problem. + uefi_error: Status::INVALID_PARAMETER.into(), + })) } } } @@ -231,11 +197,20 @@ impl<'a> FileSystem<'a> { .unwrap(); match file { - FileType::Regular(file) => file.delete().map_err(|e| { - log::error!("error removing file: {e:?}"); - FileSystemError::CantDeleteFile(path.to_cstr16().to_string()) + UefiFileType::Regular(file) => file.delete().map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::CantDeleteFile, + uefi_error: err, + }) }), - FileType::Dir(_) => Err(FileSystemError::NotAFile(path.to_cstr16().to_string())), + UefiFileType::Dir(_) => Err(Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::NotAFile, + // We do not have a real UEFI error here as we have a logical + // problem. + uefi_error: Status::INVALID_PARAMETER.into(), + })), } } @@ -271,22 +246,35 @@ impl<'a> FileSystem<'a> { .into_regular_file() .unwrap(); - handle.write(content.as_ref()).map_err(|e| { - log::error!("only wrote {e:?} bytes"); - FileSystemError::WriteFailure + handle.write(content.as_ref()).map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::WriteFailure, + uefi_error: err.to_err_without_payload(), + }) })?; - handle.flush().map_err(|e| { - log::error!("flush failure: {e:?}"); - FileSystemError::FlushFailure + handle.flush().map_err(|err| { + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::FlushFailure, + uefi_error: err, + }) })?; Ok(()) } /// Opens a fresh handle to the root directory of the volume. fn open_root(&mut self) -> FileSystemResult { - self.0.open_volume().map_err(|e| { - log::error!("Can't open root volume: {e:?}"); - FileSystemError::CantOpenVolume + self.0.open_volume().map_err(|err| { + Error::Io(IoError { + path: { + let mut path = PathBuf::new(); + path.push(SEPARATOR_STR); + path + }, + context: FileSystemIOErrorContext::CantOpenVolume, + uefi_error: err, + }) }) } @@ -303,22 +291,22 @@ impl<'a> FileSystem<'a> { is_dir: bool, ) -> FileSystemResult { validate_path(path)?; - log::trace!("open validated path: {path}"); let attr = if mode == UefiFileMode::CreateReadWrite && is_dir { - FileAttribute::DIRECTORY + UefiFileAttribute::DIRECTORY } else { - FileAttribute::empty() + UefiFileAttribute::empty() }; self.open_root()? .open(path.to_cstr16(), mode, attr) .map_err(|err| { log::trace!("Can't open file {path}: {err:?}"); - FileSystemError::OpenError { - path: path.to_cstr16().to_string(), - error: err, - } + Error::Io(IoError { + path: path.to_path_buf(), + context: FileSystemIOErrorContext::OpenError, + uefi_error: err, + }) }) } } diff --git a/uefi/src/fs/file_system/mod.rs b/uefi/src/fs/file_system/mod.rs index 27b83bcd2..3a2b9738e 100644 --- a/uefi/src/fs/file_system/mod.rs +++ b/uefi/src/fs/file_system/mod.rs @@ -1,3 +1,5 @@ +mod error; mod fs; +pub use error::*; pub use fs::*; diff --git a/uefi/src/fs/mod.rs b/uefi/src/fs/mod.rs index 18253ebce..a896e0d5e 100644 --- a/uefi/src/fs/mod.rs +++ b/uefi/src/fs/mod.rs @@ -35,7 +35,7 @@ mod file_system; mod path; mod uefi_types; -pub use file_system::{FileSystem, FileSystemError, FileSystemResult}; +pub use file_system::*; pub use path::*; use dir_entry_iter::*; diff --git a/uefi/src/fs/path/mod.rs b/uefi/src/fs/path/mod.rs index 8fc0ba7e2..35293e0ae 100644 --- a/uefi/src/fs/path/mod.rs +++ b/uefi/src/fs/path/mod.rs @@ -20,8 +20,8 @@ mod validation; pub use path::{Components, Path}; pub use pathbuf::PathBuf; -use uefi::data_types::chars::NUL_16; -use uefi::{CStr16, Char16}; +use crate::data_types::chars::NUL_16; +use crate::{CStr16, Char16}; pub(super) use validation::validate_path; pub use validation::PathError; diff --git a/uefi/src/fs/path/path.rs b/uefi/src/fs/path/path.rs index bf6af9886..122ea4965 100644 --- a/uefi/src/fs/path/path.rs +++ b/uefi/src/fs/path/path.rs @@ -2,9 +2,8 @@ #![allow(clippy::module_inception)] use crate::fs::path::{PathBuf, SEPARATOR}; -use crate::CStr16; +use crate::{CStr16, CString16}; use core::fmt::{Display, Formatter}; -use uefi::CString16; /// A path similar to the `Path` of the standard library, but based on /// [`CStr16`] strings and [`SEPARATOR`] as separator. diff --git a/uefi/src/fs/path/pathbuf.rs b/uefi/src/fs/path/pathbuf.rs index e018e3af0..222121f9b 100644 --- a/uefi/src/fs/path/pathbuf.rs +++ b/uefi/src/fs/path/pathbuf.rs @@ -7,7 +7,7 @@ use core::fmt::{Display, Formatter}; /// [`CString16`] strings and [`SEPARATOR`] as separator. /// /// `/` is replaced by [`SEPARATOR`] on the fly. -#[derive(Debug, Default, Eq, PartialOrd, Ord)] +#[derive(Clone, Debug, Default, Eq, PartialOrd, Ord)] pub struct PathBuf(CString16); impl PathBuf {