From 7e94b3d8b0acf35e60d6ae2fa13d7a6c5ef9391e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Mon, 10 Mar 2025 10:40:00 +0100 Subject: [PATCH 1/8] Remove need for generic array and typenum constants This will also allow for more flexible `Storage` implementations that use `Vec` as a cache buffer and are not fixed to a single block size --- Cargo.toml | 5 +- examples/list.rs | 31 +++++++++--- src/consts.rs | 3 -- src/driver.rs | 117 ++++++++++++++++++++++++++++++++++++------- src/fs.rs | 118 ++++++++++++++++++++------------------------ src/lib.rs | 2 +- src/macros.rs | 104 ++++++++++++++++++++++---------------- src/object_safe.rs | 16 +++--- src/tests.rs | 60 +++++++++++----------- tests/test_serde.rs | 2 +- 10 files changed, 281 insertions(+), 177 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 58632039..397f1930 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,10 +22,13 @@ repository.workspace = true [package.metadata.docs.rs] all-features = true +[[example]] +name = "list" +required-features = ["alloc"] + [dependencies] bitflags = "2.9.0" delog = "0.1.0" -generic-array = "0.14" heapless = "0.7" littlefs2-core = { version = "0.1", path = "core" } littlefs2-sys = { version = "0.3.1", features = ["multiversion"] } diff --git a/examples/list.rs b/examples/list.rs index 06300945..24721799 100644 --- a/examples/list.rs +++ b/examples/list.rs @@ -5,7 +5,6 @@ use std::{ }; use littlefs2::{ - consts::{U1, U512}, driver::Storage, fs::{Allocation, FileType, Filesystem}, io::{Error, Result}, @@ -30,7 +29,7 @@ fn main() { file, len: actual_len, }; - let mut alloc = Allocation::new(); + let mut alloc = Allocation::new(&s); let fs = Filesystem::mount(&mut alloc, &mut s).expect("failed to mount filesystem"); let available_blocks = fs.available_blocks().unwrap(); @@ -70,13 +69,29 @@ struct FileStorage { } impl Storage for FileStorage { - type CACHE_SIZE = U512; - type LOOKAHEAD_SIZE = U1; + type CACHE_BUFFER = Vec; + type LOOKAHEAD_BUFFER = Vec; - const READ_SIZE: usize = 16; - const WRITE_SIZE: usize = 512; - const BLOCK_SIZE: usize = BLOCK_SIZE; - const BLOCK_COUNT: usize = BLOCK_COUNT; + fn read_size(&self) -> usize { + 16 + } + fn write_size(&self) -> usize { + 512 + } + fn block_size(&self) -> usize { + BLOCK_SIZE + } + fn block_count(&self) -> usize { + BLOCK_COUNT + } + + fn cache_size(&self) -> usize { + 512 + } + + fn lookahead_size(&self) -> usize { + 1 + } fn read(&mut self, off: usize, buf: &mut [u8]) -> Result { assert!(off + buf.len() <= BLOCK_SIZE * BLOCK_COUNT); diff --git a/src/consts.rs b/src/consts.rs index 2abb971b..99ee0165 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -1,8 +1,5 @@ #![allow(non_camel_case_types)] -/// Re-export of `typenum::consts`. -pub use generic_array::typenum::consts::*; - pub const PATH_MAX: usize = littlefs2_core::PathBuf::MAX_SIZE; pub const PATH_MAX_PLUS_ONE: usize = littlefs2_core::PathBuf::MAX_SIZE_PLUS_ONE; pub const FILENAME_MAX_PLUS_ONE: u32 = 255 + 1; diff --git a/src/driver.rs b/src/driver.rs index 52c896c2..f6c07ebc 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -1,10 +1,92 @@ //! The `Storage`, `Read`, `Write` and `Seek` driver. #![allow(non_camel_case_types)] -use generic_array::ArrayLength; - use crate::io::Result; +mod private { + pub trait Sealed {} +} + +/// Safety: implemented only by `[u8; N]` and `Vec` if the alloc feature is enabled +pub unsafe trait Buffer: private::Sealed { + /// The maximum capacity of the buffer type. + /// Can be [`usize::Max`]() + const MAX_CAPACITY: usize; + + /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, + /// its last call defines the minimum number of valid bytes + fn as_ptr(&self) -> *const u8; + /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, + /// its last call defines the minimum number of valid bytes + fn as_mut_ptr(&mut self) -> *mut u8; + + /// Current capacity, set by the last call to [`set_capacity`](Buffer::set_capacity) + /// or at initialization through [`with_capacity`](Buffer::with_capacity) + fn current_capacity(&self) -> usize; + + /// Can panic if `capacity` > `Self::MAX_CAPACITY` + fn set_capacity(&mut self, capacity: usize); + + /// Can panic if `capacity` > `Self::MAX_CAPACITY` + fn with_capacity(capacity: usize) -> Self; +} + +impl private::Sealed for [u8; N] {} + +unsafe impl Buffer for [u8; N] { + const MAX_CAPACITY: usize = N; + + fn as_ptr(&self) -> *const u8 { + <[u8]>::as_ptr(self) + } + + fn as_mut_ptr(&mut self) -> *mut u8 { + <[u8]>::as_mut_ptr(self) + } + + fn current_capacity(&self) -> usize { + N + } + + fn set_capacity(&mut self, _capacity: usize) { + // noop, fixed capacity + } + fn with_capacity(capacity: usize) -> Self { + assert!(capacity <= N); + [0; N] + } +} + +#[cfg(feature = "alloc")] +impl private::Sealed for alloc::vec::Vec {} + +#[cfg(feature = "alloc")] +unsafe impl Buffer for alloc::vec::Vec { + const MAX_CAPACITY: usize = usize::MAX; + + fn as_ptr(&self) -> *const u8 { + <[u8]>::as_ptr(self) + } + + fn as_mut_ptr(&mut self) -> *mut u8 { + <[u8]>::as_mut_ptr(self) + } + + fn current_capacity(&self) -> usize { + self.capacity() + } + + fn set_capacity(&mut self, capacity: usize) { + self.resize(capacity, 0) + } + + fn with_capacity(capacity: usize) -> Self { + let mut this = alloc::vec::Vec::with_capacity(capacity); + this.set_capacity(capacity); + this + } +} + /// Users of this library provide a "storage driver" by implementing this trait. /// /// The `write` method is assumed to be synchronized to storage immediately. @@ -12,44 +94,43 @@ use crate::io::Result; /// Do note that due to caches, files still must be synched. And unfortunately, /// this can't be automatically done in `drop`, since it needs mut refs to both /// filesystem and storage. -/// -/// The `*_SIZE` types must be `generic_array::typenume::consts` such as `U256`. -/// -/// Why? Currently, associated constants can not be used (as constants...) to define -/// arrays. This "will be fixed" as part of const generics. -/// Once that's done, we can get rid of `generic-array`s, and replace the -/// `*_SIZE` types with `usize`s. pub trait Storage { // /// Error type for user-provided read/write/erase methods // type Error = usize; /// Minimum size of block read in bytes. Not in superblock - const READ_SIZE: usize; + fn read_size(&self) -> usize; /// Minimum size of block write in bytes. Not in superblock - const WRITE_SIZE: usize; + fn write_size(&self) -> usize; /// Size of an erasable block in bytes, as unsigned typenum. /// Must be a multiple of both `READ_SIZE` and `WRITE_SIZE`. /// [At least 128](https://github.com/littlefs-project/littlefs/issues/264#issuecomment-519963153). Stored in superblock. - const BLOCK_SIZE: usize; + fn block_size(&self) -> usize; /// Number of erasable blocks. /// Hence storage capacity is `BLOCK_COUNT * BLOCK_SIZE` - const BLOCK_COUNT: usize; + fn block_count(&self) -> usize; /// Suggested values are 100-1000, higher is more performant but /// less wear-leveled. Default of -1 disables wear-leveling. /// Value zero is invalid, must be positive or -1. - const BLOCK_CYCLES: isize = -1; + fn block_cycles(&self) -> isize { + -1 + } /// littlefs uses a read cache, a write cache, and one cache per per file. - /// Must be a multiple of `READ_SIZE` and `WRITE_SIZE`. - /// Must be a factor of `BLOCK_SIZE`. - type CACHE_SIZE: ArrayLength; + type CACHE_BUFFER: Buffer; + + /// Must be a multiple of `read_size` and `write_size`. + /// Must be a factor of `block_size`. + fn cache_size(&self) -> usize; + /// Lookahead buffer used by littlefs + type LOOKAHEAD_BUFFER: Buffer; /// Size of the lookahead buffer used by littlefs, measured in multiples of 8 bytes. - type LOOKAHEAD_SIZE: ArrayLength; + fn lookahead_size(&self) -> usize; ///// Maximum length of a filename plus one. Stored in superblock. ///// Should default to 255+1, but associated type defaults don't exist currently. diff --git a/src/fs.rs b/src/fs.rs index 5bbdaea1..c299eeac 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -7,16 +7,12 @@ use core::{ cell::{RefCell, UnsafeCell}, mem, slice, }; -use generic_array::typenum::marker_traits::Unsigned; use littlefs2_sys as ll; -// so far, don't need `heapless-bytes`. -pub type Bytes = generic_array::GenericArray; - pub use littlefs2_core::{Attribute, DirEntry, FileOpenFlags, FileType, Metadata}; use crate::{ - driver, + driver::{self, Buffer}, io::{self, Error, OpenSeekFrom, Result}, path::{Path, PathBuf}, DISK_VERSION, @@ -44,42 +40,31 @@ pub fn u32_result(return_value: i32) -> Result { } struct Cache { - read: UnsafeCell>, - write: UnsafeCell>, + read: UnsafeCell, + write: UnsafeCell, // lookahead: aligned::Aligned>, - lookahead: UnsafeCell>, + lookahead: UnsafeCell, + size: usize, } impl Cache { - pub fn new() -> Self { + pub fn new(storage: &S) -> Self { + let cache_size = storage.cache_size(); Self { - read: Default::default(), - write: Default::default(), - lookahead: Default::default(), + read: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), + write: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), + lookahead: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), + size: cache_size, } } } -impl Default for Cache { - fn default() -> Self { - Self::new() - } -} - pub struct Allocation { cache: Cache, config: ll::lfs_config, state: ll::lfs_t, } -// pub fn check_storage_requirements( - -impl Default for Allocation { - fn default() -> Self { - Self::new() - } -} - #[derive(Default, Clone, Debug)] #[non_exhaustive] pub struct Config { @@ -94,17 +79,18 @@ bitflags::bitflags! { } impl Allocation { - pub fn new() -> Self { - Self::with_config(Config::default()) + pub fn new(storage: &Storage) -> Self { + Self::with_config(storage, Config::default()) } - pub fn with_config(config: Config) -> Allocation { - let read_size: u32 = Storage::READ_SIZE as _; - let write_size: u32 = Storage::WRITE_SIZE as _; - let block_size: u32 = Storage::BLOCK_SIZE as _; - let cache_size: u32 = ::CACHE_SIZE::U32; - let lookahead_size: u32 = 8 * ::LOOKAHEAD_SIZE::U32; - let block_cycles: i32 = Storage::BLOCK_CYCLES as _; - let block_count: u32 = Storage::BLOCK_COUNT as _; + + pub fn with_config(storage: &Storage, config: Config) -> Allocation { + let read_size: u32 = storage.read_size() as _; + let write_size: u32 = storage.write_size() as _; + let block_size: u32 = storage.block_size() as _; + let cache_size: u32 = storage.cache_size() as _; + let lookahead_size: u32 = 8 * storage.lookahead_size() as u32; + let block_cycles: i32 = storage.block_cycles() as _; + let block_count: u32 = storage.block_count() as _; debug_assert!(block_cycles >= -1); debug_assert!(block_cycles != 0); @@ -130,7 +116,7 @@ impl Allocation { debug_assert!(cache_size <= block_size); debug_assert!(block_size % cache_size == 0); - let cache = Cache::new(); + let cache = Cache::new(storage); let filename_max_plus_one: u32 = crate::consts::FILENAME_MAX_PLUS_ONE; debug_assert!(filename_max_plus_one > 1); @@ -200,6 +186,7 @@ impl Allocation { // also consider "erasing" the lifetime completely pub struct Filesystem<'a, Storage: driver::Storage> { alloc: RefCell<&'a mut Allocation>, + cache_size: usize, storage: &'a mut Storage, } @@ -221,8 +208,8 @@ struct RemoveDirAllProgress { } impl Filesystem<'_, Storage> { - pub fn allocate() -> Allocation { - Allocation::new() + pub fn allocate(storage: &Storage) -> Allocation { + Allocation::new(storage) } pub fn format(storage: &mut Storage) -> Result<()> { @@ -230,7 +217,7 @@ impl Filesystem<'_, Storage> { } pub fn format_with_config(storage: &mut Storage, config: Config) -> Result<()> { - let alloc = &mut Allocation::with_config(config); + let alloc = &mut Allocation::with_config(storage, config); let fs = Filesystem::new(alloc, storage); let mut alloc = fs.alloc.borrow_mut(); let return_code = unsafe { ll::lfs_format(&mut alloc.state, &alloc.config) }; @@ -244,7 +231,7 @@ impl Filesystem<'_, Storage> { // TODO: check if this is equivalent to `is_formatted`. pub fn is_mountable_with_config(storage: &mut Storage, config: Config) -> bool { - let alloc = &mut Allocation::with_config(config); + let alloc = &mut Allocation::with_config(storage, config); Filesystem::mount(alloc, storage).is_ok() } @@ -269,7 +256,7 @@ impl Filesystem<'_, Storage> { config: Config, f: impl FnOnce(&Filesystem<'_, Storage>) -> Result, ) -> Result { - let mut alloc = Allocation::with_config(config); + let mut alloc = Allocation::with_config(storage, config); let fs = Filesystem::mount(&mut alloc, storage)?; f(&fs) } @@ -290,12 +277,12 @@ impl Filesystem<'_, Storage> { /// Total number of blocks in the filesystem pub fn total_blocks(&self) -> usize { - Storage::BLOCK_COUNT + self.storage.block_count() } /// Total number of bytes in the filesystem pub fn total_space(&self) -> usize { - Storage::BLOCK_COUNT * Storage::BLOCK_SIZE + self.storage.block_count() * self.storage.block_size() } /// Available number of unused blocks in the filesystem @@ -320,7 +307,7 @@ impl Filesystem<'_, Storage> { /// Second, files may be inlined. pub fn available_space(&self) -> Result { self.available_blocks() - .map(|blocks| blocks * Storage::BLOCK_SIZE) + .map(|blocks| blocks * self.storage.block_size()) } /// Remove a file or directory. @@ -562,7 +549,7 @@ impl Filesystem<'_, Storage> { let storage = unsafe { &mut *((*c).context as *mut Storage) }; debug_assert!(!c.is_null()); // let block_size = unsafe { c.read().block_size }; - let block_size = Storage::BLOCK_SIZE as u32; + let block_size = storage.block_size() as u32; let off = (block * block_size + off) as usize; let buf: &[u8] = unsafe { slice::from_raw_parts(buffer as *const u8, size as usize) }; @@ -574,9 +561,9 @@ impl Filesystem<'_, Storage> { extern "C" fn lfs_config_erase(c: *const ll::lfs_config, block: ll::lfs_block_t) -> c_int { // println!("in lfs_config_erase"); let storage = unsafe { &mut *((*c).context as *mut Storage) }; - let off = block as usize * Storage::BLOCK_SIZE; + let off = block as usize * storage.block_size(); - error_code_from(storage.erase(off, Storage::BLOCK_SIZE)) + error_code_from(storage.erase(off, storage.block_size())) } /// C callback interface used by LittleFS to sync data with the lower level interface below the @@ -590,22 +577,21 @@ impl Filesystem<'_, Storage> { /// The state of a `File`. Pre-allocate with `File::allocate`. pub struct FileAllocation { - cache: UnsafeCell>, + cache: UnsafeCell, + cache_size: usize, state: ll::lfs_file_t, config: ll::lfs_file_config, } -impl Default for FileAllocation { - fn default() -> Self { - Self::new() - } -} - impl FileAllocation { - pub fn new() -> Self { - let cache_size: u32 = ::CACHE_SIZE::to_u32(); + pub fn new(cache_size: usize) -> Self { debug_assert!(cache_size > 0); - unsafe { mem::MaybeUninit::zeroed().assume_init() } + Self { + cache: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), + cache_size, + state: unsafe { mem::MaybeUninit::zeroed().assume_init() }, + config: unsafe { mem::MaybeUninit::zeroed().assume_init() }, + } } } @@ -617,8 +603,8 @@ pub struct File<'a, 'b, S: driver::Storage> { } impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { - pub fn allocate() -> FileAllocation { - FileAllocation::new() + pub fn allocate(fs: &Filesystem<'_, Storage>) -> FileAllocation { + FileAllocation::new(fs.cache_size) } /// Returns a new OpenOptions object. @@ -806,7 +792,8 @@ impl OpenOptions { alloc: &mut FileAllocation, path: &Path, ) -> Result> { - alloc.config.buffer = alloc.cache.get() as *mut _; + assert_eq!(fs.cache_size, alloc.cache_size); + alloc.config.buffer = alloc.cache.get_mut().as_mut_ptr() as *mut _; // We need to use addr_of_mut! here instead of & mut since // the FFI stores a copy of a pointer to the field state, // so we cannot assert unique mutable access. @@ -833,7 +820,7 @@ impl OpenOptions { path: &Path, f: impl FnOnce(&File<'a, '_, S>) -> Result, ) -> Result { - let mut alloc = FileAllocation::new(); // lifetime 'c + let mut alloc = File::allocate(fs); // lifetime 'c let mut file = unsafe { self.open(fs, &mut alloc, path)? }; // Q: what is the actually correct behaviour? // E.g. if res is Ok but closing gives an error. @@ -1141,6 +1128,7 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { fn new(alloc: &'a mut Allocation, storage: &'a mut Storage) -> Self { Self::set_alloc_config(alloc, storage); Filesystem { + cache_size: alloc.cache.size, alloc: RefCell::new(alloc), storage, } @@ -1418,7 +1406,7 @@ mod tests { }) .unwrap(); - let mut alloc = Allocation::new(); + let mut alloc = Allocation::new(&test_storage); let fs = Filesystem::mount(&mut alloc, &mut test_storage).unwrap(); // fs.write(b"/z.txt\0".try_into().unwrap(), &jackson5).unwrap(); fs.write(path!("z.txt"), jackson5).unwrap(); @@ -1529,11 +1517,11 @@ mod tests { Ok(()) })?; - let mut a1 = File::allocate(); + let mut a1 = File::allocate(fs); let f1 = unsafe { File::create(fs, &mut a1, b"a.txt\0".try_into().unwrap())? }; f1.write(b"some text")?; - let mut a2 = File::allocate(); + let mut a2 = File::allocate(fs); let f2 = unsafe { File::create(fs, &mut a2, b"b.txt\0".try_into().unwrap())? }; f2.write(b"more text")?; diff --git a/src/lib.rs b/src/lib.rs index c7c5f92d..a4688c52 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -107,7 +107,7 @@ let mut storage = RamStorage::new(&mut ram); // must format before first mount Filesystem::format(&mut storage).unwrap(); // must allocate state statically before use -let mut alloc = Filesystem::allocate(); +let mut alloc = Filesystem::allocate(&storage); let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap(); // may use common `OpenOptions` diff --git a/src/macros.rs b/src/macros.rs index c6e448e7..64220287 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -11,12 +11,10 @@ macro_rules! ram_storage { erase_value=$erase_value:expr, read_size=$read_size:expr, write_size=$write_size:expr, - cache_size_ty=$cache_size:path, + cache_size=$cache_size:expr, block_size=$block_size:expr, block_count=$block_count:expr, - lookahead_size_ty=$lookahead_size:path, - filename_max_plus_one_ty=$filename_max_plus_one:path, - path_max_plus_one_ty=$path_max_plus_one:path, + lookahead_size=$lookahead_size:expr, ) => { pub struct $Backend { @@ -43,15 +41,30 @@ macro_rules! ram_storage { } impl<'backend> $crate::driver::Storage for $Name<'backend> { - const READ_SIZE: usize = $read_size; - const WRITE_SIZE: usize = $write_size; - type CACHE_SIZE = $cache_size; - const BLOCK_SIZE: usize = $block_size; - const BLOCK_COUNT: usize = $block_count; - type LOOKAHEAD_SIZE = $lookahead_size; + fn read_size(&self) -> usize { + $read_size + } + fn write_size(&self) -> usize { + $write_size + } + fn block_size(&self) -> usize { + $block_size + } + fn cache_size(&self) -> usize { + $cache_size + } + type CACHE_BUFFER = [u8; $cache_size]; + fn block_count(&self) -> usize { + $block_count + } + + fn lookahead_size(&self) -> usize { + $lookahead_size + } + type LOOKAHEAD_BUFFER = [u8; $lookahead_size * 8]; fn read(&mut self, offset: usize, buf: &mut [u8]) -> $crate::io::Result { - let read_size: usize = Self::READ_SIZE; + let read_size: usize = self.read_size(); debug_assert!(offset % read_size == 0); debug_assert!(buf.len() % read_size == 0); for (from, to) in self.backend.buf[offset..].iter().zip(buf.iter_mut()) { @@ -61,7 +74,7 @@ macro_rules! ram_storage { } fn write(&mut self, offset: usize, data: &[u8]) -> $crate::io::Result { - let write_size: usize = Self::WRITE_SIZE; + let write_size: usize = self.write_size(); debug_assert!(offset % write_size == 0); debug_assert!(data.len() % write_size == 0); for (from, to) in data.iter().zip(self.backend.buf[offset..].iter_mut()) { @@ -71,7 +84,7 @@ macro_rules! ram_storage { } fn erase(&mut self, offset: usize, len: usize) -> $crate::io::Result { - let block_size: usize = Self::BLOCK_SIZE; + let block_size: usize = self.block_size(); debug_assert!(offset % block_size == 0); debug_assert!(len % block_size == 0); for byte in self.backend.buf[offset..offset + len].iter_mut() { @@ -88,12 +101,10 @@ macro_rules! ram_storage { erase_value = 0xff, read_size = 1, write_size = 1, - cache_size_ty = $crate::consts::U32, + cache_size = 32, block_size = 128, block_count = $bytes / 128, - lookahead_size_ty = $crate::consts::U1, - filename_max_plus_one_ty = $crate::consts::U256, - path_max_plus_one_ty = $crate::consts::U256, + lookahead_size = 1, ); }; (tiny) => { @@ -103,12 +114,10 @@ macro_rules! ram_storage { erase_value = 0xff, read_size = 32, write_size = 32, - cache_size_ty = $crate::consts::U32, + cache_size = 32, block_size = 128, block_count = 8, - lookahead_size_ty = $crate::consts::U1, - filename_max_plus_one_ty = $crate::consts::U256, - path_max_plus_one_ty = $crate::consts::U256, + lookahead_size = 1, ); }; (large) => { @@ -118,12 +127,10 @@ macro_rules! ram_storage { erase_value = 0xff, read_size = 32, write_size = 32, - cache_size_ty = $crate::consts::U32, + cache_size = 32, block_size = 256, block_count = 512, - lookahead_size_ty = $crate::consts::U4, - filename_max_plus_one_ty = $crate::consts::U256, - path_max_plus_one_ty = $crate::consts::U256, + lookahead_size = 4, ); }; } @@ -136,12 +143,10 @@ macro_rules! const_ram_storage { erase_value=$erase_value:expr, read_size=$read_size:expr, write_size=$write_size:expr, - cache_size_ty=$cache_size:path, + cache_size=$cache_size:expr, block_size=$block_size:expr, block_count=$block_count:expr, - lookahead_size_ty=$lookahead_size:path, - filename_max_plus_one_ty=$filename_max_plus_one:path, - path_max_plus_one_ty=$path_max_plus_one:path, + lookahead_size=$lookahead_size:expr, ) => { pub struct $Name { @@ -167,15 +172,32 @@ macro_rules! const_ram_storage { } impl $crate::driver::Storage for $Name { - const READ_SIZE: usize = $read_size; - const WRITE_SIZE: usize = $write_size; - type CACHE_SIZE = $cache_size; - const BLOCK_SIZE: usize = $block_size; - const BLOCK_COUNT: usize = $block_count; - type LOOKAHEAD_SIZE = $lookahead_size; + fn read_size(&self) -> usize { + $read_size + } + + fn write_size(&self) -> usize { + $write_size + } + + fn cache_size(&self) -> usize { + $cache_size + } + type CACHE_BUFFER = [u8; $cache_size]; + fn block_size(&self) -> usize { + $block_size + } + fn block_count(&self) -> usize { + $block_count + } + + fn lookahead_size(&self) -> usize { + $lookahead_size + } + type LOOKAHEAD_BUFFER = [u8; $lookahead_size * 8]; fn read(&mut self, offset: usize, buf: &mut [u8]) -> $crate::io::Result { - let read_size: usize = Self::READ_SIZE; + let read_size = self.read_size(); debug_assert!(offset % read_size == 0); debug_assert!(buf.len() % read_size == 0); for (from, to) in self.buf[offset..].iter().zip(buf.iter_mut()) { @@ -185,7 +207,7 @@ macro_rules! const_ram_storage { } fn write(&mut self, offset: usize, data: &[u8]) -> $crate::io::Result { - let write_size: usize = Self::WRITE_SIZE; + let write_size = self.write_size(); debug_assert!(offset % write_size == 0); debug_assert!(data.len() % write_size == 0); for (from, to) in data.iter().zip(self.buf[offset..].iter_mut()) { @@ -195,7 +217,7 @@ macro_rules! const_ram_storage { } fn erase(&mut self, offset: usize, len: usize) -> $crate::io::Result { - let block_size: usize = Self::BLOCK_SIZE; + let block_size: usize = self.block_size(); debug_assert!(offset % block_size == 0); debug_assert!(len % block_size == 0); for byte in self.buf[offset..offset + len].iter_mut() { @@ -211,12 +233,10 @@ macro_rules! const_ram_storage { erase_value = 0xff, read_size = 16, write_size = 512, - cache_size_ty = $crate::consts::U512, + cache_size = 512, block_size = 512, block_count = $bytes / 512, - lookahead_size_ty = $crate::consts::U1, - filename_max_plus_one_ty = $crate::consts::U256, - path_max_plus_one_ty = $crate::consts::U256, + lookahead_size = 1, ); }; } diff --git a/src/object_safe.rs b/src/object_safe.rs index fb102859..23635e35 100644 --- a/src/object_safe.rs +++ b/src/object_safe.rs @@ -1,7 +1,5 @@ //! Object-safe traits for [`File`][], [`Filesystem`][] and [`Storage`][]. -use generic_array::typenum::Unsigned as _; - use crate::{ driver::Storage, fs::{Attribute, File, FileOpenFlags, Filesystem, Metadata}, @@ -179,31 +177,31 @@ pub trait DynStorage { impl DynStorage for S { fn read_size(&self) -> usize { - Self::READ_SIZE + ::read_size(self) } fn write_size(&self) -> usize { - Self::WRITE_SIZE + ::write_size(self) } fn block_size(&self) -> usize { - Self::BLOCK_SIZE + ::block_size(self) } fn block_count(&self) -> usize { - Self::BLOCK_COUNT + ::block_count(self) } fn block_cycles(&self) -> isize { - Self::BLOCK_CYCLES + ::block_cycles(self) } fn cache_size(&self) -> usize { - S::CACHE_SIZE::to_usize() + ::cache_size(self) } fn lookahead_size(&self) -> usize { - S::LOOKAHEAD_SIZE::to_usize() + ::lookahead_size(self) } fn read(&mut self, off: usize, buf: &mut [u8]) -> Result { diff --git a/src/tests.rs b/src/tests.rs index 2a0bf9c2..74097253 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,5 +1,4 @@ use core::convert::TryInto; -use generic_array::typenum::consts; use littlefs2_core::PathBuf; use crate::{ @@ -9,18 +8,19 @@ use crate::{ path, BACKEND_VERSION, DISK_VERSION, }; +const RAM_STORAGE_BLOCK_COUNT: usize = 32; +const LARGER_RAM_STORAGE_BLOCK_COUNT: usize = 64; + ram_storage!( name = OtherRamStorage, backend = OtherRam, erase_value = 0xff, read_size = 1, write_size = 32, - cache_size_ty = consts::U32, + cache_size = 32, block_size = 256, block_count = 512, - lookahead_size_ty = consts::U1, - filename_max_plus_one_ty = consts::U256, - path_max_plus_one_ty = consts::U256, + lookahead_size = 1, ); ram_storage!( @@ -29,12 +29,10 @@ ram_storage!( erase_value = 0xff, read_size = 20 * 5, write_size = 20 * 7, - cache_size_ty = consts::U700, + cache_size = 700, block_size = 20 * 35, - block_count = 32, - lookahead_size_ty = consts::U16, - filename_max_plus_one_ty = consts::U256, - path_max_plus_one_ty = consts::U256, + block_count = RAM_STORAGE_BLOCK_COUNT, + lookahead_size = 16, ); ram_storage!( @@ -43,12 +41,10 @@ ram_storage!( erase_value = 0xff, read_size = 20 * 5, write_size = 20 * 7, - cache_size_ty = consts::U700, + cache_size = 700, block_size = 20 * 35, - block_count = 64, - lookahead_size_ty = consts::U16, - filename_max_plus_one_ty = consts::U256, - path_max_plus_one_ty = consts::U256, + block_count = LARGER_RAM_STORAGE_BLOCK_COUNT, + lookahead_size = 16, ); #[test] @@ -61,7 +57,7 @@ fn version() { fn format() { let mut backend = OtherRam::default(); let mut storage = OtherRamStorage::new(&mut backend); - let mut alloc = Filesystem::allocate(); + let mut alloc = Filesystem::allocate(&storage); // should fail: FS is not formatted assert_eq!( @@ -93,7 +89,7 @@ fn borrow_fs_allocation() { let mut backend = OtherRam::default(); let mut storage = OtherRamStorage::new(&mut backend); - let mut alloc_fs = Filesystem::allocate(); + let mut alloc_fs = Filesystem::allocate(&storage); Filesystem::format(&mut storage).unwrap(); let _fs = Filesystem::mount(&mut alloc_fs, &mut storage).unwrap(); // previous `_fs` is fine as it's masked, due to NLL @@ -110,7 +106,7 @@ fn borrow_fs_allocation2() { let mut backend = OtherRam::default(); let mut storage = OtherRamStorage::new(&mut backend); - let mut alloc_fs = Filesystem::allocate(); + let mut alloc_fs = Filesystem::allocate(&storage); Filesystem::format(&mut storage).unwrap(); let _fs = Filesystem::mount(&mut alloc_fs, &mut storage).unwrap(); // previous `_fs` is fine as it's masked, due to NLL @@ -541,9 +537,9 @@ fn test_iter_dirs() { fn test_mount_or_else_clobber_alloc() { let mut backend = Ram::default(); let mut storage = RamStorage::new(&mut backend); - let alloc = &mut Allocation::new(); + let alloc = &mut Allocation::new(&storage); Filesystem::mount_or_else(alloc, &mut storage, |_, storage, alloc| { - *alloc = Allocation::new(); + *alloc = Allocation::new(storage); Filesystem::format(storage).unwrap(); Ok(()) }) @@ -566,7 +562,7 @@ fn test_mount_or_else_clobber_alloc() { fn shrinking() { let backend = &mut Ram::default(); let storage = &mut RamStorage::new(backend); - let alloc = &mut Allocation::new(); + let alloc = &mut Allocation::new(storage); Filesystem::format(storage).unwrap(); let fs = Filesystem::mount(alloc, storage).unwrap(); @@ -582,15 +578,18 @@ fn shrinking() { let larger_backend = &mut LargerRam::default(); larger_backend.buf[..backend.buf.len()].copy_from_slice(&backend.buf); let larger_storage = &mut LargerRamStorage::new(larger_backend); - let larger_alloc = &mut Allocation::new(); + let larger_alloc = &mut Allocation::new(larger_storage); assert!(matches!( Filesystem::mount(larger_alloc, larger_storage), Err(Error::INVALID) )); - let larger_alloc = &mut Allocation::with_config(crate::fs::Config { - mount_flags: MountFlags::DISABLE_BLOCK_COUNT_CHECK, - }); + let larger_alloc = &mut Allocation::with_config( + larger_storage, + crate::fs::Config { + mount_flags: MountFlags::DISABLE_BLOCK_COUNT_CHECK, + }, + ); let fs = Filesystem::mount(larger_alloc, larger_storage).unwrap(); assert_eq!(fs.read::<10>(path!("some-file")).unwrap(), &[42; 10]); @@ -599,14 +598,14 @@ fn shrinking() { &[42; 1024] ); - fs.grow(LargerRamStorage::BLOCK_COUNT).unwrap(); + fs.grow(LARGER_RAM_STORAGE_BLOCK_COUNT).unwrap(); assert_eq!(fs.read::<10>(path!("some-file")).unwrap(), &[42; 10]); assert_eq!( fs.read::<1024>(path!("some-large-file")).unwrap(), &[42; 1024] ); - fs.shrink(RamStorage::BLOCK_COUNT).unwrap(); + fs.shrink(RAM_STORAGE_BLOCK_COUNT).unwrap(); assert_eq!(fs.read::<10>(path!("some-file")).unwrap(), &[42; 10]); assert_eq!( fs.read::<1024>(path!("some-large-file")).unwrap(), @@ -618,7 +617,7 @@ fn shrinking() { fn shrinking_full() { let larger_backend = &mut LargerRam::default(); let larger_storage = &mut LargerRamStorage::new(larger_backend); - let larger_alloc = &mut Allocation::new(); + let larger_alloc = &mut Allocation::new(larger_storage); Filesystem::format(larger_storage).unwrap(); let fs = Filesystem::mount(larger_alloc, larger_storage).unwrap(); @@ -632,8 +631,11 @@ fn shrinking_full() { } } + let backend = &mut Ram::default(); + let storage = RamStorage::new(backend); + assert!(matches!( - fs.shrink(RamStorage::BLOCK_COUNT), + fs.shrink(storage.block_count()), Err(Error::DIR_NOT_EMPTY) )) } diff --git a/tests/test_serde.rs b/tests/test_serde.rs index de05c7f5..cbe7ddad 100644 --- a/tests/test_serde.rs +++ b/tests/test_serde.rs @@ -19,7 +19,7 @@ fn main() { let mut storage = RamStorage::new(&mut ram); Filesystem::format(&mut storage).unwrap(); - let mut alloc = Filesystem::allocate(); + let mut alloc = Filesystem::allocate(&storage); let fs = Filesystem::mount(&mut alloc, &mut storage).unwrap(); let entity = Entity::default(); From 4f055aab12cf4255bc228c7431c592c35338a2ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Wed, 18 Jun 2025 15:24:30 +0200 Subject: [PATCH 2/8] Make list accept dynamic Filesystem parameters without recompilation --- Cargo.toml | 1 + examples/list.rs | 59 +++++++++++++++++++------ src/driver.rs | 99 +++++++++++++++++++++--------------------- src/fs.rs | 109 +++++++++++++++++++++++++++++++---------------- 4 files changed, 168 insertions(+), 100 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 397f1930..43c27579 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,7 @@ littlefs2-sys = { version = "0.3.1", features = ["multiversion"] } [dev-dependencies] ssmarshal = "1" serde = { version = "1.0", default-features = false, features = ["derive"] } +clap = { version = "4.5.31", features = ["derive"] } # trybuild = "1" [features] diff --git a/examples/list.rs b/examples/list.rs index 24721799..7615e72c 100644 --- a/examples/list.rs +++ b/examples/list.rs @@ -1,5 +1,4 @@ use std::{ - env, fs::File, io::{Read as _, Seek as _, SeekFrom}, }; @@ -12,28 +11,54 @@ use littlefs2::{ path::{Path, PathBuf}, }; -const BLOCK_COUNT: usize = 128; -const BLOCK_SIZE: usize = 512; +use clap::Parser; + +#[derive(Parser)] +struct Args { + path: String, + block_size: usize, + #[arg(short, long)] + write_size: Option, + #[arg(short, long)] + read_size: Option, + #[arg(short, long)] + cache_size: Option, + #[arg(short, long)] + lookahead_size: Option, + #[arg(short, long)] + block_count: Option, +} + +const BLOCK_COUNT: usize = 288; +const BLOCK_SIZE: usize = 256; fn main() { - let path = env::args().nth(1).expect("missing argument"); - let file = File::open(&path).expect("failed to open file"); + let args = Args::parse(); + let file = File::open(&args.path).expect("failed to open file"); let metadata = file.metadata().expect("failed to query metadata"); - let expected_len = BLOCK_COUNT * BLOCK_SIZE; let actual_len = usize::try_from(metadata.len()).unwrap(); - assert_eq!(actual_len % BLOCK_COUNT, 0); + if let Some(block_count) = args.block_count { + assert_eq!(actual_len, args.block_size * block_count); + } + assert_eq!(actual_len % args.block_size, 0); + let block_count = actual_len / args.block_size; let mut s = FileStorage { file, len: actual_len, + read_size: args.read_size.unwrap_or(args.block_size), + write_size: args.write_size.unwrap_or(args.block_size), + cache_size: args.cache_size.unwrap_or(args.block_size), + lookahead_size: args.lookahead_size.unwrap_or(1), + block_count, + block_size: args.block_size, }; let mut alloc = Allocation::new(&s); let fs = Filesystem::mount(&mut alloc, &mut s).expect("failed to mount filesystem"); let available_blocks = fs.available_blocks().unwrap(); - println!("expected_len: {expected_len}"); println!("actual_len: {actual_len}"); println!("available_blocks: {available_blocks}"); println!(); @@ -66,6 +91,12 @@ fn list(fs: &dyn DynFilesystem, path: &Path) { struct FileStorage { file: File, len: usize, + read_size: usize, + write_size: usize, + block_size: usize, + block_count: usize, + cache_size: usize, + lookahead_size: usize, } impl Storage for FileStorage { @@ -73,24 +104,24 @@ impl Storage for FileStorage { type LOOKAHEAD_BUFFER = Vec; fn read_size(&self) -> usize { - 16 + self.read_size } fn write_size(&self) -> usize { - 512 + self.write_size } fn block_size(&self) -> usize { - BLOCK_SIZE + self.block_size } fn block_count(&self) -> usize { - BLOCK_COUNT + self.block_count } fn cache_size(&self) -> usize { - 512 + self.cache_size } fn lookahead_size(&self) -> usize { - 1 + self.lookahead_size } fn read(&mut self, off: usize, buf: &mut [u8]) -> Result { diff --git a/src/driver.rs b/src/driver.rs index f6c07ebc..75a98715 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -1,41 +1,37 @@ //! The `Storage`, `Read`, `Write` and `Seek` driver. #![allow(non_camel_case_types)] -use crate::io::Result; +use crate::io::Error; mod private { - pub trait Sealed {} -} - -/// Safety: implemented only by `[u8; N]` and `Vec` if the alloc feature is enabled -pub unsafe trait Buffer: private::Sealed { - /// The maximum capacity of the buffer type. - /// Can be [`usize::Max`]() - const MAX_CAPACITY: usize; - - /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, - /// its last call defines the minimum number of valid bytes - fn as_ptr(&self) -> *const u8; - /// Returns a buffer of bytes initialized and valid. If [`set_capacity`]() was called previously, - /// its last call defines the minimum number of valid bytes - fn as_mut_ptr(&mut self) -> *mut u8; - - /// Current capacity, set by the last call to [`set_capacity`](Buffer::set_capacity) - /// or at initialization through [`with_capacity`](Buffer::with_capacity) - fn current_capacity(&self) -> usize; - - /// Can panic if `capacity` > `Self::MAX_CAPACITY` - fn set_capacity(&mut self, capacity: usize); - - /// Can panic if `capacity` > `Self::MAX_CAPACITY` - fn with_capacity(capacity: usize) -> Self; + pub struct NotEnoughCapacity; + pub trait Sealed { + /// Returns a buffer of bytes initialized and valid. If [`set_len`]() was called previously successfully, + /// its last call defines the minimum number of valid bytes + fn as_ptr(&self) -> *const u8; + /// Returns a buffer of bytes initialized and valid. If [`set_len`]() was called previously successfully, + /// its last call defines the minimum number of valid bytes + fn as_mut_ptr(&mut self) -> *mut u8; + + /// Current lenght, set by the last call to [`set_len`](Buffer::set_len) + fn current_len(&self) -> usize; + + /// Atempts to set the length of the buffer to `len` + /// + /// If succeeded, the buffer obtained through the pointer operation **must** be of at least `len` bytes + fn set_len(&mut self, len: usize) -> Result<(), NotEnoughCapacity>; + + // We could use a `Default` trait bound but it's not implemented for all array sizes + fn empty() -> Self; + } } -impl private::Sealed for [u8; N] {} +pub(crate) use private::Sealed; -unsafe impl Buffer for [u8; N] { - const MAX_CAPACITY: usize = N; +/// Safety: implemented only by `[u8; N]` and `Vec` if the alloc feature is enabled +pub unsafe trait Buffer: private::Sealed {} +impl private::Sealed for [u8; N] { fn as_ptr(&self) -> *const u8 { <[u8]>::as_ptr(self) } @@ -44,26 +40,27 @@ unsafe impl Buffer for [u8; N] { <[u8]>::as_mut_ptr(self) } - fn current_capacity(&self) -> usize { + fn current_len(&self) -> usize { N } - fn set_capacity(&mut self, _capacity: usize) { - // noop, fixed capacity + fn set_len(&mut self, len: usize) -> Result<(), private::NotEnoughCapacity> { + if len > N { + Err(private::NotEnoughCapacity) + } else { + Ok(()) + } } - fn with_capacity(capacity: usize) -> Self { - assert!(capacity <= N); + + fn empty() -> Self { [0; N] } } -#[cfg(feature = "alloc")] -impl private::Sealed for alloc::vec::Vec {} +unsafe impl Buffer for [u8; N] {} #[cfg(feature = "alloc")] -unsafe impl Buffer for alloc::vec::Vec { - const MAX_CAPACITY: usize = usize::MAX; - +impl private::Sealed for alloc::vec::Vec { fn as_ptr(&self) -> *const u8 { <[u8]>::as_ptr(self) } @@ -72,21 +69,23 @@ unsafe impl Buffer for alloc::vec::Vec { <[u8]>::as_mut_ptr(self) } - fn current_capacity(&self) -> usize { - self.capacity() + fn current_len(&self) -> usize { + self.len() } - fn set_capacity(&mut self, capacity: usize) { - self.resize(capacity, 0) + fn set_len(&mut self, len: usize) -> Result<(), private::NotEnoughCapacity> { + self.resize(len, 0); + Ok(()) } - fn with_capacity(capacity: usize) -> Self { - let mut this = alloc::vec::Vec::with_capacity(capacity); - this.set_capacity(capacity); - this + fn empty() -> Self { + Self::new() } } +#[cfg(feature = "alloc")] +unsafe impl Buffer for alloc::vec::Vec {} + /// Users of this library provide a "storage driver" by implementing this trait. /// /// The `write` method is assumed to be synchronized to storage immediately. @@ -164,13 +163,13 @@ pub trait Storage { /// Read data from the storage device. /// Guaranteed to be called only with bufs of length a multiple of READ_SIZE. - fn read(&mut self, off: usize, buf: &mut [u8]) -> Result; + fn read(&mut self, off: usize, buf: &mut [u8]) -> Result; /// Write data to the storage device. /// Guaranteed to be called only with bufs of length a multiple of WRITE_SIZE. - fn write(&mut self, off: usize, data: &[u8]) -> Result; + fn write(&mut self, off: usize, data: &[u8]) -> Result; /// Erase data from the storage device. /// Guaranteed to be called only with bufs of length a multiple of BLOCK_SIZE. - fn erase(&mut self, off: usize, len: usize) -> Result; + fn erase(&mut self, off: usize, len: usize) -> Result; // /// Synchronize writes to the storage device. // fn sync(&mut self) -> Result; } diff --git a/src/fs.rs b/src/fs.rs index c299eeac..818644bd 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -12,7 +12,7 @@ use littlefs2_sys as ll; pub use littlefs2_core::{Attribute, DirEntry, FileOpenFlags, FileType, Metadata}; use crate::{ - driver::{self, Buffer}, + driver::{self, Sealed}, io::{self, Error, OpenSeekFrom, Result}, path::{Path, PathBuf}, DISK_VERSION, @@ -42,19 +42,15 @@ pub fn u32_result(return_value: i32) -> Result { struct Cache { read: UnsafeCell, write: UnsafeCell, - // lookahead: aligned::Aligned>, - lookahead: UnsafeCell, - size: usize, + lookahead: UnsafeCell, } impl Cache { - pub fn new(storage: &S) -> Self { - let cache_size = storage.cache_size(); + pub fn new() -> Self { Self { - read: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), - write: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), - lookahead: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), - size: cache_size, + read: UnsafeCell::new(S::CACHE_BUFFER::empty()), + write: UnsafeCell::new(S::CACHE_BUFFER::empty()), + lookahead: UnsafeCell::new(S::LOOKAHEAD_BUFFER::empty()), } } } @@ -116,7 +112,7 @@ impl Allocation { debug_assert!(cache_size <= block_size); debug_assert!(block_size % cache_size == 0); - let cache = Cache::new(storage); + let cache = Cache::new(); let filename_max_plus_one: u32 = crate::consts::FILENAME_MAX_PLUS_ONE; debug_assert!(filename_max_plus_one > 1); @@ -186,8 +182,8 @@ impl Allocation { // also consider "erasing" the lifetime completely pub struct Filesystem<'a, Storage: driver::Storage> { alloc: RefCell<&'a mut Allocation>, - cache_size: usize, storage: &'a mut Storage, + cache_size: usize, } fn metadata(info: ll::lfs_info) -> Metadata { @@ -218,7 +214,7 @@ impl Filesystem<'_, Storage> { pub fn format_with_config(storage: &mut Storage, config: Config) -> Result<()> { let alloc = &mut Allocation::with_config(storage, config); - let fs = Filesystem::new(alloc, storage); + let fs = Filesystem::new(alloc, storage)?; let mut alloc = fs.alloc.borrow_mut(); let return_code = unsafe { ll::lfs_format(&mut alloc.state, &alloc.config) }; result_from((), return_code) @@ -578,23 +574,26 @@ impl Filesystem<'_, Storage> { /// The state of a `File`. Pre-allocate with `File::allocate`. pub struct FileAllocation { cache: UnsafeCell, - cache_size: usize, state: ll::lfs_file_t, config: ll::lfs_file_config, } impl FileAllocation { - pub fn new(cache_size: usize) -> Self { - debug_assert!(cache_size > 0); + pub fn new() -> Self { Self { - cache: UnsafeCell::new(S::CACHE_BUFFER::with_capacity(cache_size)), - cache_size, + cache: UnsafeCell::new(S::CACHE_BUFFER::empty()), state: unsafe { mem::MaybeUninit::zeroed().assume_init() }, config: unsafe { mem::MaybeUninit::zeroed().assume_init() }, } } } +impl Default for FileAllocation { + fn default() -> Self { + Self::new() + } +} + pub struct File<'a, 'b, S: driver::Storage> { // We must store a raw pointer here since the FFI retains a copy of a pointer // to the field alloc.state, so we cannot assert unique mutable access. @@ -603,8 +602,8 @@ pub struct File<'a, 'b, S: driver::Storage> { } impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { - pub fn allocate(fs: &Filesystem<'_, Storage>) -> FileAllocation { - FileAllocation::new(fs.cache_size) + pub fn allocate() -> FileAllocation { + FileAllocation::new() } /// Returns a new OpenOptions object. @@ -792,7 +791,10 @@ impl OpenOptions { alloc: &mut FileAllocation, path: &Path, ) -> Result> { - assert_eq!(fs.cache_size, alloc.cache_size); + alloc.cache.get_mut().set_len(fs.cache_size).map_err(|_| { + error_now!("Buffer is not large enough for cache size of {cache_size}"); + Error::NO_MEMORY + })?; alloc.config.buffer = alloc.cache.get_mut().as_mut_ptr() as *mut _; // We need to use addr_of_mut! here instead of & mut since // the FFI stores a copy of a pointer to the field state, @@ -820,7 +822,7 @@ impl OpenOptions { path: &Path, f: impl FnOnce(&File<'a, '_, S>) -> Result, ) -> Result { - let mut alloc = File::allocate(fs); // lifetime 'c + let mut alloc = File::allocate(); // lifetime 'c let mut file = unsafe { self.open(fs, &mut alloc, path)? }; // Q: what is the actually correct behaviour? // E.g. if res is Ok but closing gives an error. @@ -1086,16 +1088,51 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { pub fn mount(alloc: &'a mut Allocation, storage: &'a mut Storage) -> Result { - let fs = Self::new(alloc, storage); + let fs = Self::new(alloc, storage)?; fs.raw_mount()?; Ok(fs) } - fn set_alloc_config(alloc: &mut Allocation, storage: &mut Storage) { + fn set_alloc_config(alloc: &mut Allocation, storage: &mut Storage) -> Result { + let cache_size = storage.cache_size(); + let lookahead_size = storage.lookahead_size(); + + alloc + .cache + .read + .get_mut() + .set_len(cache_size) + .map_err(|_| { + error_now!("Buffer is not large enough for cache size of {cache_size}"); + Error::NO_MEMORY + })?; + alloc + .cache + .write + .get_mut() + .set_len(cache_size) + .map_err(|_| { + error_now!("Buffer is not large enough for cache size of {cache_size}"); + Error::NO_MEMORY + })?; + alloc + .cache + .lookahead + .get_mut() + .set_len(lookahead_size) + .map_err(|_| { + error_now!("Buffer is not large enough for lookahead size of {lookahead_size}"); + Error::NO_MEMORY + })?; + alloc.config.context = storage as *mut _ as *mut c_void; - alloc.config.read_buffer = alloc.cache.read.get() as *mut c_void; - alloc.config.prog_buffer = alloc.cache.write.get() as *mut c_void; - alloc.config.lookahead_buffer = alloc.cache.lookahead.get() as *mut c_void; + + alloc.config.read_buffer = alloc.cache.read.get_mut().as_mut_ptr() as *mut c_void; + alloc.config.prog_buffer = alloc.cache.write.get_mut().as_mut_ptr() as *mut c_void; + alloc.config.lookahead_buffer = alloc.cache.lookahead.get_mut().as_mut_ptr() as *mut c_void; + alloc.config.cache_size = cache_size as _; + alloc.config.lookahead_size = lookahead_size as _; + Ok(cache_size) } /// Mount the filesystem or, if that fails, call `f` with the mount error and the storage and then try again. @@ -1107,11 +1144,11 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { where F: FnOnce(Error, &mut Storage, &mut Allocation) -> Result<()>, { - let mut fs = Self::new(alloc, storage); + let mut fs = Self::new(alloc, storage)?; if let Err(err) = fs.raw_mount() { let alloc = fs.alloc.get_mut(); f(err, fs.storage, alloc)?; - Self::set_alloc_config(alloc, fs.storage); + Self::set_alloc_config(alloc, fs.storage)?; fs.raw_mount()?; } Ok(fs) @@ -1125,13 +1162,13 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { } // Not public, user should use `mount`, possibly after `format` - fn new(alloc: &'a mut Allocation, storage: &'a mut Storage) -> Self { - Self::set_alloc_config(alloc, storage); - Filesystem { - cache_size: alloc.cache.size, + fn new(alloc: &'a mut Allocation, storage: &'a mut Storage) -> Result { + let cache_size = Self::set_alloc_config(alloc, storage)?; + Ok(Filesystem { + cache_size, alloc: RefCell::new(alloc), storage, - } + }) } /// Deconstruct `Filesystem`, intention is to allow access to @@ -1517,11 +1554,11 @@ mod tests { Ok(()) })?; - let mut a1 = File::allocate(fs); + let mut a1 = File::allocate(); let f1 = unsafe { File::create(fs, &mut a1, b"a.txt\0".try_into().unwrap())? }; f1.write(b"some text")?; - let mut a2 = File::allocate(fs); + let mut a2 = File::allocate(); let f2 = unsafe { File::create(fs, &mut a2, b"b.txt\0".try_into().unwrap())? }; f2.write(b"more text")?; From d0ebffac28f264990e8eedea98074005f318f0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 11 Mar 2025 17:08:54 +0100 Subject: [PATCH 3/8] Allow showing hex content of littlefs files --- Cargo.toml | 3 ++- examples/list.rs | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 43c27579..17169b6c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ all-features = true [[example]] name = "list" -required-features = ["alloc"] +required-features = ["alloc", "littlefs2-core/heapless07"] [dependencies] bitflags = "2.9.0" @@ -37,6 +37,7 @@ littlefs2-sys = { version = "0.3.1", features = ["multiversion"] } ssmarshal = "1" serde = { version = "1.0", default-features = false, features = ["derive"] } clap = { version = "4.5.31", features = ["derive"] } +hex = "0.4.3" # trybuild = "1" [features] diff --git a/examples/list.rs b/examples/list.rs index 7615e72c..94ca0b9a 100644 --- a/examples/list.rs +++ b/examples/list.rs @@ -27,6 +27,8 @@ struct Args { lookahead_size: Option, #[arg(short, long)] block_count: Option, + #[arg(short, long)] + show_hex: bool, } const BLOCK_COUNT: usize = 288; @@ -64,21 +66,26 @@ fn main() { println!(); let path = PathBuf::new(); - list(&fs, &path); + list(&fs, &path, args.show_hex); } -fn list(fs: &dyn DynFilesystem, path: &Path) { +fn list(fs: &dyn DynFilesystem, path: &Path, show_hex: bool) { fs.read_dir_and_then(path, &mut |iter| { for entry in iter { let entry = entry.unwrap(); match entry.file_type() { - FileType::File => println!("F {}", entry.path()), + FileType::File => { + println!("F {}", entry.path()); + if show_hex { + let bytes: heapless::Vec = fs.read(entry.path()).unwrap(); + println!(" {}", hex::encode_upper(&bytes)); + } + } FileType::Dir => match entry.file_name().as_str() { "." => (), ".." => (), _ => { - println!("D {}", entry.path()); - list(fs, entry.path()); + list(fs, entry.path(), show_hex); } }, } From 95d53c8f80349df991cd268c0cff0db36e760bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Mon, 16 Jun 2025 12:29:38 +0200 Subject: [PATCH 4/8] Remove the use of `RefCell` in `File` The previous `RefCell` was only dealing with a pointer and the data was always held by the lfs side so the checking from the refcell was useless anyways This also contains a breaking change because the `unsafe` `open` function did not take the correct lifetime. It think it's an acceptable breaking change because the previous behaviour was buggy and any code that depends on it likely has a use after free. --- src/fs.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 818644bd..9405382c 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,6 +1,7 @@ //! Experimental Filesystem version using closures. use core::ffi::{c_int, c_void}; +use core::marker::PhantomData; use core::ptr::addr_of; use core::ptr::addr_of_mut; use core::{ @@ -597,7 +598,8 @@ impl Default for FileAllocation { pub struct File<'a, 'b, S: driver::Storage> { // We must store a raw pointer here since the FFI retains a copy of a pointer // to the field alloc.state, so we cannot assert unique mutable access. - alloc: RefCell<*mut FileAllocation>, + alloc: *mut FileAllocation, + phantom: PhantomData<&'b mut FileAllocation>, fs: &'b Filesystem<'a, S>, } @@ -672,7 +674,7 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { // We need to use addr_of_mut! here instead of & mut since // the FFI stores a copy of a pointer to the field state, // so we cannot assert unique mutable access. - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), ); result_from((), return_code) } @@ -685,7 +687,7 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { // so we cannot assert unique mutable access. ll::lfs_file_sync( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), ) }; result_from((), return_code) @@ -699,7 +701,7 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { // so we cannot assert unique mutable access. ll::lfs_file_size( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), ) }; u32_result(return_code).map(|n| n as usize) @@ -721,7 +723,7 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { // so we cannot assert unique mutable access. ll::lfs_file_truncate( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), size as u32, ) }; @@ -788,7 +790,7 @@ impl OpenOptions { pub unsafe fn open<'a, 'b, S: driver::Storage>( &self, fs: &'b Filesystem<'a, S>, - alloc: &mut FileAllocation, + alloc: &'b mut FileAllocation, path: &Path, ) -> Result> { alloc.cache.get_mut().set_len(fs.cache_size).map_err(|_| { @@ -808,7 +810,8 @@ impl OpenOptions { ); let file = File { - alloc: RefCell::new(alloc), + alloc, + phantom: PhantomData, fs, }; @@ -908,7 +911,7 @@ impl io::Read for File<'_, '_, S> { // so we cannot assert unique mutable access. ll::lfs_file_read( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), buf.as_mut_ptr() as *mut c_void, buf.len() as u32, ) @@ -925,7 +928,7 @@ impl io::Seek for File<'_, '_, S> { // so we cannot assert unique mutable access. ll::lfs_file_seek( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), pos.off(), pos.whence(), ) @@ -942,7 +945,7 @@ impl io::Write for File<'_, '_, S> { // so we cannot assert unique mutable access. ll::lfs_file_write( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), buf.as_ptr() as *const c_void, buf.len() as u32, ) @@ -974,7 +977,8 @@ impl ReadDirAllocation { pub struct ReadDir<'a, 'b, S: driver::Storage> { // We must store a raw pointer here since the FFI retains a copy of a pointer // to the field alloc.state, so we cannot assert unique mutable access. - alloc: RefCell<*mut ReadDirAllocation>, + alloc: *mut ReadDirAllocation, + phantom: PhantomData<&'b mut ReadDirAllocation>, fs: &'b Filesystem<'a, S>, path: &'b Path, } @@ -992,7 +996,7 @@ impl Iterator for ReadDir<'_, '_, S> { let return_code = unsafe { ll::lfs_dir_read( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), &mut info, ) }; @@ -1037,7 +1041,7 @@ impl ReadDir<'_, '_, S> { // so we cannot assert unique mutable access. ll::lfs_dir_close( &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*(*self.alloc.borrow_mut())).state), + addr_of_mut!((*self.alloc).state), ) }; result_from((), return_code) @@ -1077,7 +1081,8 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { ); let read_dir = ReadDir { - alloc: RefCell::new(alloc), + alloc, + phantom: PhantomData, fs: self, path, }; From d5317124f81e5e7a9178dbe7652a3619529b2a80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Mon, 16 Jun 2025 12:48:05 +0200 Subject: [PATCH 5/8] Make `Filesystem` `Send` `Filesystem` is still `!Sync`, so this allows putting the filesystem behind a `Mutex` and sharing it across threads but still prevents concurrent operations from multiple threads. See https://github.com/trussed-dev/littlefs2/issues/108#issuecomment-2975793283 --- src/fs.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/fs.rs b/src/fs.rs index 9405382c..b7762de7 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -62,6 +62,19 @@ pub struct Allocation { state: ll::lfs_t, } +/// # Safety +/// +/// All operations are done on `&mut Allocation, and the reference is held +/// during the entire lifetime of the filesystem, so once the reference is +/// available again, the filesystem is closed +unsafe impl Sync for Allocation {} +/// # Safety +/// +/// All operations are done on `&mut Allocation, and the reference is held +/// during the entire lifetime of the filesystem, so once the reference is +/// available again, the filesystem is closed +unsafe impl Send for Allocation {} + #[derive(Default, Clone, Debug)] #[non_exhaustive] pub struct Config { @@ -1574,4 +1587,21 @@ mod tests { }) .unwrap(); } + + #[allow(unreachable_code)] + fn _filesystem_is_sync() { + fn assert_is_sync(_: &T) -> R { + todo!() + } + fn assert_is_send(_: &T) -> R { + todo!() + } + + assert_is_sync::, ()>(todo!()); + assert_is_send::<&mut Allocation, ()>(todo!()); + assert_is_send::>, ()>(todo!()); + + let mut test_storage = TestStorage::new(); + Filesystem::mount_and_then(&mut test_storage, |fs| assert_is_send(fs)).unwrap() + } } From 5c1e43fb6efe7d72f361992b812e382fc7faa0bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Thu, 19 Jun 2025 14:45:17 +0200 Subject: [PATCH 6/8] Make use of `Pin` to make the main API safe --- Cargo.toml | 1 + src/fs.rs | 175 ++++++++++++++++++++++++++++----------------- src/object_safe.rs | 2 +- 3 files changed, 111 insertions(+), 67 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 17169b6c..879cbc03 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ delog = "0.1.0" heapless = "0.7" littlefs2-core = { version = "0.1", path = "core" } littlefs2-sys = { version = "0.3.1", features = ["multiversion"] } +pin-project = "1.1.10" [dev-dependencies] ssmarshal = "1" diff --git a/src/fs.rs b/src/fs.rs index b7762de7..d0f6707f 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -2,6 +2,7 @@ use core::ffi::{c_int, c_void}; use core::marker::PhantomData; +use core::pin::{pin, Pin}; use core::ptr::addr_of; use core::ptr::addr_of_mut; use core::{ @@ -9,6 +10,7 @@ use core::{ mem, slice, }; use littlefs2_sys as ll; +use pin_project::{pin_project, pinned_drop}; pub use littlefs2_core::{Attribute, DirEntry, FileOpenFlags, FileType, Metadata}; @@ -217,7 +219,7 @@ struct RemoveDirAllProgress { skipped_any: bool, } -impl Filesystem<'_, Storage> { +impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { pub fn allocate(storage: &Storage) -> Allocation { Allocation::new(storage) } @@ -445,7 +447,7 @@ impl Filesystem<'_, Storage> { pub fn create_file_and_then( &self, path: &Path, - f: impl FnOnce(&File<'_, '_, Storage>) -> Result, + f: impl FnOnce(&File<'_, '_, '_, Storage>) -> Result, ) -> Result { File::create_and_then(self, path, f) } @@ -453,7 +455,7 @@ impl Filesystem<'_, Storage> { pub fn open_file_and_then( &self, path: &Path, - f: impl FnOnce(&File<'_, '_, Storage>) -> Result, + f: impl FnOnce(&File<'_, '_, '_, Storage>) -> Result, ) -> Result { File::open_and_then(self, path, f) } @@ -462,23 +464,23 @@ impl Filesystem<'_, Storage> { OpenOptions::new() } - pub fn open_file_with_options_and_then( - &self, + pub fn open_file_with_options_and_then<'b, R>( + &'b self, o: impl FnOnce(&mut OpenOptions) -> &OpenOptions, path: &Path, - f: impl FnOnce(&File<'_, '_, Storage>) -> Result, + f: impl FnOnce(&File<'a, 'b, '_, Storage>) -> Result, ) -> Result { let mut options = OpenOptions::new(); o(&mut options).open_and_then(self, path, f) } /// Read attribute. - pub fn attribute<'a>( + pub fn attribute<'b>( &self, path: &Path, id: u8, - buffer: &'a mut [u8], - ) -> Result>> { + buffer: &'b mut [u8], + ) -> Result>> { let n = u32::try_from(buffer.len()).unwrap_or(u32::MAX); let return_code = unsafe { @@ -585,39 +587,63 @@ impl Filesystem<'_, Storage> { } } +#[pin_project(PinnedDrop)] /// The state of a `File`. Pre-allocate with `File::allocate`. -pub struct FileAllocation { +pub struct FileAllocation<'a, 'b, S: driver::Storage> { cache: UnsafeCell, state: ll::lfs_file_t, config: ll::lfs_file_config, + // If filesystem is Some, the file was opened + filesystem: Option<&'b Filesystem<'a, S>>, } -impl FileAllocation { +impl<'a, 'b, S: driver::Storage> FileAllocation<'a, 'b, S> { pub fn new() -> Self { Self { cache: UnsafeCell::new(S::CACHE_BUFFER::empty()), state: unsafe { mem::MaybeUninit::zeroed().assume_init() }, config: unsafe { mem::MaybeUninit::zeroed().assume_init() }, + filesystem: None, } } } -impl Default for FileAllocation { +impl<'a, 'b, S: driver::Storage> FileAllocation<'a, 'b, S> { + fn close(self: Pin<&mut Self>) -> Result<()> { + let this = self.project(); + let Some(fs) = this.filesystem.take() else { + return Ok(()); + }; + + let return_code = unsafe { + ll::lfs_file_close( + &mut fs.alloc.borrow_mut().state, + // We need to use addr_of_mut! here instead of & mut since + // the FFI stores a copy of a pointer to the field state, + // so we cannot assert unique mutable access. + addr_of_mut!(*this.state), + ) + }; + result_from((), return_code) + } +} + +impl<'a, 'b, S: driver::Storage> Default for FileAllocation<'a, 'b, S> { fn default() -> Self { Self::new() } } -pub struct File<'a, 'b, S: driver::Storage> { +pub struct File<'a, 'b, 'c, S: driver::Storage> { // We must store a raw pointer here since the FFI retains a copy of a pointer // to the field alloc.state, so we cannot assert unique mutable access. - alloc: *mut FileAllocation, - phantom: PhantomData<&'b mut FileAllocation>, + alloc: *mut FileAllocation<'a, 'b, S>, + phantom: PhantomData>>, fs: &'b Filesystem<'a, S>, } -impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { - pub fn allocate() -> FileAllocation { +impl<'a, 'b, 'c, Storage: driver::Storage> File<'a, 'b, 'c, Storage> { + pub fn allocate() -> FileAllocation<'a, 'b, Storage> { FileAllocation::new() } @@ -632,25 +658,25 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { OpenOptions::new() } - pub unsafe fn open( + pub fn open( fs: &'b Filesystem<'a, Storage>, - alloc: &'b mut FileAllocation, + alloc: Pin<&'c mut FileAllocation<'a, 'b, Storage>>, path: &Path, ) -> Result { OpenOptions::new().read(true).open(fs, alloc, path) } pub fn open_and_then( - fs: &Filesystem<'a, Storage>, + fs: &'b Filesystem<'a, Storage>, path: &Path, - f: impl FnOnce(&File<'_, '_, Storage>) -> Result, + f: impl FnOnce(&File<'a, 'b, '_, Storage>) -> Result, ) -> Result { OpenOptions::new().read(true).open_and_then(fs, path, f) } - pub unsafe fn create( + pub fn create( fs: &'b Filesystem<'a, Storage>, - alloc: &'b mut FileAllocation, + alloc: Pin<&'c mut FileAllocation<'a, 'b, Storage>>, path: &Path, ) -> Result { OpenOptions::new() @@ -661,9 +687,9 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { } pub fn create_and_then( - fs: &Filesystem<'a, Storage>, + fs: &'b Filesystem<'a, Storage>, path: &Path, - f: impl FnOnce(&File<'_, '_, Storage>) -> Result, + f: impl FnOnce(&File<'a, '_, '_, Storage>) -> Result, ) -> Result { OpenOptions::new() .write(true) @@ -673,7 +699,7 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { } // Safety-hatch to experiment with missing parts of API - pub unsafe fn borrow_filesystem<'c>(&'c mut self) -> &'c Filesystem<'a, Storage> { + pub unsafe fn borrow_filesystem(&mut self) -> &Filesystem<'a, Storage> { self.fs } @@ -681,15 +707,9 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { /// Not doing this is UB, which is why we have all the closure-based APIs. /// /// This must not be called twice. - pub unsafe fn close(self) -> Result<()> { - let return_code = ll::lfs_file_close( - &mut self.fs.alloc.borrow_mut().state, - // We need to use addr_of_mut! here instead of & mut since - // the FFI stores a copy of a pointer to the field state, - // so we cannot assert unique mutable access. - addr_of_mut!((*self.alloc).state), - ); - result_from((), return_code) + pub fn close(self) -> Result<()> { + // Safety: alloc was gotten pinned + unsafe { Pin::new_unchecked(&mut *self.alloc) }.close() } /// Synchronize file contents to storage. @@ -772,6 +792,13 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> { } } +#[pinned_drop] +impl<'a, 'b, S: driver::Storage> PinnedDrop for FileAllocation<'a, 'b, S> { + fn drop(self: Pin<&mut Self>) { + self.close().ok(); + } +} + /// Options and flags which can be used to configure how a file is opened. /// /// This builder exposes the ability to configure how a File is opened and what operations @@ -800,30 +827,43 @@ impl OpenOptions { /// closing removes them from there /// - since littlefs is supposed to be *fail-safe*, we can't just close files in /// Drop and panic if something went wrong. - pub unsafe fn open<'a, 'b, S: driver::Storage>( + pub fn open<'a, 'b, 'c, S: driver::Storage>( &self, fs: &'b Filesystem<'a, S>, - alloc: &'b mut FileAllocation, + mut alloc: Pin<&'c mut FileAllocation<'a, 'b, S>>, path: &Path, - ) -> Result> { - alloc.cache.get_mut().set_len(fs.cache_size).map_err(|_| { - error_now!("Buffer is not large enough for cache size of {cache_size}"); - Error::NO_MEMORY - })?; - alloc.config.buffer = alloc.cache.get_mut().as_mut_ptr() as *mut _; + ) -> Result> { + let alloc_proj = alloc.as_mut().project(); + alloc_proj + .cache + .get_mut() + .set_len(fs.cache_size) + .map_err(|_| { + error_now!("Buffer is not large enough for cache size of {cache_size}"); + Error::NO_MEMORY + })?; + alloc_proj.config.buffer = alloc_proj.cache.get_mut().as_mut_ptr() as *mut _; + // We need to use addr_of_mut! here instead of & mut since // the FFI stores a copy of a pointer to the field state, // so we cannot assert unique mutable access. - let return_code = ll::lfs_file_opencfg( - &mut fs.alloc.borrow_mut().state, - addr_of_mut!(alloc.state), - path.as_ptr(), - self.0.bits(), - addr_of!(alloc.config), - ); + let return_code = unsafe { + ll::lfs_file_opencfg( + &mut fs.alloc.borrow_mut().state, + addr_of_mut!(*alloc_proj.state), + path.as_ptr(), + self.0.bits(), + addr_of!(*alloc_proj.config), + ) + }; + + // The file was opened + if return_code >= 0 { + *alloc_proj.filesystem = Some(fs); + } let file = File { - alloc, + alloc: unsafe { alloc.get_unchecked_mut() }, phantom: PhantomData, fs, }; @@ -832,20 +872,21 @@ impl OpenOptions { } /// (Hopefully) safe abstraction around `open`. - pub fn open_and_then<'a, R, S: driver::Storage>( + pub fn open_and_then<'a, 'b, R, S: driver::Storage>( &self, - fs: &Filesystem<'a, S>, + fs: &'b Filesystem<'a, S>, path: &Path, - f: impl FnOnce(&File<'a, '_, S>) -> Result, + f: impl FnOnce(&File<'a, 'b, '_, S>) -> Result, ) -> Result { - let mut alloc = File::allocate(); // lifetime 'c - let mut file = unsafe { self.open(fs, &mut alloc, path)? }; + let alloc = File::allocate(); + let alloc = pin!(alloc); + let mut file = self.open(fs, alloc, path)?; // Q: what is the actually correct behaviour? // E.g. if res is Ok but closing gives an error. // Or if closing fails because something is broken and // we'd already know that from an Err res. let res = f(&mut file); - unsafe { file.close()? }; + file.close()?; res } @@ -916,7 +957,7 @@ impl From for OpenOptions { } } -impl io::Read for File<'_, '_, S> { +impl io::Read for File<'_, '_, '_, S> { fn read(&self, buf: &mut [u8]) -> Result { let return_code = unsafe { // We need to use addr_of_mut! here instead of & mut since @@ -933,7 +974,7 @@ impl io::Read for File<'_, '_, S> { } } -impl io::Seek for File<'_, '_, S> { +impl io::Seek for File<'_, '_, '_, S> { fn seek(&self, pos: io::SeekFrom) -> Result { let return_code = unsafe { // We need to use addr_of_mut! here instead of & mut since @@ -950,7 +991,7 @@ impl io::Seek for File<'_, '_, S> { } } -impl io::Write for File<'_, '_, S> { +impl io::Write for File<'_, '_, '_, S> { fn write(&self, buf: &[u8]) -> Result { let return_code = unsafe { // We need to use addr_of_mut! here instead of & mut since @@ -1572,16 +1613,18 @@ mod tests { Ok(()) })?; - let mut a1 = File::allocate(); - let f1 = unsafe { File::create(fs, &mut a1, b"a.txt\0".try_into().unwrap())? }; + let a1 = File::allocate(); + let a1 = pin!(a1); + let f1 = File::create(fs, a1, b"a.txt\0".try_into().unwrap())?; f1.write(b"some text")?; - let mut a2 = File::allocate(); - let f2 = unsafe { File::create(fs, &mut a2, b"b.txt\0".try_into().unwrap())? }; + let a2 = File::allocate(); + let a2 = pin!(a2); + let f2 = File::create(fs, a2, b"b.txt\0".try_into().unwrap())?; f2.write(b"more text")?; - unsafe { f1.close()? }; // program hangs here - unsafe { f2.close()? }; // this statement is never reached + f1.close()?; // program hangs here + f2.close()?; // this statement is never reached Ok(()) }) diff --git a/src/object_safe.rs b/src/object_safe.rs index 23635e35..4ac4ab41 100644 --- a/src/object_safe.rs +++ b/src/object_safe.rs @@ -17,7 +17,7 @@ pub type FilesystemCallback<'a, R = ()> = &'a mut dyn FnMut(&dyn DynFilesystem) pub type FilesystemCallbackOnce<'a, R = ()> = alloc::boxed::Box Result + 'a>; -impl DynFile for File<'_, '_, S> { +impl DynFile for File<'_, '_, '_, S> { fn sync(&self) -> Result<()> { File::sync(self) } From a913f1b902501563ca89ecdbc16c9e48082f7225 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Thu, 19 Jun 2025 15:58:08 +0200 Subject: [PATCH 7/8] Make read_dir api safe --- src/fs.rs | 90 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 32 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index d0f6707f..0ea2fba1 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -708,7 +708,7 @@ impl<'a, 'b, 'c, Storage: driver::Storage> File<'a, 'b, 'c, Storage> { /// /// This must not be called twice. pub fn close(self) -> Result<()> { - // Safety: alloc was gotten pinned + // Safety: alloc was obtained pinned unsafe { Pin::new_unchecked(&mut *self.alloc) }.close() } @@ -1012,32 +1012,58 @@ impl io::Write for File<'_, '_, '_, S> { } } -pub struct ReadDirAllocation { +#[pin_project(PinnedDrop)] +pub struct ReadDirAllocation<'a, 'b, S: driver::Storage> { state: ll::lfs_dir_t, + fs: Option<&'b Filesystem<'a, S>>, } -impl Default for ReadDirAllocation { +#[pinned_drop] +impl PinnedDrop for ReadDirAllocation<'_, '_, S> { + fn drop(self: Pin<&mut Self>) { + self.close().ok(); + } +} + +impl Default for ReadDirAllocation<'_, '_, S> { fn default() -> Self { Self::new() } } -impl ReadDirAllocation { +impl ReadDirAllocation<'_, '_, S> { pub fn new() -> Self { - unsafe { mem::MaybeUninit::zeroed().assume_init() } + Self { + state: unsafe { mem::MaybeUninit::zeroed().assume_init() }, + fs: None, + } + } + + fn close(self: Pin<&mut Self>) -> Result<()> { + let this = self.project(); + + let Some(fs) = this.fs else { return Ok(()) }; + + let return_code = unsafe { + // We need to use addr_of_mut! here instead of & mut since + // the FFI stores a copy of a pointer to the field state, + // so we cannot assert unique mutable access. + ll::lfs_dir_close(&mut fs.alloc.borrow_mut().state, addr_of_mut!(*this.state)) + }; + result_from((), return_code) } } -pub struct ReadDir<'a, 'b, S: driver::Storage> { +pub struct ReadDir<'a, 'b, 'c, S: driver::Storage> { // We must store a raw pointer here since the FFI retains a copy of a pointer // to the field alloc.state, so we cannot assert unique mutable access. - alloc: *mut ReadDirAllocation, - phantom: PhantomData<&'b mut ReadDirAllocation>, + alloc: *mut ReadDirAllocation<'a, 'b, S>, + phantom: PhantomData>>, fs: &'b Filesystem<'a, S>, path: &'b Path, } -impl Iterator for ReadDir<'_, '_, S> { +impl Iterator for ReadDir<'_, '_, '_, S> { type Item = Result; // remove this allowance again, once path overflow is properly handled @@ -1073,14 +1099,14 @@ impl Iterator for ReadDir<'_, '_, S> { } } -impl<'a, S: driver::Storage> ReadDir<'a, '_, S> { +impl<'a, S: driver::Storage> ReadDir<'a, '_, '_, S> { // Safety-hatch to experiment with missing parts of API pub unsafe fn borrow_filesystem<'b>(&'b mut self) -> &'b Filesystem<'a, S> { self.fs } } -impl ReadDir<'_, '_, S> { +impl ReadDir<'_, '_, '_, S> { // Again, not sure if this can be called twice // Update: This one seems to be safe to call multiple times, // it just goes through the "mlist" and removes itself. @@ -1089,28 +1115,21 @@ impl ReadDir<'_, '_, S> { // have an (unsafely genereated) ReadDir with that handle; on the other hand // as long as ReadDir is not Copy. pub fn close(self) -> Result<()> { - let return_code = unsafe { - // We need to use addr_of_mut! here instead of & mut since - // the FFI stores a copy of a pointer to the field state, - // so we cannot assert unique mutable access. - ll::lfs_dir_close( - &mut self.fs.alloc.borrow_mut().state, - addr_of_mut!((*self.alloc).state), - ) - }; - result_from((), return_code) + unsafe { Pin::new_unchecked(&mut *self.alloc) }.close() } } +impl ReadDir<'_, '_, '_, S> {} impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { pub fn read_dir_and_then( &self, path: &Path, // *not* &ReadDir, as Iterator takes &mut - f: impl FnOnce(&mut ReadDir<'_, '_, Storage>) -> Result, + f: impl FnOnce(&mut ReadDir<'_, '_, '_, Storage>) -> Result, ) -> Result { let mut alloc = ReadDirAllocation::new(); - let mut read_dir = unsafe { self.read_dir(&mut alloc, path)? }; + let alloc = pin!(alloc); + let mut read_dir = self.read_dir(alloc, path)?; let res = f(&mut read_dir); // unsafe { read_dir.close()? }; read_dir.close()?; @@ -1120,22 +1139,29 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { /// Returns a pseudo-iterator over the entries within a directory. /// /// This is unsafe since it can induce UB just like File::open. - pub unsafe fn read_dir<'b>( + pub fn read_dir<'b, 'c>( &'b self, - alloc: &'b mut ReadDirAllocation, + mut alloc: Pin<&'c mut ReadDirAllocation<'a, 'b, Storage>>, path: &'b Path, - ) -> Result> { + ) -> Result> { + let alloc_proj = alloc.as_mut().project(); // ll::lfs_dir_open stores a copy of the pointer to alloc.state, so // we must use addr_of_mut! here, since &mut alloc.state asserts unique // mutable access, and we need shared mutable access. - let return_code = ll::lfs_dir_open( - &mut self.alloc.borrow_mut().state, - addr_of_mut!(alloc.state), - path.as_ptr(), - ); + let return_code = unsafe { + ll::lfs_dir_open( + &mut self.alloc.borrow_mut().state, + addr_of_mut!(*alloc_proj.state), + path.as_ptr(), + ) + }; + + if return_code >= 0 { + *alloc_proj.fs = Some(self); + } let read_dir = ReadDir { - alloc, + alloc: unsafe { alloc.get_unchecked_mut() }, phantom: PhantomData, fs: self, path, From 15816dfb5182898520bde639db1b79d94b86f44e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Thu, 19 Jun 2025 16:29:11 +0200 Subject: [PATCH 8/8] Make allocation `!Unpin` --- src/fs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 0ea2fba1..5195478d 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -587,7 +587,7 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { } } -#[pin_project(PinnedDrop)] +#[pin_project(PinnedDrop, !Unpin)] /// The state of a `File`. Pre-allocate with `File::allocate`. pub struct FileAllocation<'a, 'b, S: driver::Storage> { cache: UnsafeCell, @@ -1012,7 +1012,7 @@ impl io::Write for File<'_, '_, '_, S> { } } -#[pin_project(PinnedDrop)] +#[pin_project(PinnedDrop, !Unpin)] pub struct ReadDirAllocation<'a, 'b, S: driver::Storage> { state: ll::lfs_dir_t, fs: Option<&'b Filesystem<'a, S>>,