From 1eb3da2a4fa7204177135f1e67e1a1ac6de271f0 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Thu, 23 Jan 2025 20:32:22 +0100 Subject: [PATCH 1/4] Simplify Store trait This patch simplifies the Store trait by removing the Fs wrapper struct, the Storage types, the static lifetime and the Copy requirement and replaces it with a reference to a DynFilesystem. This gives runners more options on how to implement the store. As the Store trait can now easily be implemented in a safe way, this patch also removes the unsafe keyword from the trait definition. --- CHANGELOG.md | 6 ++ src/store.rs | 127 +++++++++++++++----------------------- src/store/certstore.rs | 6 +- src/store/counterstore.rs | 4 +- src/store/filestore.rs | 22 +++---- src/store/keystore.rs | 12 ++-- src/virt/store.rs | 5 +- 7 files changed, 81 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f651814bc3..e73a6a8ab2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added support for non-static channels: - Added lifetimes to `ClientImplementation` and `ServiceEndpoints`. - Added the `pipe::TrussedChannel` type. +- Refactored the `Store` trait: + - Removed the requirement for a static lifetime. + - Removed the `Fs` wrapper type. + - Removed the storage types to return `&dyn DynFilesystem` instead. + - Removed the `Copy` requirement. + - Removed the `unsafe` keyword for the `Store` trait. ### Fixed diff --git a/src/store.rs b/src/store.rs index 43e239efafc..9f5a435d5c5 100644 --- a/src/store.rs +++ b/src/store.rs @@ -72,7 +72,7 @@ //! - Alternative: subdirectory <==> RP hash, everything else in flat files //! - In any case need to "list dirs excluding . and .." or similar -use littlefs2::{driver::Storage, fs::Filesystem, path}; +use littlefs2::path; use crate::error::Error; use crate::types::{Bytes, Location}; @@ -127,39 +127,19 @@ pub mod keystore; // LfsStorage-bound types) to remove lifetimes and generic parameters from Store. // // This makes everything using it *much* more ergonomic. -pub unsafe trait Store: Copy { - type I: 'static + Storage; - type E: 'static + Storage; - type V: 'static + Storage; - fn ifs(self) -> &'static Fs; - fn efs(self) -> &'static Fs; - fn vfs(self) -> &'static Fs; +pub trait Store { + fn ifs(&self) -> &dyn DynFilesystem; + fn efs(&self) -> &dyn DynFilesystem; + fn vfs(&self) -> &dyn DynFilesystem; fn fs(&self, location: Location) -> &dyn DynFilesystem { match location { - Location::Internal => self.ifs().fs, - Location::External => self.efs().fs, - Location::Volatile => self.vfs().fs, + Location::Internal => self.ifs(), + Location::External => self.efs(), + Location::Volatile => self.vfs(), } } } -pub struct Fs { - fs: &'static Filesystem<'static, S>, -} - -impl core::ops::Deref for Fs { - type Target = Filesystem<'static, S>; - fn deref(&self) -> &Self::Target { - self.fs - } -} - -impl Fs { - pub fn new(fs: &'static Filesystem<'static, S>) -> Self { - Self { fs } - } -} - #[macro_export] macro_rules! store { ( @@ -174,19 +154,15 @@ macro_rules! store { __: core::marker::PhantomData<*mut ()>, } - unsafe impl $crate::store::Store for $store { - type I = $Ifs; - type E = $Efs; - type V = $Vfs; - - fn ifs(self) -> &'static $crate::store::Fs<$Ifs> { - unsafe { &*Self::ifs_ptr() } + impl $crate::store::Store for $store { + fn ifs(&self) -> &dyn littlefs2::object_safe::DynFilesystem { + unsafe { Self::ifs_ptr().assume_init() } } - fn efs(self) -> &'static $crate::store::Fs<$Efs> { - unsafe { &*Self::efs_ptr() } + fn efs(&self) -> &dyn littlefs2::object_safe::DynFilesystem { + unsafe { Self::efs_ptr().assume_init() } } - fn vfs(self) -> &'static $crate::store::Fs<$Vfs> { - unsafe { &*Self::vfs_ptr() } + fn vfs(&self) -> &dyn littlefs2::object_safe::DynFilesystem { + unsafe { Self::vfs_ptr().assume_init() } } } @@ -277,14 +253,9 @@ macro_rules! store { efs: &'static littlefs2::fs::Filesystem<$Efs>, vfs: &'static littlefs2::fs::Filesystem<$Vfs>, ) -> Self { - let store_ifs = $crate::store::Fs::new(ifs); - let store_efs = $crate::store::Fs::new(efs); - let store_vfs = $crate::store::Fs::new(vfs); - unsafe { - Self::ifs_ptr().write(store_ifs); - Self::efs_ptr().write(store_efs); - Self::vfs_ptr().write(store_vfs); - } + Self::ifs_ptr().write(ifs); + Self::efs_ptr().write(efs); + Self::vfs_ptr().write(vfs); Self::claim().unwrap() } @@ -317,28 +288,31 @@ macro_rules! store { } } - fn ifs_ptr() -> *mut $crate::store::Fs<$Ifs> { + fn ifs_ptr() -> &'static mut core::mem::MaybeUninit< + &'static dyn littlefs2::object_safe::DynFilesystem, + > { use core::mem::MaybeUninit; - use core::ptr::addr_of_mut; - use $crate::store::Fs; - static mut IFS: MaybeUninit> = MaybeUninit::uninit(); - unsafe { (*addr_of_mut!(IFS)).as_mut_ptr() } + static mut IFS: MaybeUninit<&'static dyn littlefs2::object_safe::DynFilesystem> = + MaybeUninit::uninit(); + unsafe { (&raw mut IFS).as_mut().unwrap() } } - fn efs_ptr() -> *mut $crate::store::Fs<$Efs> { + fn efs_ptr() -> &'static mut core::mem::MaybeUninit< + &'static dyn littlefs2::object_safe::DynFilesystem, + > { use core::mem::MaybeUninit; - use core::ptr::addr_of_mut; - use $crate::store::Fs; - static mut EFS: MaybeUninit> = MaybeUninit::uninit(); - unsafe { (*addr_of_mut!(EFS)).as_mut_ptr() } + static mut EFS: MaybeUninit<&'static dyn littlefs2::object_safe::DynFilesystem> = + MaybeUninit::uninit(); + unsafe { (&raw mut EFS).as_mut().unwrap() } } - fn vfs_ptr() -> *mut $crate::store::Fs<$Vfs> { + fn vfs_ptr() -> &'static mut core::mem::MaybeUninit< + &'static dyn littlefs2::object_safe::DynFilesystem, + > { use core::mem::MaybeUninit; - use core::ptr::addr_of_mut; - use $crate::store::Fs; - static mut VFS: MaybeUninit> = MaybeUninit::uninit(); - unsafe { (*addr_of_mut!(VFS)).as_mut_ptr() } + static mut VFS: MaybeUninit<&'static dyn littlefs2::object_safe::DynFilesystem> = + MaybeUninit::uninit(); + unsafe { (&raw mut VFS).as_mut().unwrap() } } // Ignore lint for compatibility @@ -365,7 +339,7 @@ macro_rules! store { format: bool, ) -> littlefs2::io::Result<()> { use core::mem::MaybeUninit; - use core::ptr::{addr_of, addr_of_mut}; + use core::ptr::addr_of_mut; use littlefs2::fs::{Allocation, Filesystem}; static mut IFS_ALLOC: MaybeUninit<&'static mut Allocation<$Ifs>> = @@ -401,8 +375,7 @@ macro_rules! store { &mut *(*addr_of_mut!(IFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(IFS_STORAGE)).as_mut_ptr(), )?); - let ifs = $crate::store::Fs::new((*addr_of!(IFS)).as_ref().unwrap()); - Self::ifs_ptr().write(ifs); + Self::ifs_ptr().write(IFS.as_ref().unwrap()); (*addr_of_mut!(EFS_ALLOC)).as_mut_ptr().write(efs_alloc); (*addr_of_mut!(EFS_STORAGE)).as_mut_ptr().write(efs_storage); @@ -410,8 +383,7 @@ macro_rules! store { &mut *(*addr_of_mut!(EFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(EFS_STORAGE)).as_mut_ptr(), )?); - let efs = $crate::store::Fs::new((*addr_of_mut!(EFS)).as_ref().unwrap()); - Self::efs_ptr().write(efs); + Self::efs_ptr().write(EFS.as_ref().unwrap()); (*addr_of_mut!(VFS_ALLOC)).as_mut_ptr().write(vfs_alloc); (*addr_of_mut!(VFS_STORAGE)).as_mut_ptr().write(vfs_storage); @@ -419,8 +391,7 @@ macro_rules! store { &mut *(*addr_of_mut!(VFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(VFS_STORAGE)).as_mut_ptr(), )?); - let vfs = $crate::store::Fs::new((*addr_of!(VFS)).as_ref().unwrap()); - Self::vfs_ptr().write(vfs); + Self::vfs_ptr().write(VFS.as_ref().unwrap()); Ok(()) } @@ -509,7 +480,7 @@ pub fn create_directories(fs: &dyn DynFilesystem, path: &Path) -> Result<(), Err /// Reads contents from path in location of store. #[inline(never)] pub fn read( - store: impl Store, + store: &impl Store, location: Location, path: &Path, ) -> Result, Error> { @@ -523,7 +494,7 @@ pub fn read( /// Writes contents to path in location of store. #[inline(never)] pub fn write( - store: impl Store, + store: &impl Store, location: Location, path: &Path, contents: &[u8], @@ -538,7 +509,7 @@ pub fn write( /// Creates parent directory if necessary, then writes. #[inline(never)] pub fn store( - store: impl Store, + store: &impl Store, location: Location, path: &Path, contents: &[u8], @@ -552,7 +523,7 @@ pub fn store( } #[inline(never)] -pub fn delete(store: impl Store, location: Location, path: &Path) -> bool { +pub fn delete(store: &impl Store, location: Location, path: &Path) -> bool { debug_now!("deleting {}", &path); let fs = store.fs(location); if fs.remove(path).is_err() { @@ -583,14 +554,14 @@ pub fn delete(store: impl Store, location: Location, path: &Path) -> bool { } #[inline(never)] -pub fn exists(store: impl Store, location: Location, path: &Path) -> bool { +pub fn exists(store: &impl Store, location: Location, path: &Path) -> bool { debug_now!("checking existence of {}", &path); store.fs(location).exists(path) } #[inline(never)] pub fn metadata( - store: impl Store, + store: &impl Store, location: Location, path: &Path, ) -> Result, Error> { @@ -603,7 +574,7 @@ pub fn metadata( } #[inline(never)] -pub fn rename(store: impl Store, location: Location, from: &Path, to: &Path) -> Result<(), Error> { +pub fn rename(store: &impl Store, location: Location, from: &Path, to: &Path) -> Result<(), Error> { debug_now!("renaming {} to {}", &from, &to); store .fs(location) @@ -612,14 +583,14 @@ pub fn rename(store: impl Store, location: Location, from: &Path, to: &Path) -> } #[inline(never)] -pub fn remove_dir(store: impl Store, location: Location, path: &Path) -> bool { +pub fn remove_dir(store: &impl Store, location: Location, path: &Path) -> bool { debug_now!("remove_dir'ing {}", &path); store.fs(location).remove_dir(path).is_ok() } #[inline(never)] pub fn remove_dir_all_where( - store: impl Store, + store: &impl Store, location: Location, path: &Path, predicate: &dyn Fn(&DirEntry) -> bool, diff --git a/src/store/certstore.rs b/src/store/certstore.rs index e37c4663a2b..69b184f25f3 100644 --- a/src/store/certstore.rs +++ b/src/store/certstore.rs @@ -31,7 +31,7 @@ impl Certstore for ClientCertstore { let locations = [Location::Internal, Location::External, Location::Volatile]; locations .iter() - .any(|&location| store::delete(self.store, location, &path)) + .any(|&location| store::delete(&self.store, location, &path)) .then_some(()) .ok_or(Error::NoSuchKey) } @@ -41,14 +41,14 @@ impl Certstore for ClientCertstore { let locations = [Location::Internal, Location::External, Location::Volatile]; locations .iter() - .find_map(|&location| store::read(self.store, location, &path).ok()) + .find_map(|&location| store::read(&self.store, location, &path).ok()) .ok_or(Error::NoSuchCertificate) } fn write_certificate(&mut self, location: Location, der: &Message) -> Result { let id = CertId::new(&mut self.rng); let path = self.cert_path(id); - store::store(self.store, location, &path, der.as_slice())?; + store::store(&self.store, location, &path, der.as_slice())?; Ok(id) } } diff --git a/src/store/counterstore.rs b/src/store/counterstore.rs index 8d5765037a7..c38e856fc2b 100644 --- a/src/store/counterstore.rs +++ b/src/store/counterstore.rs @@ -37,14 +37,14 @@ impl ClientCounterstore { fn read_counter(&mut self, location: Location, id: CounterId) -> Result { let path = self.counter_path(id); - let mut bytes: crate::Bytes<16> = store::read(self.store, location, &path)?; + let mut bytes: crate::Bytes<16> = store::read(&self.store, location, &path)?; bytes.resize_default(16).ok(); Ok(u128::from_le_bytes(bytes.as_slice().try_into().unwrap())) } fn write_counter(&mut self, location: Location, id: CounterId, value: u128) -> Result<()> { let path = self.counter_path(id); - store::store(self.store, location, &path, &value.to_le_bytes()) + store::store(&self.store, location, &path, &value.to_le_bytes()) } fn increment_location(&mut self, location: Location, id: CounterId) -> Result { diff --git a/src/store/filestore.rs b/src/store/filestore.rs index 4a813318805..cbedf19e909 100644 --- a/src/store/filestore.rs +++ b/src/store/filestore.rs @@ -286,7 +286,7 @@ impl ClientFilestore { // the client, and return both the entry and the state .map(|(i, entry)| { // The semantics is that for a non-existent file, we return None (not an error) - let data = store::read(self.store, location, entry.path()).ok(); + let data = store::read(&self.store, location, entry.path()).ok(); (i, data) // the `ok_or` dummy error followed by the `ok` in the next line is because @@ -352,7 +352,7 @@ impl ClientFilestore { }) .map(|(i, entry)| { // The semantics is that for a non-existent file, we return None (not an error) - let data = store::read(self.store, location, entry.path()).ok(); + let data = store::read(&self.store, location, entry.path()).ok(); (i, data) }) // convert Option into Result, again because `read_dir_and_then` expects this @@ -376,36 +376,36 @@ impl Filestore for ClientFilestore { fn read(&mut self, path: &Path, location: Location) -> Result> { let path = self.actual_path(path)?; - store::read(self.store, location, &path) + store::read(&self.store, location, &path) } fn write(&mut self, path: &Path, location: Location, data: &[u8]) -> Result<()> { let path = self.actual_path(path)?; - store::store(self.store, location, &path, data) + store::store(&self.store, location, &path, data) } fn exists(&mut self, path: &Path, location: Location) -> bool { if let Ok(path) = self.actual_path(path) { - store::exists(self.store, location, &path) + store::exists(&self.store, location, &path) } else { false } } fn metadata(&mut self, path: &Path, location: Location) -> Result> { let path = self.actual_path(path)?; - store::metadata(self.store, location, &path) + store::metadata(&self.store, location, &path) } fn rename(&mut self, from: &Path, to: &Path, location: Location) -> Result<()> { let from = self.actual_path(from)?; let to = self.actual_path(to)?; - store::rename(self.store, location, &from, &to) + store::rename(&self.store, location, &from, &to) } fn remove_file(&mut self, path: &Path, location: Location) -> Result<()> { let path = self.actual_path(path)?; - match store::delete(self.store, location, &path) { + match store::delete(&self.store, location, &path) { true => Ok(()), false => Err(Error::InternalError), } @@ -414,7 +414,7 @@ impl Filestore for ClientFilestore { fn remove_dir(&mut self, path: &Path, location: Location) -> Result<()> { let path = self.actual_path(path)?; - match store::delete(self.store, location, &path) { + match store::delete(&self.store, location, &path) { true => Ok(()), false => Err(Error::InternalError), } @@ -423,7 +423,7 @@ impl Filestore for ClientFilestore { fn remove_dir_all(&mut self, path: &Path, location: Location) -> Result { let path = self.actual_path(path)?; - store::remove_dir_all_where(self.store, location, &path, &|_| true) + store::remove_dir_all_where(&self.store, location, &path, &|_| true) .map_err(|_| Error::InternalError) } fn remove_dir_all_where( @@ -434,7 +434,7 @@ impl Filestore for ClientFilestore { ) -> Result { let path = self.actual_path(path)?; - store::remove_dir_all_where(self.store, location, &path, &predicate) + store::remove_dir_all_where(&self.store, location, &path, &predicate) .map_err(|_| Error::InternalError) } diff --git a/src/store/keystore.rs b/src/store/keystore.rs index 7b4f927f5c7..47417c6105e 100644 --- a/src/store/keystore.rs +++ b/src/store/keystore.rs @@ -118,7 +118,7 @@ impl Keystore for ClientKeystore { let id = self.generate_key_id(); let path = self.key_path(secrecy, &id); - store::store(self.store, location, &path, &key.serialize())?; + store::store(&self.store, location, &path, &key.serialize())?; Ok(id) } @@ -145,7 +145,7 @@ impl Keystore for ClientKeystore { locations.iter().any(|location| { secrecies.iter().any(|secrecy| { let path = self.key_path(*secrecy, id); - store::delete(self.store, *location, &path) + store::delete(&self.store, *location, &path) }) }) } @@ -159,12 +159,12 @@ impl Keystore for ClientKeystore { fn delete_all(&self, location: Location) -> Result { let secret_path = self.key_directory(key::Secrecy::Secret); let secret_deleted = - store::remove_dir_all_where(self.store, location, &secret_path, &|dir_entry| { + store::remove_dir_all_where(&self.store, location, &secret_path, &|dir_entry| { dir_entry.file_name().as_ref().len() >= 4 })?; let public_path = self.key_directory(key::Secrecy::Public); let public_deleted = - store::remove_dir_all_where(self.store, location, &public_path, &|dir_entry| { + store::remove_dir_all_where(&self.store, location, &public_path, &|dir_entry| { dir_entry.file_name().as_ref().len() >= 4 })?; Ok(secret_deleted + public_deleted) @@ -181,7 +181,7 @@ impl Keystore for ClientKeystore { let location = self.location(secrecy, id).ok_or(Error::NoSuchKey)?; - let bytes: Bytes<{ MAX_KEY_MATERIAL_LENGTH }> = store::read(self.store, location, &path)?; + let bytes: Bytes<{ MAX_KEY_MATERIAL_LENGTH }> = store::read(&self.store, location, &path)?; let key = key::Key::try_deserialize(&bytes)?; @@ -212,7 +212,7 @@ impl Keystore for ClientKeystore { }; let path = self.key_path(secrecy, id); - store::store(self.store, location, &path, &key.serialize())?; + store::store(&self.store, location, &path, &key.serialize())?; Ok(()) } diff --git a/src/virt/store.rs b/src/virt/store.rs index 25f8b69404f..5e5cd8344c4 100644 --- a/src/virt/store.rs +++ b/src/virt/store.rs @@ -16,8 +16,9 @@ use crate::{store, store::Store}; pub trait StoreProvider { type Store: Store; + type Ifs; - unsafe fn ifs() -> &'static mut ::I; + unsafe fn ifs() -> &'static mut Self::Ifs; unsafe fn store() -> Self::Store; @@ -131,6 +132,7 @@ impl Filesystem { impl StoreProvider for Filesystem { type Store = FilesystemStore; + type Ifs = FilesystemStorage; unsafe fn ifs() -> &'static mut FilesystemStorage { (*addr_of_mut!(INTERNAL_FILESYSTEM_STORAGE)) @@ -180,6 +182,7 @@ pub struct Ram {} impl StoreProvider for Ram { type Store = RamStore; + type Ifs = InternalStorage; unsafe fn ifs() -> &'static mut InternalStorage { (*addr_of_mut!(INTERNAL_RAM_STORAGE)) From 1bd555c23ae983a5406727e8dbbfd1e35421aa19 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 24 Jan 2025 11:29:23 +0100 Subject: [PATCH 2/4] fixup! Simplify Store trait --- Cargo.toml | 1 + src/store.rs | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f179f02eae6..e34ccf2792d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -91,6 +91,7 @@ virt = ["std"] log-all = [] log-none = [] +log-trace = [] log-info = [] log-debug = [] log-warn = [] diff --git a/src/store.rs b/src/store.rs index 9f5a435d5c5..f9ded2046e6 100644 --- a/src/store.rs +++ b/src/store.rs @@ -339,7 +339,7 @@ macro_rules! store { format: bool, ) -> littlefs2::io::Result<()> { use core::mem::MaybeUninit; - use core::ptr::addr_of_mut; + use core::ptr::{addr_of, addr_of_mut}; use littlefs2::fs::{Allocation, Filesystem}; static mut IFS_ALLOC: MaybeUninit<&'static mut Allocation<$Ifs>> = @@ -375,7 +375,7 @@ macro_rules! store { &mut *(*addr_of_mut!(IFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(IFS_STORAGE)).as_mut_ptr(), )?); - Self::ifs_ptr().write(IFS.as_ref().unwrap()); + Self::ifs_ptr().write(addr_of!(IFS).as_ref().unwrap().as_ref().unwrap()); (*addr_of_mut!(EFS_ALLOC)).as_mut_ptr().write(efs_alloc); (*addr_of_mut!(EFS_STORAGE)).as_mut_ptr().write(efs_storage); @@ -383,7 +383,7 @@ macro_rules! store { &mut *(*addr_of_mut!(EFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(EFS_STORAGE)).as_mut_ptr(), )?); - Self::efs_ptr().write(EFS.as_ref().unwrap()); + Self::efs_ptr().write(addr_of!(EFS).as_ref().unwrap().as_ref().unwrap()); (*addr_of_mut!(VFS_ALLOC)).as_mut_ptr().write(vfs_alloc); (*addr_of_mut!(VFS_STORAGE)).as_mut_ptr().write(vfs_storage); @@ -391,7 +391,7 @@ macro_rules! store { &mut *(*addr_of_mut!(VFS_ALLOC)).as_mut_ptr(), &mut *(*addr_of_mut!(VFS_STORAGE)).as_mut_ptr(), )?); - Self::vfs_ptr().write(VFS.as_ref().unwrap()); + Self::vfs_ptr().write(addr_of!(VFS).as_ref().unwrap().as_ref().unwrap()); Ok(()) } From 609d906d48a97fed4532726c8811f1e9d99b6535 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 24 Jan 2025 11:41:17 +0100 Subject: [PATCH 3/4] fixup! Simplify Store trait --- src/service/attest.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/service/attest.rs b/src/service/attest.rs index ddabe475133..08e36d40598 100644 --- a/src/service/attest.rs +++ b/src/service/attest.rs @@ -439,10 +439,10 @@ impl Encodable for Name<'_> { l += 0xD; } if let Some(organization) = self.organization { - l += 11 + organization.as_bytes().len() as u16; + l += 11 + organization.len() as u16; } if let Some(state) = self.state { - l += 11 + state.as_bytes().len() as u16; + l += 11 + state.len() as u16; } Ok(l.into()) } From be9bdfaa9d70f080887f3a309fbbcf5530d69f15 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 24 Jan 2025 12:37:03 +0100 Subject: [PATCH 4/4] Simplify virtual store This patch uses the simplified Store trait to also simplify the store implementation used by the virt module: Instead of using references to static storages and filesystems protected by a mutex, we can now just use separate non-static instances for every test. The only downside is that we can no longer provide raw access to the IFS via the StoreProvider::ifs function, so we cannot run the provisioner app with this platform implementation. But I think this is an acceptable tradeoff. --- src/virt.rs | 60 +++++++++------ src/virt/store.rs | 182 +++++++++++++++++++--------------------------- 2 files changed, 112 insertions(+), 130 deletions(-) diff --git a/src/virt.rs b/src/virt.rs index 4bd91933ff1..472bc7ce829 100644 --- a/src/virt.rs +++ b/src/virt.rs @@ -9,10 +9,7 @@ mod ui; use std::{ iter, path::PathBuf, - sync::{ - mpsc::{self, Receiver, Sender}, - Mutex, - }, + sync::mpsc::{self, Receiver, Sender}, thread, }; @@ -27,31 +24,52 @@ use crate::{ types::CoreContext, ClientImplementation, }; +use store::{Resources, Store}; pub use store::{Filesystem, Ram, StoreProvider}; pub use ui::UserInterface; pub type Client<'a, D = CoreOnly> = ClientImplementation<'a, Syscall, D>; -// We need this mutex to make sure that: -// - TrussedInterchange is not used concurrently (panics if violated) -// - the Store is not used concurrently -static MUTEX: Mutex<()> = Mutex::new(()); - -pub fn with_platform(store: S, f: F) -> R +pub fn with_platform(store_provider: S, f: F) -> R where S: StoreProvider, - F: FnOnce(Platform) -> R, + F: FnOnce(Platform<'_>) -> R, { - let _guard = MUTEX.lock().unwrap_or_else(|err| err.into_inner()); - unsafe { - store.reset(); + let format_internal = store_provider.format_internal(); + let format_external = store_provider.format_external(); + let format_volatile = store_provider.format_volatile(); + + let (internal, external, volatile) = store_provider.split(); + let mut internal = Resources::new(internal); + let mut external = Resources::new(external); + let mut volatile = Resources::new(volatile); + + if format_internal { + internal.format().unwrap(); + } + if format_external { + external.format().unwrap(); + } + if format_volatile { + volatile.format().unwrap(); } + + let ifs = internal.mount().unwrap(); + let efs = external.mount().unwrap(); + let vfs = volatile.mount().unwrap(); + + let store = Store { + internal: &ifs, + external: &efs, + volatile: &vfs, + }; + // causing a regression again // let rng = chacha20::ChaCha8Rng::from_rng(rand_core::OsRng).unwrap(); let platform = Platform { rng: ChaCha8Rng::from_seed([42u8; 32]), - _store: store, + store, ui: UserInterface::new(), }; f(platform) @@ -184,13 +202,13 @@ impl<'a, I: 'static, C: Default> Runner<'a, I, C> { } } -pub struct Platform { +pub struct Platform<'a> { rng: ChaCha8Rng, - _store: S, + store: store::Store<'a>, ui: UserInterface, } -impl Platform { +impl Platform<'_> { pub fn run_client(self, client_id: &str, test: impl FnOnce(Client) -> R) -> R { self.run_client_with_backends(client_id, CoreOnly, &[], test) } @@ -255,9 +273,9 @@ impl Platform { } } -unsafe impl platform::Platform for Platform { +unsafe impl<'a> platform::Platform for Platform<'a> { type R = ChaCha8Rng; - type S = S::Store; + type S = store::Store<'a>; type UI = UserInterface; fn user_interface(&mut self) -> &mut Self::UI { @@ -269,6 +287,6 @@ unsafe impl platform::Platform for Platform { } fn store(&self) -> Self::S { - unsafe { S::store() } + self.store } } diff --git a/src/virt/store.rs b/src/virt/store.rs index 5e5cd8344c4..6beeaeeb249 100644 --- a/src/virt/store.rs +++ b/src/virt/store.rs @@ -1,44 +1,39 @@ -use core::ptr::addr_of_mut; use std::{ fs::{File, OpenOptions}, io::{Read as _, Seek as _, SeekFrom, Write as _}, - marker::PhantomData, path::PathBuf, }; use generic_array::typenum::{U512, U8}; use littlefs2::{ const_ram_storage, driver::Storage, driver::Storage as LfsStorage, fs::Allocation, - io::Result as LfsResult, + io::Result as LfsResult, object_safe::DynFilesystem, }; -use crate::{store, store::Store}; +use crate::store; pub trait StoreProvider { - type Store: Store; - type Ifs; + type Internal: Storage; + type External: Storage; + type Volatile: Storage; - unsafe fn ifs() -> &'static mut Self::Ifs; + fn split(self) -> (Self::Internal, Self::External, Self::Volatile); - unsafe fn store() -> Self::Store; + fn format_internal(&self) -> bool { + true + } + + fn format_external(&self) -> bool { + true + } - unsafe fn reset(&self); + fn format_volatile(&self) -> bool { + true + } } const STORAGE_SIZE: usize = 512 * 128; -static mut INTERNAL_RAM_STORAGE: Option = None; -static mut INTERNAL_RAM_FS_ALLOC: Option> = None; - -static mut INTERNAL_FILESYSTEM_STORAGE: Option = None; -static mut INTERNAL_FILESYSTEM_FS_ALLOC: Option> = None; - -static mut EXTERNAL_STORAGE: Option = None; -static mut EXTERNAL_FS_ALLOC: Option> = None; - -static mut VOLATILE_STORAGE: Option = None; -static mut VOLATILE_FS_ALLOC: Option> = None; - const_ram_storage!(InternalStorage, STORAGE_SIZE); const_ram_storage!(ExternalStorage, STORAGE_SIZE); const_ram_storage!(VolatileStorage, STORAGE_SIZE); @@ -95,19 +90,6 @@ impl Storage for FilesystemStorage { } } -store!( - FilesystemStore, - Internal: FilesystemStorage, - External: ExternalStorage, - Volatile: VolatileStorage -); - -impl Default for FilesystemStore { - fn default() -> Self { - Self { __: PhantomData } - } -} - #[derive(Clone, Debug)] pub struct Filesystem { internal: PathBuf, @@ -131,95 +113,77 @@ impl Filesystem { } impl StoreProvider for Filesystem { - type Store = FilesystemStore; - type Ifs = FilesystemStorage; - - unsafe fn ifs() -> &'static mut FilesystemStorage { - (*addr_of_mut!(INTERNAL_FILESYSTEM_STORAGE)) - .as_mut() - .expect("ifs not initialized") - } - - unsafe fn store() -> Self::Store { - Self::Store { __: PhantomData } - } - - unsafe fn reset(&self) { - (*addr_of_mut!(INTERNAL_FILESYSTEM_STORAGE)) - .replace(FilesystemStorage(self.internal.clone())); - (*addr_of_mut!(INTERNAL_FILESYSTEM_FS_ALLOC)) - .replace(littlefs2::fs::Filesystem::allocate()); - reset_external(); - reset_volatile(); - - Self::store() - .mount( - (*addr_of_mut!(INTERNAL_FILESYSTEM_FS_ALLOC)) - .as_mut() - .unwrap(), - (*addr_of_mut!(INTERNAL_FILESYSTEM_STORAGE)) - .as_mut() - .unwrap(), - (*addr_of_mut!(EXTERNAL_FS_ALLOC)).as_mut().unwrap(), - (*addr_of_mut!(EXTERNAL_STORAGE)).as_mut().unwrap(), - (*addr_of_mut!(VOLATILE_FS_ALLOC)).as_mut().unwrap(), - (*addr_of_mut!(VOLATILE_STORAGE)).as_mut().unwrap(), - self.format, - ) - .expect("failed to mount filesystem"); + type Internal = FilesystemStorage; + type External = ExternalStorage; + type Volatile = VolatileStorage; + + fn split(self) -> (Self::Internal, Self::External, Self::Volatile) { + let internal = FilesystemStorage(self.internal); + let external = Self::External::new(); + let volatile = Self::Volatile::new(); + (internal, external, volatile) } -} -store!( - RamStore, - Internal: InternalStorage, - External: ExternalStorage, - Volatile: VolatileStorage -); + fn format_internal(&self) -> bool { + self.format + } +} -#[derive(Copy, Clone, Debug, Default)] +#[derive(Clone, Copy, Debug, Default)] pub struct Ram {} impl StoreProvider for Ram { - type Store = RamStore; - type Ifs = InternalStorage; - - unsafe fn ifs() -> &'static mut InternalStorage { - (*addr_of_mut!(INTERNAL_RAM_STORAGE)) - .as_mut() - .expect("ifs not initialized") + type Internal = InternalStorage; + type External = ExternalStorage; + type Volatile = VolatileStorage; + + fn split(self) -> (Self::Internal, Self::External, Self::Volatile) { + let internal = Self::Internal::new(); + let external = Self::External::new(); + let volatile = Self::Volatile::new(); + (internal, external, volatile) } +} - unsafe fn store() -> Self::Store { - Self::Store { __: PhantomData } +pub struct Resources { + storage: S, + alloc: Allocation, +} + +impl Resources { + pub fn new(storage: S) -> Self { + Self { + storage, + alloc: Allocation::new(), + } } - unsafe fn reset(&self) { - (*addr_of_mut!(INTERNAL_RAM_STORAGE)).replace(InternalStorage::new()); - (*addr_of_mut!(INTERNAL_RAM_FS_ALLOC)).replace(littlefs2::fs::Filesystem::allocate()); - reset_external(); - reset_volatile(); + pub fn format(&mut self) -> LfsResult<()> { + littlefs2::fs::Filesystem::format(&mut self.storage) + } - Self::store() - .mount( - (*addr_of_mut!(INTERNAL_RAM_FS_ALLOC)).as_mut().unwrap(), - (*addr_of_mut!(INTERNAL_RAM_STORAGE)).as_mut().unwrap(), - (*addr_of_mut!(EXTERNAL_FS_ALLOC)).as_mut().unwrap(), - (*addr_of_mut!(EXTERNAL_STORAGE)).as_mut().unwrap(), - (*addr_of_mut!(VOLATILE_FS_ALLOC)).as_mut().unwrap(), - (*addr_of_mut!(VOLATILE_STORAGE)).as_mut().unwrap(), - true, - ) - .expect("failed to mount filesystem"); + pub fn mount(&mut self) -> LfsResult> { + littlefs2::fs::Filesystem::mount(&mut self.alloc, &mut self.storage) } } -unsafe fn reset_external() { - (*addr_of_mut!(EXTERNAL_STORAGE)).replace(ExternalStorage::new()); - (*addr_of_mut!(EXTERNAL_FS_ALLOC)).replace(littlefs2::fs::Filesystem::allocate()); +#[derive(Clone, Copy)] +pub struct Store<'a> { + pub internal: &'a dyn DynFilesystem, + pub external: &'a dyn DynFilesystem, + pub volatile: &'a dyn DynFilesystem, } -unsafe fn reset_volatile() { - (*addr_of_mut!(VOLATILE_STORAGE)).replace(VolatileStorage::new()); - (*addr_of_mut!(VOLATILE_FS_ALLOC)).replace(littlefs2::fs::Filesystem::allocate()); +impl store::Store for Store<'_> { + fn ifs(&self) -> &dyn DynFilesystem { + self.internal + } + + fn efs(&self) -> &dyn DynFilesystem { + self.external + } + + fn vfs(&self) -> &dyn DynFilesystem { + self.volatile + } }