Unable to recover from CorruptedFileSystem unless storage is Clone

[MathiasKoch]

The current implementation makes it impossible to recover from a CorruptedFileSystem error by formatting filesystem, unless the storage argument of Filesystem::new() implements Clone or similar.

let fs = match FileSystem::new(storage, FsOptions::new()) {
    Ok(fs) => fs,
    Err(fatfs::Error::CorruptedFileSystem) => {
        // `storage` moved, so it is impossible to call `fatfs::format_volume` here.
        todo!()
    }
    Err(_) => todo!(),
};

It is also impossible to fix it by implementing Read, Write & friends for &mut storage due to the current IntoStorage that will consume by self...

Another related issue is that storage is not returned in unmount, resulting in it being lost forever in e.g. an embedded system where storage directly correlates to a peripheral.

I have played around with one possible solution, that would be able to fix all of the above issues, but i am not sure if it is a direction you want to go? MathiasKoch@6ed236a

If so, i would love to finish it up and make a PR.

Currently it is missing the return of storage on unmounts, an Into implementation for StdIoWrapper and fixing the examples & tests.

[rafalh]

I added IntoStorage trait to simplify usage with std and keep compatibility with old version, so people don't have to wrap IO into StdIoWrapper. But I was never happy with that solution and was planning to remove it so I support this part of your PR. I was thinking about moving Read/Write/Seek traits that are used in no_std mode to fscommon trait and implementing them for BufStream type. This way no wrapping would be needed anymore when fatfs is used with BufStream. Do you think it's a good idea?
I feel uneasy about your change to format_volume - why did remove reference from storage parameter? I don't think there is any situation when we want to pass ownership of storage to this function. I know we can still pass reference in std mode because Read/Write/Seek are implemented for reference to type implementing those traits. But it won't work for no_std, will it?
If I understand correctly removal of IntoStorage trait allows you to avoid losing storage in Filesystem::new() by passing a reference. I always imagined people would pass ownership and use Filesystem like a wrapper. In such case we still lose access to storage object. Perhaps Filesystem::new() should return it together with the error...

[MathiasKoch]

I added IntoStorage trait to simplify usage with std and keep compatibility with old version, so people don't have to wrap IO into StdIoWrapper. But I was never happy with that solution and was planning to remove it so I support this part of your PR. I was thinking about moving Read/Write/Seek traits that are used in no_std mode to fscommon trait and implementing them for BufStream type. This way no wrapping would be needed anymore when fatfs is used with BufStream. Do you think it's a good idea?

Yeah, i think that would be a great solution!

I feel uneasy about your change to format_volume - why did remove reference from storage parameter? I don't think there is any situation when we want to pass ownership of storage to this function. I know we can still pass reference in std mode because Read/Write/Seek are implemented for reference to type implementing those traits. But it won't work for no_std, will it?

I did this in my previous attempt (OP), because with those changes, i now effectively had implemented Read/Write/Seek for &mut T instead of T.

If I understand correctly removal of IntoStorage trait allows you to avoid losing storage in Filesystem::new() by passing a reference. I always imagined people would pass ownership and use Filesystem like a wrapper. In such case we still lose access to storage object. Perhaps Filesystem::new() should return it together with the error...

This is the actual issue i am facing, and passing storage by reference is more of a workaround. Returning storage if the filesystem fails in Filesystem::new(), as well as when you explicitly unmount it, would probably solve most if not all of my issues. Problem with the current implementation is that IntoStorage consumes self in a way, such that there is no way to get storage back.