Skip to content

Commit

Permalink
Do not panic on errors during drop, add optional manual closing to no…
Browse files Browse the repository at this point in the history
…n-raw
  • Loading branch information
peterkrull committed Mar 29, 2024
1 parent 55c77ce commit 86c20e0
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 53 deletions.
23 changes: 17 additions & 6 deletions src/filesystem/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<D::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
Expand All @@ -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)
}
}

Expand Down
23 changes: 17 additions & 6 deletions src/filesystem/files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -128,6 +131,16 @@ where
pub fn flush(&mut self) -> Result<(), Error<D::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<D::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
Expand All @@ -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);
}
}

Expand Down
23 changes: 17 additions & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<D::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
Expand All @@ -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)
}
}

Expand Down
60 changes: 25 additions & 35 deletions src/volume_mgr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -775,14 +775,36 @@ where

/// Close a file with the given raw file handle.
pub fn close_file(&mut self, file: RawFile) -> Result<(), Error<D::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<D::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.
Expand Down Expand Up @@ -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<usize, Error<D::Error>> {
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
Expand Down

0 comments on commit 86c20e0

Please sign in to comment.