| Message ID | 20250326171411.590681-8-remo@buenzli.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: > +pub struct ArrayVec<const N: usize, T> { > + array: [core::mem::MaybeUninit<T>; N], > + len: usize, > +} I would reverse the order of the generic arguments so T is first so that it matches the declaration of an array: [T; N] > + /// Adds a new element to the end of the vector. > + /// > + /// # Panics > + /// > + /// Panics if the vector is already full. > + pub fn push(&mut self, elem: T) { > + if self.len == N { > + panic!("OOM") > + } > + self.array[self.len] = MaybeUninit::new(elem); > + self.len += 1; > + } This function should not panic. I would rewrite it so that its signature is pub fn push(&mut self, elem: T) -> Result<(), T> > +impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> { > + fn as_ref(&self) -> &[T] { > + // SAFETY: As per the type invariant, all elements at index < self.len > + // are initialized. > + unsafe { core::mem::transmute(&self.array[..self.len]) } > + } > +} > + > +impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> { > + fn as_mut(&mut self) -> &mut [T] { > + // SAFETY: As per the type invariant, all elements at index < self.len > + // are initialized. > + unsafe { core::mem::transmute(&mut self.array[..self.len]) } > + } > +} I would replace both uses of transmute here with slice::from_raw_parts like you did in drop. also it's not needed but you could also impl Deref and DerefMut for ArrayVec because you have already impl'ed AsRef and AsMut so it should be simple > +impl<const N: usize, T> Drop for ArrayVec<N, T> { > + fn drop(&mut self) { > + unsafe { this is missing a safety comment > + let slice: &mut [T] = > + core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len); > + core::ptr::drop_in_place(slice); > + } > + } > +} Andrew
On Wed, Mar 26, 2025 at 06:13:46PM +0100, Remo Senekowitsch wrote: > This patch is basically a proof of concept intendend to gather feedback > about how to do this properly. Normally I would want to use the crate > from crates.io[1], but that's not an option in the kernel. We could also > vendor the entire source code of arrayvec. I'm not sure if people will > be happy with that. Do we really need this? The only user in this series I could spot was property_get_reference_args(). And I think that with a proper abstraction of struct fwnode_reference_args we could avoid to copy memory at all. If it turns out we actually need something like this, I'd prefer to move it to rust/kernel/alloc/ and see if it's worth to derive a common trait that maybe can share a few things between ArrayVec and Vec. > > [1] https://crates.io/crates/arrayvec > > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > --- > rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 2 files changed, 82 insertions(+) > create mode 100644 rust/kernel/arrayvec.rs > > diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs > new file mode 100644 > index 000000000..041e7dcce > --- /dev/null > +++ b/rust/kernel/arrayvec.rs > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Provides [ArrayVec], a stack-allocated vector with static capacity. > + > +use core::mem::MaybeUninit; > + > +/// A stack-allocated vector with statically fixed capacity. > +/// > +/// This can be useful to avoid heap allocation and still ensure safety where a > +/// small but dynamic number of elements is needed. > +/// > +/// For example, consider a function that returns a variable number of values, > +/// but no more than 8. In C, one might achieve this by passing a pointer to > +/// a stack-allocated array as an out-parameter and making the function return > +/// the actual number of elements. This is not safe, because nothing prevents > +/// the caller from reading elements from the array that weren't actually > +/// initialized by the function. `ArrayVec` solves this problem, users are > +/// prevented from accessing uninitialized elements. > +/// > +/// This basically exists already (in a much more mature form) on crates.io: > +/// <https://crates.io/crates/arrayvec> > +#[derive(Debug)] > +pub struct ArrayVec<const N: usize, T> { > + array: [core::mem::MaybeUninit<T>; N], > + len: usize, > +} > + > +impl<const N: usize, T> ArrayVec<N, T> { > + /// Adds a new element to the end of the vector. > + /// > + /// # Panics > + /// > + /// Panics if the vector is already full. > + pub fn push(&mut self, elem: T) { > + if self.len == N { > + panic!("OOM") Please do not panic, this should return a Result instead.
diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs new file mode 100644 index 000000000..041e7dcce --- /dev/null +++ b/rust/kernel/arrayvec.rs @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Provides [ArrayVec], a stack-allocated vector with static capacity. + +use core::mem::MaybeUninit; + +/// A stack-allocated vector with statically fixed capacity. +/// +/// This can be useful to avoid heap allocation and still ensure safety where a +/// small but dynamic number of elements is needed. +/// +/// For example, consider a function that returns a variable number of values, +/// but no more than 8. In C, one might achieve this by passing a pointer to +/// a stack-allocated array as an out-parameter and making the function return +/// the actual number of elements. This is not safe, because nothing prevents +/// the caller from reading elements from the array that weren't actually +/// initialized by the function. `ArrayVec` solves this problem, users are +/// prevented from accessing uninitialized elements. +/// +/// This basically exists already (in a much more mature form) on crates.io: +/// <https://crates.io/crates/arrayvec> +#[derive(Debug)] +pub struct ArrayVec<const N: usize, T> { + array: [core::mem::MaybeUninit<T>; N], + len: usize, +} + +impl<const N: usize, T> ArrayVec<N, T> { + /// Adds a new element to the end of the vector. + /// + /// # Panics + /// + /// Panics if the vector is already full. + pub fn push(&mut self, elem: T) { + if self.len == N { + panic!("OOM") + } + self.array[self.len] = MaybeUninit::new(elem); + self.len += 1; + } + + /// Returns the length of the vector. + pub fn len(&self) -> usize { + self.len + } +} + +impl<const N: usize, T> Default for ArrayVec<N, T> { + fn default() -> Self { + Self { + array: [const { MaybeUninit::uninit() }; N], + len: 0, + } + } +} + +impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> { + fn as_ref(&self) -> &[T] { + // SAFETY: As per the type invariant, all elements at index < self.len + // are initialized. + unsafe { core::mem::transmute(&self.array[..self.len]) } + } +} + +impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> { + fn as_mut(&mut self) -> &mut [T] { + // SAFETY: As per the type invariant, all elements at index < self.len + // are initialized. + unsafe { core::mem::transmute(&mut self.array[..self.len]) } + } +} + +impl<const N: usize, T> Drop for ArrayVec<N, T> { + fn drop(&mut self) { + unsafe { + let slice: &mut [T] = + core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len); + core::ptr::drop_in_place(slice); + } + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index ca233fd20..0777f7a42 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -36,6 +36,7 @@ pub use ffi; pub mod alloc; +pub mod arrayvec; #[cfg(CONFIG_BLOCK)] pub mod block; #[doc(hidden)]
This patch is basically a proof of concept intendend to gather feedback about how to do this properly. Normally I would want to use the crate from crates.io[1], but that's not an option in the kernel. We could also vendor the entire source code of arrayvec. I'm not sure if people will be happy with that. [1] https://crates.io/crates/arrayvec Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> --- rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 2 files changed, 82 insertions(+) create mode 100644 rust/kernel/arrayvec.rs