Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,10 @@ where
OpenedDirAsFile,
/// You can't open a file as a directory
OpenedFileAsDir,
/// You can't delete a directory as a file
/// You can't delete a directory as a file [no longer being emitted]
DeleteDirAsFile,
/// You can't delete a non-empty directory
DeleteNonEmptyDir,
/// You can't close a volume with open files or directories
VolumeStillInUse,
/// You can't open a volume twice
Expand Down Expand Up @@ -255,6 +257,7 @@ impl<E: Debug> embedded_io::Error for Error<E> {
Error::OpenedDirAsFile
| Error::OpenedFileAsDir
| Error::DeleteDirAsFile
| Error::DeleteNonEmptyDir
| Error::BadCluster
| Error::ConversionError
| Error::UnterminatedFatChain => ErrorKind::InvalidData,
Expand Down Expand Up @@ -294,6 +297,7 @@ where
Error::OpenedDirAsFile => write!(f, "cannot open directory as file"),
Error::OpenedFileAsDir => write!(f, "cannot open file as directory"),
Error::DeleteDirAsFile => write!(f, "cannot delete directory as file"),
Error::DeleteNonEmptyDir => write!(f, "cannot delete a non-empty directory"),
Error::VolumeStillInUse => write!(f, "volume is still in use"),
Error::VolumeAlreadyOpen => write!(f, "cannot open volume twice"),
Error::Unsupported => write!(f, "unsupported operation"),
Expand Down
56 changes: 47 additions & 9 deletions src/volume_mgr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,26 +665,64 @@ where
let data = data.deref_mut();

let dir_idx = data.get_dir_by_id(directory)?;
let dir_info = &data.open_dirs[dir_idx];
let volume_idx = data.get_volume_by_id(dir_info.raw_volume)?;
let parent_dir_info = data.open_dirs[dir_idx].clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall needing to clone an entry for an open directory before (unless I'm forgetting about it). What's the reason for the clone as opposed to borrowing the entry, or referring to it by index? This is probably fine, it just gave me pause as I read it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only needed if we want to allow deletion of currently open directories, because I'm creating a new DirectoryInfo in a block and making it a borrow would require using some of the lifetime extension tricks, while the struct looked tiny enough that cloning seemed cleaner to do.

let volume_idx = data.get_volume_by_id(parent_dir_info.raw_volume)?;
let sfn = name.to_short_filename().map_err(Error::FilenameError)?;

let dir_entry = match &data.open_volumes[volume_idx].volume_type {
VolumeType::Fat(fat) => fat.find_directory_entry(&mut data.block_cache, dir_info, &sfn),
VolumeType::Fat(fat) => {
fat.find_directory_entry(&mut data.block_cache, &parent_dir_info, &sfn)
}
}?;

if dir_entry.attributes.is_directory() {
return Err(Error::DeleteDirAsFile);
}

if data.file_is_open(dir_info.raw_volume, &dir_entry) {
// Find the directory to be deleted, so that we can check its contents.
let dir_info = if let Some(dir_info) = data
.open_dirs
.iter()
.find(|dir_info| {
// Subdirectory is already open.
dir_info.cluster == dir_entry.cluster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if the directory you are deleting is already open, you shouldn't be able to delete it because it would leave a dangling entry.

})
.cloned()
{
dir_info
} else {
// The subdirectory isn't yet open. Open it in order to be able to list it.
let raw_directory = RawDirectory(data.id_generator.generate());
DirectoryInfo {
raw_directory,
raw_volume: data.open_volumes[volume_idx].raw_volume,
cluster: dir_entry.cluster,
}
};
// Can only delete directories that are already empty.
let mut count = 0;
// Equivalent to `self.iterate_dir(raw_dir, |_| count += 1)?;`, without locking again.
match &data.open_volumes[volume_idx].volume_type {
VolumeType::Fat(fat) => {
fat.iterate_dir(&mut data.block_cache, &dir_info, |de| {
// Hide all the LFN directory entries
if !de.attributes.is_lfn()
&& de.name != ShortFileName::this_dir()
&& de.name != ShortFileName::parent_dir()
{
count += 1;
}
})?;
}
}
if count != 0 {
return Err(Error::DeleteNonEmptyDir);
}
} else if data.file_is_open(parent_dir_info.raw_volume, &dir_entry) {
return Err(Error::FileAlreadyOpen);
}

let volume_idx = data.get_volume_by_id(dir_info.raw_volume)?;
let volume_idx = data.get_volume_by_id(parent_dir_info.raw_volume)?;
match &data.open_volumes[volume_idx].volume_type {
VolumeType::Fat(fat) => {
fat.delete_directory_entry(&mut data.block_cache, dir_info, &sfn)?
fat.delete_directory_entry(&mut data.block_cache, &parent_dir_info, &sfn)?
}
}

Expand Down
Loading