Introduce littlefs2-core crate #73
Conversation
Previously, DynFilesystem used OpenOptions for opening files in a custom mode. This has two drawbacks: OpenOptions are constructed using a callback leading to a rather complex method signature. And OpenOption depends on the Filesystem type, making it impossible to move DynFilesystem into a separate core crate without also moving Filesystem. This patch makes the FileOpenFlags struct public, changes its backing type to i32 to avoid unnecessary conversions and changes the DynFilesystem trait to accept FileOpenFlags instead of an OpenOptionsCallback. The affected methods are renamed from *with_options* to *with_flags*.
To make it easier to maintain applications using littlefs2, this patch introduces a littlefs2-core crate with the basic types and traits used by littlefs2, but without a specific implementation. Fixes: #55
By refactoring the Attribute struct using const generics, we can get rid of the (public) generic-array dependency.
To reduce the number of global items and for consistency with constants like u8::MAX, this patch replaces the size constants ATTRBYTES_MAX, PATH_MAX and PATH_MAX_PLUS_ONE with associated constants Attribute::MAX_SIZE, PathBuf::MAX_SIZE and PathBuf::MAX_SIZE_PLUS_ONE.
| pub fn is_dir(&self) -> bool { | ||
| *self == FileType::Dir | ||
| } | ||
|
|
||
| pub fn is_file(&self) -> bool { | ||
| *self == FileType::File | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: I'd rather use matches! here.
There was a problem hiding this comment.
Interesting. I just copied over the code, but would probably have used == too. It’s more concise and more intuitive and you don’t have to care about the correct order. Why do you prefer matches!?
There was a problem hiding this comment.
It doesn't depend on the PartialEq implementation. Since not all enums are PartialEq, I tend to prefer matches! everywhere for these small helper methods, even when PartialEq is derived for consistency. Not an important issue.
| pub struct Attribute { | ||
| id: u8, | ||
| data: [u8; Attribute::MAX_SIZE as _], | ||
| // invariant: size <= data.len() | ||
| size: usize, | ||
| } |
There was a problem hiding this comment.
Do we have actual uses of attributes?
I don't like the very large owned buffer maybe this shouldn't be in core for now?
There was a problem hiding this comment.
It is used in Trussed to implement user attributes, but as far as I see, they are only read and never written. But let’s discuss that separately from this PR.
| fn attribute(&self, path: &Path, id: u8) -> Result<Option<Attribute>>; | ||
| fn remove_attribute(&self, path: &Path, id: u8) -> Result<()>; | ||
| fn set_attribute(&self, path: &Path, attribute: &Attribute) -> Result<()>; |
There was a problem hiding this comment.
I think the attribute code could very well be exposed with a API like that:
fn attribute(&self, path: &Path, id: u8, buf: &mut [u8]) -> Result<Option<usize>>;
fn remove_attribute(&self, path: &Path, id: u8) -> Result<()>;
fn set_attribute(&self, id: u8, path: &Path, id: u8, data: &[u8)) -> Result<()>;There was a problem hiding this comment.
I think we need a low-level object-safe API like you suggested, and a non-object-safe but more user-friendly API. I’ll open a separate PR for that.
This PR introduces a
littlefs2-corecrate that provides an interface that does not depend on a specific littlefs2 version. The goal is to make applications usinglittlefs2easier to maintain as they no longer need to be updated if alittlefs2implementation detail is changed.The API of
littlefs2is mostly unaffected by this change as all moved items are re-exported under their old name. Some changes are required to de-couple the moved types from the actual implementation. See the changelog for details.Contrary to the initial proposal in #55, this does not move the
DynStoragetrait. From my point of view, it is coupled too tightly to the actual storage implementation that could be subject to change. Also, there is no real benefit of having it in the core crate as applications are not expected to use the storage directly. If necessary, we can always add this in the future.Before the core crate is released, we should consider removing the
dir-entry-pathfeature (and just always including the path) and putting theheaplessdependency behind a feature flag with potential support for other versions. But I’d prefer to discuss that separately.Fixes: #55