Alpha state

[ryankurte]

Hi hi, thanks for making a neat thing ^_^

I'm about to use littlefs in a project and I was just wondering whether there are outstanding issues / what you're waiting for for a non-alpha v0.x release?

Cheers,

Ryan

[nickray]

Awesome. I just put it out as alpha to gather some feedback on the API, do you have any?
My main concern is passing in &mut constantly, which would be a bit of work to finish implementing the parallel APIs. So I'd be interested in your thoughts on that.
It's something like:

let fs_with = fs.with(&mut storage);
fs_with.create_dir(path).unwrap();

etc.

[ryankurte]

Ah neat, and thanks for the reply! I'll get back to you on the API once I've had a real go at it.

[ryankurte]

Right so, having a go at using this and finding it rather,, difficult. Some feedback/thoughts/suggesion:

• Filesystem::mount imposes a lifetime requirement on the &mut alloc which means it's a real pain to embed the Filesystem object in another object.
• As you mentioned, constant passing of &mut T is pretty annoying
• Many of the APIs seem backwards? Filesystem::create(...) would be much clearer than File::create(&mut Filesystem...) (and could have the correct associated types from the Filesystem instance)
• How about making the filesystem object generic over the allocator and storage objects and storing them internally rather than taking references so you don't have to constantly pass them as arguments

You mention in the docs the &mut Storage binding to ensure thread safety, could this also be achieved with Filesystem::create<'a>(&'a mut self, ...) -> Result<File<'a>, ...> so the filesystem object is borrowed mutably as long as the file exists (and given Filesystem is !Sync this should impose the same requirement)

[nickray]

Thank you for the feedback, this is helpful! I'm currently using these bindings and getting actual experience myself.

Filesystem::mount imposes a lifetime requirement on the &mut alloc which means it's a real pain to embed the Filesystem object in another object.

The problem is this allocation is used in self-referential ways, so I need to fix it in place :/ In case you're good with Pin, do you see a way to use that here?

As you mentioned, constant passing of &mut T is pretty annoying

Yeah, so I'm tending towards changing this to take the T and hand it out on demand. I already have Filesystem/File and FilesystemWith/FileWith, thinking of making the (second) versions that own the references the only interface, and adding a way to borrow out the references.

Many of the APIs seem backwards? Filesystem::create(...) would be much clearer than File::create(&mut Filesystem...) (and could have the correct associated types from the Filesystem instance)

This is done to follow std::fs, where the constructor is on the file and not the filesystem. I'll definitel think about changing some more when I revisit.

How about making the filesystem object generic over the allocator and storage objects and storing them internally rather than taking references so you don't have to constantly pass them as arguments

See above, I think the self-referentiality prevents this. I may be misunderstanding your suggestion, can you clarify?

You mention in the docs the &mut Storage binding to ensure thread safety, could this also be achieved with Filesystem::create<'a>(&'a mut self, ...) -> Result<File<'a>, ...> so the filesystem object is borrowed mutably as long as the file exists (and given Filesystem is !Sync this should impose the same requirement)

Granted in my current use case I only have one file open at a time (store/retrieve blob), so I've considered this too, but it would prevent having more than one file open (unless the second borrows out the filesystem from the first etc.).

--

There's one more complication I've run into, which is that these files really need to be closed (or you run into problems caused by littlefs assumptions vs compiler optimizations). std::fs does this in drop, which we can't do since sync/close can fail (I don't want to panic).

--

We plan to use littlefs for our new FIDO2 firmware for SoloKeys, so this library will definitely be iterated/improved/tested etc. I understand @jonas-schievink is evaluating use on Cortex-A (iqlusioninc/usbarmory.rs#39) so I hope for a bright future (with possible RIIR...).

[nickray]

• Many of the APIs seem backwards? Filesystem::create(...) would be much clearer than File::create(&mut Filesystem...) (and could have the correct associated types from the Filesystem instance)

Regarding this, I believe Filesystem::{read,write} in https://github.com/nickray/littlefs2/pull/6 (mirroring std::fs, which I had missed in the alpha you looked at) should alleviate some of the pain points.

I do still want to mirror std::fs, but see your point! In https://github.com/nickray/littlefs2/pull/6 I am adding {create,open}_file_and_then (and the general open_file_with_options_and_then methods) to the Filesystem object. Does this address your suggestion appropriately @ryankurte?

In case you are wondering what's up with and_then:

• std::fs closes files on drop, and panics if something goes wrong
• with littlefs, flash corruption can be detected, and we want and API that allows to handle this case
• more importantly, a File can be mem::forgetted (as japaric noted), and then it won't be closed
• this then leads to UB of various kinds (as the main littlefs state object contains a linked list of all open files and directories, which fall off the stack)

The gist of said PR is to make the file/directory constructors unsafe, and instead add closure-based safe APIs that:

• do the file/directory allocation for you (yay!)
• close the objects appropriately even on error

With file/directory allocations handled in this way, of your original remarks the following remain (correct me if I missed something):

• Filesystem's basic allocation - I think one allocation (one lifetime on the type) is manageable. As an alternative, you could use mount_and_then
• I understand @japaric is working on some wrapper that does more/other allocation for you, I've yet to see what that is and whether it's still needed.

Thanks for your feedback!

[ryankurte]

I do still want to mirror std::fs, but see your point! In #6 I am adding {create,open}_file_and_then (and the general open_file_with_options_and_then methods) to the Filesystem object. Does this address your suggestion appropriately @ryankurte?

neat! looks much easier but i'll have to have a play with it once you've merged it in, i haven't had any more time to look at our littlefs stuff recently / got stalled on a segfault somewhere and had to move on, so, will update you when i have a chance.

The gist of said PR is to make the file/directory constructors unsafe, and instead add closure-based safe APIs ...

sounds like a good approach to me!