Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rid into storage trait #74

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

unizippro
Copy link

This PR seeks to solve #54.
This is done by implementing Read, Write, Seek, IoBase for mutable references and removing the IntoStorage trait.
This enables one to give a mutable reference to Filesystem::new(..). Which can be freed up when an error occurs.

An example of recovering a corrupted filesystem can be seen in /tests/format.rs#L78.

@MathiasKoch
Copy link
Contributor

ping @rafalh

Copy link
Owner

@rafalh rafalh left a comment

Choose a reason for hiding this comment

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

I made some comments but I still want to wait until I experiment with other options that would not require users to use StdIoWrapper before merging it.

}

impl<IO: Read + Write + Seek, TP, OCC> FileSystem<IO, TP, OCC> {
// #[cfg(feature = "std")]
Copy link
Owner

Choose a reason for hiding this comment

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

to be removed?

@@ -807,7 +808,7 @@ impl LfnBuffer {
}

pub(crate) fn as_ucs2_units(&self) -> &[u16] {
&self.ucs2_units
&self.ucs2_units[..self.len]
Copy link
Owner

Choose a reason for hiding this comment

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

could you please rebase so those changes are not visible?

use fscommon::BufStream;

fn main() -> io::Result<()> {
let img_file = match OpenOptions::new().read(true).write(true).open("fat.img") {
let img_file = match OpenOptions::new().create(true).read(true).write(true).open("fat.img") {
Copy link
Owner

Choose a reason for hiding this comment

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

create(true) does not make sense for me here. You are not formatting the filesystem. If fat.img does not exist it will fail trying to read BPB.

@@ -24,7 +24,7 @@ fn format_file_size(size: u64) -> String {
fn main() -> io::Result<()> {
let file = File::open("resources/fat32.img")?;
let buf_rdr = BufStream::new(file);
let fs = FileSystem::new(buf_rdr, FsOptions::new())?;
let fs = FileSystem::new(StdIoWrapper::new(buf_rdr), FsOptions::new())?;
Copy link
Owner

Choose a reason for hiding this comment

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

This is exactly why I made IntoStorage trait - to make it possible to pass std::fs::File directly. Anyway why in some places you used into() and in some StdIoWrapper::new directly? IMHO into way is better

Choose a reason for hiding this comment

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

Could do something like pub fn new(disk: impl Into<IO>, ...) to keep making this work.

trace!("FileSystem::new");
debug_assert!(disk.seek(SeekFrom::Current(0))? == 0);
disk.seek(SeekFrom::Start(0))?;
Copy link
Owner

Choose a reason for hiding this comment

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

This is a change of behavior that requires changing the function comment.
My assertion was a sanity check that let people know that you can't open a disk image, seek to a partition and hope it will open a filesystem from on that partition. On the other hand it should fail anyway because in the first sector there will be MBR or GPT, so new behavior should be okay. I can see that it is useful for situation like formatting after error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants