From 86c20e042385428c299b3eaf6f703293245e9dc1 Mon Sep 17 00:00:00 2001 From: Peter Krull Date: Fri, 29 Mar 2024 19:22:15 +0100 Subject: [PATCH] Do not panic on errors during drop, add optional manual closing to non-raw --- src/filesystem/directory.rs | 23 ++++++++++---- src/filesystem/files.rs | 23 ++++++++++---- src/lib.rs | 23 ++++++++++---- src/volume_mgr.rs | 60 ++++++++++++++++--------------------- 4 files changed, 76 insertions(+), 53 deletions(-) diff --git a/src/filesystem/directory.rs b/src/filesystem/directory.rs index f0a7bb0..0ed53e8 100644 --- a/src/filesystem/directory.rs +++ b/src/filesystem/directory.rs @@ -72,9 +72,12 @@ impl RawDirectory { /// Represents an open directory on disk. /// -/// If you drop a value of this type, it closes the directory automatically. However, -/// it holds a mutable reference to its parent `VolumeManager`, which restricts -/// which operations you can perform. +/// In contrast to a `RawDirectory`, a `Directory` holds a mutable reference to +/// its parent `VolumeManager`, which restricts which operations you can perform. +/// +/// If you drop a value of this type, it closes the directory automatically, but +/// any error that may occur will be ignored. To handle potential errors, use +/// the [`Directory::close`] method. pub struct Directory< 'a, D, @@ -188,6 +191,16 @@ where core::mem::forget(self); d } + + /// Consume the `Directory` handle and close it. The behavior of this is similar + /// to using [`core::mem::drop`] or letting the `Directory` go out of scope, + /// except this lets the user handle any errors that may occur in the process, + /// whereas when using drop, any errors will be discarded silently. + pub fn close(self) -> Result<(), Error> { + let result = self.volume_mgr.close_dir(self.raw_directory); + core::mem::forget(self); + result + } } impl<'a, D, T, const MAX_DIRS: usize, const MAX_FILES: usize, const MAX_VOLUMES: usize> Drop @@ -197,9 +210,7 @@ where T: crate::TimeSource, { fn drop(&mut self) { - self.volume_mgr - .close_dir(self.raw_directory) - .expect("Failed to close directory"); + _ = self.volume_mgr.close_dir(self.raw_directory) } } diff --git a/src/filesystem/files.rs b/src/filesystem/files.rs index a06b2e7..08a50e8 100644 --- a/src/filesystem/files.rs +++ b/src/filesystem/files.rs @@ -39,9 +39,12 @@ impl RawFile { /// Represents an open file on disk. /// -/// If you drop a value of this type, it closes the file automatically. However, -/// it holds a mutable reference to its parent `VolumeManager`, which restricts -/// which operations you can perform. +/// In contrast to a `RawFile`, a `File` holds a mutable reference to its +/// parent `VolumeManager`, which restricts which operations you can perform. +/// +/// If you drop a value of this type, it closes the file automatically, and but +/// error that may occur will be ignored. To handle potential errors, use +/// the [`File::close`] method. pub struct File<'a, D, T, const MAX_DIRS: usize, const MAX_FILES: usize, const MAX_VOLUMES: usize> where D: crate::BlockDevice, @@ -128,6 +131,16 @@ where pub fn flush(&mut self) -> Result<(), Error> { self.volume_mgr.flush_file(self.raw_file) } + + /// Consume the `File` handle and close it. The behavior of this is similar + /// to using [`core::mem::drop`] or letting the `File` go out of scope, + /// except this lets the user handle any errors that may occur in the process, + /// whereas when using drop, any errors will be discarded silently. + pub fn close(self) -> Result<(), Error> { + let result = self.volume_mgr.close_file(self.raw_file); + core::mem::forget(self); + result + } } impl<'a, D, T, const MAX_DIRS: usize, const MAX_FILES: usize, const MAX_VOLUMES: usize> Drop @@ -137,9 +150,7 @@ where T: crate::TimeSource, { fn drop(&mut self) { - self.volume_mgr - .close_file(self.raw_file) - .expect("Failed to close file"); + _ = self.volume_mgr.close_file(self.raw_file); } } diff --git a/src/lib.rs b/src/lib.rs index ebf9a4d..1bcd429 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -239,9 +239,12 @@ impl RawVolume { /// Represents an open volume on disk. /// -/// If you drop a value of this type, it closes the volume automatically. However, -/// it holds a mutable reference to its parent `VolumeManager`, which restricts -/// which operations you can perform. +/// In contrast to a `RawVolume`, a `Volume` holds a mutable reference to its +/// parent `VolumeManager`, which restricts which operations you can perform. +/// +/// If you drop a value of this type, it closes the volume automatically, but +/// any error that may occur will be ignored. To handle potential errors, use +/// the [`Volume::close`] method. pub struct Volume<'a, D, T, const MAX_DIRS: usize, const MAX_FILES: usize, const MAX_VOLUMES: usize> where D: crate::BlockDevice, @@ -285,6 +288,16 @@ where core::mem::forget(self); v } + + /// Consume the `Volume` handle and close it. The behavior of this is similar + /// to using [`core::mem::drop`] or letting the `Volume` go out of scope, + /// except this lets the user handle any errors that may occur in the process, + /// whereas when using drop, any errors will be discarded silently. + pub fn close(self) -> Result<(), Error> { + let result = self.volume_mgr.close_volume(self.raw_volume); + core::mem::forget(self); + result + } } impl<'a, D, T, const MAX_DIRS: usize, const MAX_FILES: usize, const MAX_VOLUMES: usize> Drop @@ -294,9 +307,7 @@ where T: crate::TimeSource, { fn drop(&mut self) { - self.volume_mgr - .close_volume(self.raw_volume) - .expect("Failed to close volume"); + _ = self.volume_mgr.close_volume(self.raw_volume) } } diff --git a/src/volume_mgr.rs b/src/volume_mgr.rs index 43c2dcb..b5f3940 100644 --- a/src/volume_mgr.rs +++ b/src/volume_mgr.rs @@ -775,14 +775,36 @@ where /// Close a file with the given raw file handle. pub fn close_file(&mut self, file: RawFile) -> Result<(), Error> { - let file_idx = self.flush_file_get_index(file)?; + let flush_result = self.flush_file(file); + let file_idx = self.get_file_by_id(file)?; self.open_files.swap_remove(file_idx); - Ok(()) + flush_result } /// Flush (update the entry) for a file with the given raw file handle. pub fn flush_file(&mut self, file: RawFile) -> Result<(), Error> { - self.flush_file_get_index(file).map(|_| ()) + let file_info = self + .open_files + .iter() + .find(|info| info.file_id == file) + .ok_or(Error::BadHandle)?; + + if file_info.dirty { + let volume_idx = self.get_volume_by_id(file_info.volume_id)?; + match self.open_volumes[volume_idx].volume_type { + VolumeType::Fat(ref mut fat) => { + debug!("Updating FAT info sector"); + fat.update_info_sector(&self.block_device)?; + debug!("Updating dir entry {:?}", file_info.entry); + if file_info.entry.size != 0 { + // If you have a length, you must have a cluster + assert!(file_info.entry.cluster.0 != 0); + } + fat.write_entry_to_disk(&self.block_device, &file_info.entry)?; + } + }; + } + Ok(()) } /// Check if any files or folders are open. @@ -1054,38 +1076,6 @@ where let available = Block::LEN - block_offset; Ok((block_idx, block_offset, available)) } - - /// Flush (update the entry) for a file with the given raw file handle and - /// get its index. - fn flush_file_get_index(&mut self, file: RawFile) -> Result> { - let mut found_idx = None; - for (idx, info) in self.open_files.iter().enumerate() { - if file == info.file_id { - found_idx = Some((info, idx)); - break; - } - } - - let (file_info, file_idx) = found_idx.ok_or(Error::BadHandle)?; - - if file_info.dirty { - let volume_idx = self.get_volume_by_id(file_info.volume_id)?; - match self.open_volumes[volume_idx].volume_type { - VolumeType::Fat(ref mut fat) => { - debug!("Updating FAT info sector"); - fat.update_info_sector(&self.block_device)?; - debug!("Updating dir entry {:?}", file_info.entry); - if file_info.entry.size != 0 { - // If you have a length, you must have a cluster - assert!(file_info.entry.cluster.0 != 0); - } - fat.write_entry_to_disk(&self.block_device, &file_info.entry)?; - } - }; - } - - Ok(file_idx) - } } /// Transform mode variants (ReadWriteCreate_Or_Append) to simple modes ReadWriteAppend or