This patch series adds several improvements to the pin-init api:
- a derive macro for the `Zeroable` trait,
- makes hygiene of fields in initializers behave like normal struct
initializers would behave,
- prevent stackoverflow without optimizations
- add `..Zeroable::zeroed()` syntax to zero missing fields.
- support arbitrary paths in initializer macros
This is the second version of this patch series.
* v1: https://lore.kernel.org/rust-for-linux/[email protected]/
Changes not present on modified commits:
v1 -> v2:
- implement `Zeroable` for `Opaque`,
- remove blanket impl of `PinInit` for `Init` and make it a supertrait
instead,
- add `{pin_}chain` functions to execute code after initialization,
- update the example macro expansion
Benno Lossin (12):
rust: init: consolidate init macros
rust: add derive macro for `Zeroable`
rust: init: make guards in the init macros hygienic
rust: init: wrap type checking struct initializers in a closure
rust: init: make initializer values inaccessible after initializing
rust: init: add `..Zeroable::zeroed()` syntax for zeroing all missing
fields
rust: init: Add functions to create array initializers
rust: init: add support for arbitrary paths in init macros
rust: init: implement Zeroable for Opaque<T>
rust: init: make `PinInit<T, E>` a supertrait of `Init<T, E>`
rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
rust: init: update expanded macro explanation
rust/kernel/init.rs | 633 ++++++++++++++-------------------
rust/kernel/init/__internal.rs | 39 +-
rust/kernel/init/macros.rs | 510 +++++++++++++++++++++++---
rust/kernel/prelude.rs | 2 +-
rust/kernel/types.rs | 2 +
rust/macros/lib.rs | 20 ++
rust/macros/quote.rs | 6 +
rust/macros/zeroable.rs | 25 ++
8 files changed, 784 insertions(+), 453 deletions(-)
create mode 100644 rust/macros/zeroable.rs
base-commit: 6946437838b0ec7c976252f5d1872d4d8b679515
--
2.41.0
Add the struct update syntax to the init macros, but only for
`..Zeroable::zeroed()`. Adding this at the end of the struct initializer
allows one to omit fields from the initializer, these fields will be
initialized with 0x00 set to every byte. Only types that implement the
`Zeroable` trait can utilize this.
Suggested-by: Asahi Lina <[email protected]>
Signed-off-by: Benno Lossin <[email protected]>
---
v1 -> v2:
* fix doctest imports
* fix doctest examples
* fix `Zeroable` path in the `__init_internal` macro
* rename `is_zeroable` -> `assert_zeroable`
* add missing `{}` to the case when `..Zeroable::zeroed()` is present
* add `allow(unused_assignments)` in the type-checked struct initializer
rust/kernel/init.rs | 16 +++++-
rust/kernel/init/macros.rs | 115 ++++++++++++++++++++++++++++++++++++-
2 files changed, 129 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 0120674b451e..460f808ebf84 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -517,13 +517,17 @@ macro_rules! stack_try_pin_init {
/// - Fields that you want to initialize in-place have to use `<-` instead of `:`.
/// - In front of the initializer you can write `&this in` to have access to a [`NonNull<Self>`]
/// pointer named `this` inside of the initializer.
+/// - Using struct update syntax one can place `..Zeroable::zeroed()` at the very end of the
+/// struct, this initializes every field with 0 and then runs all initializers specified in the
+/// body. This can only be done if [`Zeroable`] is implemented for the struct.
///
/// For instance:
///
/// ```rust
-/// # use kernel::{macros::pin_data, pin_init};
+/// # use kernel::{macros::{Zeroable, pin_data}, pin_init};
/// # use core::{ptr::addr_of_mut, marker::PhantomPinned};
/// #[pin_data]
+/// #[derive(Zeroable)]
/// struct Buf {
/// // `ptr` points into `buf`.
/// ptr: *mut u8,
@@ -536,6 +540,10 @@ macro_rules! stack_try_pin_init {
/// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() },
/// pin: PhantomPinned,
/// });
+/// pin_init!(Buf {
+/// buf: [1; 64],
+/// ..Zeroable::zeroed()
+/// });
/// ```
///
/// [`try_pin_init!`]: kernel::try_pin_init
@@ -555,6 +563,7 @@ macro_rules! pin_init {
@data(PinData, use_data),
@has_data(HasPinData, __pin_data),
@construct_closure(pin_init_from_closure),
+ @munch_fields($($fields)*),
)
};
}
@@ -611,6 +620,7 @@ macro_rules! try_pin_init {
@data(PinData, use_data),
@has_data(HasPinData, __pin_data),
@construct_closure(pin_init_from_closure),
+ @munch_fields($($fields)*),
)
};
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
@@ -624,6 +634,7 @@ macro_rules! try_pin_init {
@data(PinData, use_data),
@has_data(HasPinData, __pin_data),
@construct_closure(pin_init_from_closure),
+ @munch_fields($($fields)*),
)
};
}
@@ -658,6 +669,7 @@ macro_rules! init {
@data(InitData, /*no use_data*/),
@has_data(HasInitData, __init_data),
@construct_closure(init_from_closure),
+ @munch_fields($($fields)*),
)
}
}
@@ -708,6 +720,7 @@ macro_rules! try_init {
@data(InitData, /*no use_data*/),
@has_data(HasInitData, __init_data),
@construct_closure(init_from_closure),
+ @munch_fields($($fields)*),
)
};
($(&$this:ident in)? $t:ident $(::<$($generics:ty),* $(,)?>)? {
@@ -721,6 +734,7 @@ macro_rules! try_init {
@data(InitData, /*no use_data*/),
@has_data(HasInitData, __init_data),
@construct_closure(init_from_closure),
+ @munch_fields($($fields)*),
)
};
}
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 5de939e0801f..f5d7f0943f60 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -989,6 +989,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*>
///
/// This macro has multiple internal call configurations, these are always the very first ident:
/// - nothing: this is the base case and called by the `{try_}{pin_}init!` macros.
+/// - `with_update_parsed`: when the `..Zeroable::zeroed()` syntax has been handled.
/// - `init_slot`: recursively creates the code that initializes all fields in `slot`.
/// - `make_initializer`: recursively create the struct initializer that guarantees that every
/// field has been initialized exactly once.
@@ -1007,6 +1008,82 @@ macro_rules! __init_internal {
@has_data($has_data:ident, $get_data:ident),
// `pin_init_from_closure` or `init_from_closure`.
@construct_closure($construct_closure:ident),
+ @munch_fields(),
+ ) => {
+ $crate::__init_internal!(with_update_parsed:
+ @this($($this)?),
+ @typ($t $(::<$($generics),*>)? ),
+ @fields($($fields)*),
+ @error($err),
+ @data($data, $($use_data)?),
+ @has_data($has_data, $get_data),
+ @construct_closure($construct_closure),
+ @zeroed(), // nothing means default behavior.
+ )
+ };
+ (
+ @this($($this:ident)?),
+ @typ($t:ident $(::<$($generics:ty),*>)?),
+ @fields($($fields:tt)*),
+ @error($err:ty),
+ // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
+ // case.
+ @data($data:ident, $($use_data:ident)?),
+ // `HasPinData` or `HasInitData`.
+ @has_data($has_data:ident, $get_data:ident),
+ // `pin_init_from_closure` or `init_from_closure`.
+ @construct_closure($construct_closure:ident),
+ @munch_fields(..Zeroable::zeroed()),
+ ) => {
+ $crate::__init_internal!(with_update_parsed:
+ @this($($this)?),
+ @typ($t $(::<$($generics),*>)? ),
+ @fields($($fields)*),
+ @error($err),
+ @data($data, $($use_data)?),
+ @has_data($has_data, $get_data),
+ @construct_closure($construct_closure),
+ @zeroed(()), // `()` means zero all fields not mentioned.
+ )
+ };
+ (
+ @this($($this:ident)?),
+ @typ($t:ident $(::<$($generics:ty),*>)?),
+ @fields($($fields:tt)*),
+ @error($err:ty),
+ // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
+ // case.
+ @data($data:ident, $($use_data:ident)?),
+ // `HasPinData` or `HasInitData`.
+ @has_data($has_data:ident, $get_data:ident),
+ // `pin_init_from_closure` or `init_from_closure`.
+ @construct_closure($construct_closure:ident),
+ @munch_fields($ignore:tt $($rest:tt)*),
+ ) => {
+ $crate::__init_internal!(
+ @this($($this)?),
+ @typ($t $(::<$($generics),*>)? ),
+ @fields($($fields)*),
+ @error($err),
+ @data($data, $($use_data)?),
+ @has_data($has_data, $get_data),
+ @construct_closure($construct_closure),
+ @munch_fields($($rest)*),
+ )
+ };
+ (with_update_parsed:
+ @this($($this:ident)?),
+ @typ($t:ident $(::<$($generics:ty),*>)?),
+ @fields($($fields:tt)*),
+ @error($err:ty),
+ // Either `PinData` or `InitData`, `$use_data` should only be present in the `PinData`
+ // case.
+ @data($data:ident, $($use_data:ident)?),
+ // `HasPinData` or `HasInitData`.
+ @has_data($has_data:ident, $get_data:ident),
+ // `pin_init_from_closure` or `init_from_closure`.
+ @construct_closure($construct_closure:ident),
+ @zeroed($($init_zeroed:expr)?),
) => {{
// We do not want to allow arbitrary returns, so we declare this type as the `Ok` return
// type and shadow it later when we insert the arbitrary user code. That way there will be
@@ -1024,6 +1101,17 @@ macro_rules! __init_internal {
{
// Shadow the structure so it cannot be used to return early.
struct __InitOk;
+ // If `$init_zeroed` is present we should zero the slot now and not emit an
+ // error when fields are missing (since they will be zeroed). We also have to
+ // check that the type actually implements `Zeroable`.
+ $({
+ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}
+ // Ensure that the struct is indeed `Zeroable`.
+ assert_zeroable(slot);
+ // SAFETY: The type implements `Zeroable` by the check above.
+ unsafe { ::core::ptr::write_bytes(slot, 0, 1) };
+ $init_zeroed // this will be `()` if set.
+ })?
// Create the `this` so it can be referenced by the user inside of the
// expressions creating the individual fields.
$(let $this = unsafe { ::core::ptr::NonNull::new_unchecked(slot) };)?
@@ -1060,7 +1148,7 @@ macro_rules! __init_internal {
@data($data:ident),
@slot($slot:ident),
@guards($($guards:ident,)*),
- @munch_fields($(,)?),
+ @munch_fields($(..Zeroable::zeroed())? $(,)?),
) => {
// Endpoint of munching, no fields are left. If execution reaches this point, all fields
// have been initialized. Therefore we can now dismiss the guards by forgetting them.
@@ -1161,6 +1249,31 @@ macro_rules! __init_internal {
);
}
};
+ (make_initializer:
+ @slot($slot:ident),
+ @type_name($t:ident),
+ @munch_fields(..Zeroable::zeroed() $(,)?),
+ @acc($($acc:tt)*),
+ ) => {
+ // Endpoint, nothing more to munch, create the initializer. Since the users specified
+ // `..Zeroable::zeroed()`, the slot will already have been zeroed and all field that have
+ // not been overwritten are thus zero and initialized. We still check that all fields are
+ // actually accessible by using the struct update syntax ourselves.
+ // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
+ // get the correct type inference here:
+ #[allow(unused_assignments)]
+ unsafe {
+ let mut zeroed = ::core::mem::zeroed();
+ // We have to use type inference here to make zeroed have the correct type. This does
+ // not get executed, so it has no effect.
+ ::core::ptr::write($slot, zeroed);
+ zeroed = ::core::mem::zeroed();
+ ::core::ptr::write($slot, $t {
+ $($acc)*
+ ..zeroed
+ });
+ }
+ };
(make_initializer:
@slot($slot:ident),
@type_name($t:ident),
--
2.41.0
Previously the init macros would create a local variable with the name
and hygiene of the field that is being initialized to store the value of
the field. This would override any user defined variables. For example:
```
struct Foo {
a: usize,
b: usize,
}
let a = 10;
let foo = init!(Foo{
a: a + 1, // This creates a local variable named `a`.
b: a, // This refers to that variable!
});
let foo = Box::init!(foo)?;
assert_eq!(foo.a, 11);
assert_eq!(foo.b, 11);
```
This patch changes this behavior, so the above code would panic at the
last assertion, since `b` would have value 10.
Signed-off-by: Benno Lossin <[email protected]>
---
rust/kernel/init/macros.rs | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/rust/kernel/init/macros.rs b/rust/kernel/init/macros.rs
index 160b95fc03c9..5de939e0801f 100644
--- a/rust/kernel/init/macros.rs
+++ b/rust/kernel/init/macros.rs
@@ -1073,13 +1073,13 @@ macro_rules! __init_internal {
// In-place initialization syntax.
@munch_fields($field:ident <- $val:expr, $($rest:tt)*),
) => {
- let $field = $val;
+ let init = $val;
// Call the initializer.
//
// SAFETY: `slot` is valid, because we are inside of an initializer closure, we
// return when an error/panic occurs.
// We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`.
- unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), $field)? };
+ unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), init)? };
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1105,12 +1105,12 @@ macro_rules! __init_internal {
// In-place initialization syntax.
@munch_fields($field:ident <- $val:expr, $($rest:tt)*),
) => {
- let $field = $val;
+ let init = $val;
// Call the initializer.
//
// SAFETY: `slot` is valid, because we are inside of an initializer closure, we
// return when an error/panic occurs.
- unsafe { $crate::init::Init::__init($field, ::core::ptr::addr_of_mut!((*$slot).$field))? };
+ unsafe { $crate::init::Init::__init(init, ::core::ptr::addr_of_mut!((*$slot).$field))? };
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
@@ -1136,11 +1136,13 @@ macro_rules! __init_internal {
// Init by-value.
@munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*),
) => {
- $(let $field = $val;)?
- // Initialize the field.
- //
- // SAFETY: The memory at `slot` is uninitialized.
- unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
+ {
+ $(let $field = $val;)?
+ // Initialize the field.
+ //
+ // SAFETY: The memory at `slot` is uninitialized.
+ unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) };
+ }
// Create the drop guard:
//
// We rely on macro hygiene to make it impossible for users to access this local variable.
--
2.41.0
Remove the blanket implementation of `PinInit<T, E> for I where I:
Init<T, E>`. This blanket implementation prevented custom types that
implement `PinInit`.
Signed-off-by: Benno Lossin <[email protected]>
---
rust/kernel/init.rs | 20 +++++++-------------
rust/kernel/init/__internal.rs | 12 ++++++++++++
2 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index fa1ebdbf5f4b..3c7cd36a424b 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -805,7 +805,7 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
///
/// [`Arc<T>`]: crate::sync::Arc
#[must_use = "An initializer must be used in order to create its value."]
-pub unsafe trait Init<T: ?Sized, E = Infallible>: Sized {
+pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
/// Initializes `slot`.
///
/// # Safety
@@ -816,18 +816,6 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: Sized {
unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
}
-// SAFETY: Every in-place initializer can also be used as a pin-initializer.
-unsafe impl<T: ?Sized, E, I> PinInit<T, E> for I
-where
- I: Init<T, E>,
-{
- unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
- // SAFETY: `__init` meets the same requirements as `__pinned_init`, except that it does not
- // require `slot` to not move after init.
- unsafe { self.__init(slot) }
- }
-}
-
/// Creates a new [`PinInit<T, E>`] from the given closure.
///
/// # Safety
@@ -968,6 +956,12 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
Ok(())
}
}
+// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
+unsafe impl<T, E> PinInit<T, E> for T {
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ unsafe { self.__init(slot) }
+ }
+}
/// Smart pointer that can initialize memory in-place.
pub trait InPlaceInit<T>: Sized {
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index 7abd1fb65e41..12e195061525 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -32,6 +32,18 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
}
}
+// SAFETY: While constructing the `InitClosure`, the user promised that it upholds the
+// `__pinned_init` invariants.
+unsafe impl<T: ?Sized, F, E> PinInit<T, E> for InitClosure<F, T, E>
+where
+ F: FnOnce(*mut T) -> Result<(), E>,
+{
+ #[inline]
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ (self.0)(slot)
+ }
+}
+
/// This trait is only implemented via the `#[pin_data]` proc-macro. It is used to facilitate
/// the pin projections within the initializers.
///
--
2.41.0
The `{pin_}chain` functions extend an initializer: it not only
initializes the value, but also executes a closure taking a reference to
the initialized value. This allows to do something with a value directly
after initialization.
Suggested-by: Asahi Lina <[email protected]>
Signed-off-by: Benno Lossin <[email protected]>
---
rust/kernel/init.rs | 138 +++++++++++++++++++++++++++++++++
rust/kernel/init/__internal.rs | 2 +-
2 files changed, 139 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 3c7cd36a424b..3b0df839f64c 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -773,6 +773,77 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
/// deallocate.
/// - `slot` will not move until it is dropped, i.e. it will be pinned.
unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E>;
+
+ /// First initializes the value using `self` then calls the function `f` with the initialized
+ /// value.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// # #![allow(clippy::disallowed_names)]
+ /// use kernel::{types::Opaque, init::pin_init_from_closure};
+ /// #[repr(C)]
+ /// struct RawFoo([u8; 16]);
+ /// extern {
+ /// fn init_foo(_: *mut RawFoo);
+ /// }
+ ///
+ /// #[pin_data]
+ /// struct Foo {
+ /// #[pin]
+ /// raw: Opaque<RawFoo>,
+ /// }
+ ///
+ /// impl Foo {
+ /// fn setup(self: Pin<&mut Self>) {
+ /// pr_info!("Setting up foo");
+ /// }
+ /// }
+ ///
+ /// let foo = pin_init!(Foo {
+ /// raw <- unsafe {
+ /// Opaque::ffi_init(|s| {
+ /// init_foo(s);
+ /// })
+ /// },
+ /// }).pin_chain(|foo| {
+ /// foo.setup();
+ /// Ok(())
+ /// });
+ /// ```
+ fn pin_chain<F>(self, f: F) -> ChainPinInit<Self, F, T, E>
+ where
+ F: FnOnce(Pin<&mut T>) -> Result<(), E>,
+ {
+ ChainPinInit(self, f, PhantomData)
+ }
+}
+
+/// An initializer returned by [`PinInit::pin_chain`].
+pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
+
+// SAFETY: the `__pinned_init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+// - considers `slot` pinned.
+unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
+where
+ I: PinInit<T, E>,
+ F: FnOnce(Pin<&mut T>) -> Result<(), E>,
+{
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
+ unsafe { self.0.__pinned_init(slot)? };
+ // SAFETY: The above call initialized `slot` and we still have unique access.
+ let val = unsafe { &mut *slot };
+ // SAFETY: `slot` is considered pinned
+ let val = unsafe { Pin::new_unchecked(val) };
+ (self.1)(val).map_err(|e| {
+ // SAFETY: `slot` was initialized above.
+ unsafe { core::ptr::drop_in_place(slot) };
+ e
+ })
+ }
}
/// An initializer for `T`.
@@ -814,6 +885,73 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
/// - the caller does not touch `slot` when `Err` is returned, they are only permitted to
/// deallocate.
unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
+
+ /// First initializes the value using `self` then calls the function `f` with the initialized
+ /// value.
+ ///
+ /// # Examples
+ ///
+ /// ```rust
+ /// # #![allow(clippy::disallowed_names)]
+ /// use kernel::{types::Opaque, init::{self, init_from_closure}};
+ /// struct Foo {
+ /// buf: [u8; 1_000_000],
+ /// }
+ ///
+ /// impl Foo {
+ /// fn setup(&mut self) {
+ /// pr_info!("Setting up foo");
+ /// }
+ /// }
+ ///
+ /// let foo = init!(Foo {
+ /// buf <- init::zeroed()
+ /// }).chain(|foo| {
+ /// foo.setup();
+ /// Ok(())
+ /// });
+ /// ```
+ fn chain<F>(self, f: F) -> ChainInit<Self, F, T, E>
+ where
+ F: FnOnce(&mut T) -> Result<(), E>,
+ {
+ ChainInit(self, f, PhantomData)
+ }
+}
+
+/// An initializer returned by [`Init::chain`].
+pub struct ChainInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
+
+// SAFETY: the `__init` function is implemented such that it
+// - returns `Ok(())` on successful initialization,
+// - returns `Err(err)` on error and in this case `slot` will be dropped.
+unsafe impl<T: ?Sized, E, I, F> Init<T, E> for ChainInit<I, F, T, E>
+where
+ I: Init<T, E>,
+ F: FnOnce(&mut T) -> Result<(), E>,
+{
+ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: all requirements fulfilled since this function is `__init`.
+ unsafe { self.0.__pinned_init(slot)? };
+ // SAFETY: The above call initialized `slot` and we still have unique access.
+ (self.1)(unsafe { &mut *slot }).map_err(|e| {
+ // SAFETY: `slot` was initialized above.
+ unsafe { core::ptr::drop_in_place(slot) };
+ e
+ })
+ }
+}
+
+// SAFETY: `__pinned_init` behaves exactly the same as `__init`.
+unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainInit<I, F, T, E>
+where
+ I: Init<T, E>,
+ F: FnOnce(&mut T) -> Result<(), E>,
+{
+ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
+ // SAFETY: `__init` has less strict requirements compared to `__pinned_init`.
+ unsafe { self.__init(slot) }
+ }
}
/// Creates a new [`PinInit<T, E>`] from the given closure.
diff --git a/rust/kernel/init/__internal.rs b/rust/kernel/init/__internal.rs
index 12e195061525..db3372619ecd 100644
--- a/rust/kernel/init/__internal.rs
+++ b/rust/kernel/init/__internal.rs
@@ -13,7 +13,7 @@
///
/// [nomicon]: https://doc.rust-lang.org/nomicon/subtyping.html
/// [this table]: https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns
-type Invariant<T> = PhantomData<fn(*mut T) -> *mut T>;
+pub(super) type Invariant<T> = PhantomData<fn(*mut T) -> *mut T>;
/// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this
/// type, since the closure needs to fulfill the same safety requirement as the
--
2.41.0
On 19.07.23 16:20, Benno Lossin wrote:
> This patch series adds several improvements to the pin-init api:
> - a derive macro for the `Zeroable` trait,
> - makes hygiene of fields in initializers behave like normal struct
> initializers would behave,
> - prevent stackoverflow without optimizations
> - add `..Zeroable::zeroed()` syntax to zero missing fields.
> - support arbitrary paths in initializer macros
>
> This is the second version of this patch series.
> * v1: https://lore.kernel.org/rust-for-linux/[email protected]/
>
> Changes not present on modified commits:
> v1 -> v2:
> - implement `Zeroable` for `Opaque`,
> - remove blanket impl of `PinInit` for `Init` and make it a supertrait
> instead,
> - add `{pin_}chain` functions to execute code after initialization,
> - update the example macro expansion
I forgot to mention that this patch series is based on rust-dev [1],
as it depends on Gary's `paste!` macro.
[1]: https://github.com/Rust-for-Linux/linux/tree/rust-dev
Repository: https://github.com/Rust-for-Linux/linux.git rust-dev
--
Cheers,
Benno
> Previously the init macros would create a local variable with the name
> and hygiene of the field that is being initialized to store the value of
> the field. This would override any user defined variables. For example:
> ```
> struct Foo {
> a: usize,
> b: usize,
> }
> let a = 10;
> let foo = init!(Foo{
> a: a + 1, // This creates a local variable named `a`.
> b: a, // This refers to that variable!
> });
> let foo = Box::init!(foo)?;
> assert_eq!(foo.a, 11);
> assert_eq!(foo.b, 11);
> ```
>
> This patch changes this behavior, so the above code would panic at the
> last assertion, since `b` would have value 10.
>
> Signed-off-by: Benno Lossin <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
Benno Lossin <[email protected]> writes:
> Add the struct update syntax to the init macros, but only for
> `..Zeroable::zeroed()`. Adding this at the end of the struct initializer
> allows one to omit fields from the initializer, these fields will be
> initialized with 0x00 set to every byte. Only types that implement the
> `Zeroable` trait can utilize this.
>
> Suggested-by: Asahi Lina <[email protected]>
> Signed-off-by: Benno Lossin <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
> + (make_initializer:
> + @slot($slot:ident),
> + @type_name($t:ident),
> + @munch_fields(..Zeroable::zeroed() $(,)?),
> + @acc($($acc:tt)*),
> + ) => {
> + // Endpoint, nothing more to munch, create the initializer. Since the users specified
> + // `..Zeroable::zeroed()`, the slot will already have been zeroed and all field that have
> + // not been overwritten are thus zero and initialized. We still check that all fields are
> + // actually accessible by using the struct update syntax ourselves.
> + // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
> + // get the correct type inference here:
Didn't you just change it to a closure rather than an `if else`?
Regardless, I'm happy with this change.
Alice
Benno Lossin <[email protected]> writes:
> Remove the blanket implementation of `PinInit<T, E> for I where I:
> Init<T, E>`. This blanket implementation prevented custom types that
> implement `PinInit`.
>
> Signed-off-by: Benno Lossin <[email protected]>
Reviewed-by: Alice Ryhl <[email protected]>
On 7/19/23 11:20, Benno Lossin wrote:
> Add the struct update syntax to the init macros, but only for
> `..Zeroable::zeroed()`. Adding this at the end of the struct initializer
> allows one to omit fields from the initializer, these fields will be
> initialized with 0x00 set to every byte. Only types that implement the
> `Zeroable` trait can utilize this.
>
> Suggested-by: Asahi Lina <[email protected]>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
On 7/19/23 11:21, Benno Lossin wrote:
> The `{pin_}chain` functions extend an initializer: it not only
> initializes the value, but also executes a closure taking a reference to
> the initialized value. This allows to do something with a value directly
> after initialization.
>
> Suggested-by: Asahi Lina <[email protected]>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> rust/kernel/init.rs | 138 +++++++++++++++++++++++++++++++++
> rust/kernel/init/__internal.rs | 2 +-
> 2 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 3c7cd36a424b..3b0df839f64c 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -773,6 +773,77 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {
> /// deallocate.
> /// - `slot` will not move until it is dropped, i.e. it will be pinned.
> unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E>;
> +
> + /// First initializes the value using `self` then calls the function `f` with the initialized
> + /// value.
> + ///
> + /// # Examples
> + ///
> + /// ```rust
> + /// # #![allow(clippy::disallowed_names)]
> + /// use kernel::{types::Opaque, init::pin_init_from_closure};
> + /// #[repr(C)]
> + /// struct RawFoo([u8; 16]);
> + /// extern {
> + /// fn init_foo(_: *mut RawFoo);
> + /// }
> + ///
> + /// #[pin_data]
> + /// struct Foo {
> + /// #[pin]
> + /// raw: Opaque<RawFoo>,
> + /// }
> + ///
> + /// impl Foo {
> + /// fn setup(self: Pin<&mut Self>) {
> + /// pr_info!("Setting up foo");
> + /// }
> + /// }
> + ///
> + /// let foo = pin_init!(Foo {
> + /// raw <- unsafe {
> + /// Opaque::ffi_init(|s| {
> + /// init_foo(s);
> + /// })
> + /// },
> + /// }).pin_chain(|foo| {
> + /// foo.setup();
> + /// Ok(())
> + /// });
> + /// ```
> + fn pin_chain<F>(self, f: F) -> ChainPinInit<Self, F, T, E>
> + where
> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
> + {
> + ChainPinInit(self, f, PhantomData)
> + }
> +}
> +
> +/// An initializer returned by [`PinInit::pin_chain`].
> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
> +
> +// SAFETY: the `__pinned_init` function is implemented such that it
> +// - returns `Ok(())` on successful initialization,
> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
> +// - considers `slot` pinned.
> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
> +where
> + I: PinInit<T, E>,
> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
> +{
> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
> + unsafe { self.0.__pinned_init(slot)? };
> + // SAFETY: The above call initialized `slot` and we still have unique access.
> + let val = unsafe { &mut *slot };
> + // SAFETY: `slot` is considered pinned
> + let val = unsafe { Pin::new_unchecked(val) };
> + (self.1)(val).map_err(|e| {
> + // SAFETY: `slot` was initialized above.
> + unsafe { core::ptr::drop_in_place(slot) };
> + e
I might stumble upon an error like EAGAIN if I call `pin_chain` but that
means `slot` will be dropped. So my recommendation is to either not drop
the value or detail in `pin_chain`'s doc comment that the closure will
drop on error.
> + })
> + }
> }
>
> /// An initializer for `T`.
> @@ -814,6 +885,73 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {
> /// - the caller does not touch `slot` when `Err` is returned, they are only permitted to
> /// deallocate.
> unsafe fn __init(self, slot: *mut T) -> Result<(), E>;
> +
> + /// First initializes the value using `self` then calls the function `f` with the initialized
> + /// value.
> + ///
> + /// # Examples
> + ///
> + /// ```rust
> + /// # #![allow(clippy::disallowed_names)]
> + /// use kernel::{types::Opaque, init::{self, init_from_closure}};
> + /// struct Foo {
> + /// buf: [u8; 1_000_000],
> + /// }
> + ///
> + /// impl Foo {
> + /// fn setup(&mut self) {
> + /// pr_info!("Setting up foo");
> + /// }
> + /// }
> + ///
> + /// let foo = init!(Foo {
> + /// buf <- init::zeroed()
> + /// }).chain(|foo| {
> + /// foo.setup();
> + /// Ok(())
> + /// });
> + /// ```
> + fn chain<F>(self, f: F) -> ChainInit<Self, F, T, E>
> + where
> + F: FnOnce(&mut T) -> Result<(), E>,
> + {
> + ChainInit(self, f, PhantomData)
> + }
> +}
> +
> +/// An initializer returned by [`Init::chain`].
> +pub struct ChainInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
> +
> +// SAFETY: the `__init` function is implemented such that it
> +// - returns `Ok(())` on successful initialization,
> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
> +unsafe impl<T: ?Sized, E, I, F> Init<T, E> for ChainInit<I, F, T, E>
> +where
> + I: Init<T, E>,
> + F: FnOnce(&mut T) -> Result<(), E>,
> +{
> + unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
> + // SAFETY: all requirements fulfilled since this function is `__init`.
> + unsafe { self.0.__pinned_init(slot)? };
> + // SAFETY: The above call initialized `slot` and we still have unique access.
> + (self.1)(unsafe { &mut *slot }).map_err(|e| {
> + // SAFETY: `slot` was initialized above.
> + unsafe { core::ptr::drop_in_place(slot) };
> + e
> + })
> + }
> +}
> +
> [...]
Same as above.
On 7/19/23 11:20, Benno Lossin wrote:
> Previously the init macros would create a local variable with the name
> and hygiene of the field that is being initialized to store the value of
> the field. This would override any user defined variables. For example:
> ```
> struct Foo {
> a: usize,
> b: usize,
> }
> let a = 10;
> let foo = init!(Foo{
> a: a + 1, // This creates a local variable named `a`.
> b: a, // This refers to that variable!
> });
> let foo = Box::init!(foo)?;
> assert_eq!(foo.a, 11);
> assert_eq!(foo.b, 11);
> ```
>
> This patch changes this behavior, so the above code would panic at the
> last assertion, since `b` would have value 10.
>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
On 7/19/23 11:21, Benno Lossin wrote:
> Remove the blanket implementation of `PinInit<T, E> for I where I:
> Init<T, E>`. This blanket implementation prevented custom types that
> implement `PinInit`.
>
> Signed-off-by: Benno Lossin <[email protected]>
> ---
> [...]
> @@ -968,6 +956,12 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
> Ok(())
> }
> }
I'd put an empty line here, so to separate each block.
> +// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
> +unsafe impl<T, E> PinInit<T, E> for T {
> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
> + unsafe { self.__init(slot) }
> + }
> +}
>
> [...]
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
> On 7/19/23 11:21, Benno Lossin wrote:
>> +/// An initializer returned by [`PinInit::pin_chain`].
>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
>> +
>> +// SAFETY: the `__pinned_init` function is implemented such that it
>> +// - returns `Ok(())` on successful initialization,
>> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
>> +// - considers `slot` pinned.
>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
>> +where
>> + I: PinInit<T, E>,
>> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>> +{
>> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
>> + unsafe { self.0.__pinned_init(slot)? };
>> + // SAFETY: The above call initialized `slot` and we still have unique access.
>> + let val = unsafe { &mut *slot };
>> + // SAFETY: `slot` is considered pinned
>> + let val = unsafe { Pin::new_unchecked(val) };
>> + (self.1)(val).map_err(|e| {
>> + // SAFETY: `slot` was initialized above.
>> + unsafe { core::ptr::drop_in_place(slot) };
>> + e
>
> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
> means `slot` will be dropped. So my recommendation is to either not drop
> the value or detail in `pin_chain`'s doc comment that the closure will
> drop on error.
This is a bit confusing to me, because dropping the value on returning `Err`
is a safety requirement of `PinInit`. Could you elaborate why this is
surprising? I can of course add it to the documentation, but I do not see
how it could be implemented differently. Since if you do not drop the value
here, nobody would know that it is still initialized.
--
Cheers,
Benno
On 7/24/23 11:08, Benno Lossin wrote:
> On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
>> On 7/19/23 11:21, Benno Lossin wrote:
>>> +/// An initializer returned by [`PinInit::pin_chain`].
>>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant<(E, Box<T>)>);
>>> +
>>> +// SAFETY: the `__pinned_init` function is implemented such that it
>>> +// - returns `Ok(())` on successful initialization,
>>> +// - returns `Err(err)` on error and in this case `slot` will be dropped.
>>> +// - considers `slot` pinned.
>>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T, E>
>>> +where
>>> + I: PinInit<T, E>,
>>> + F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>>> +{
>>> + unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>>> + // SAFETY: all requirements fulfilled since this function is `__pinned_init`.
>>> + unsafe { self.0.__pinned_init(slot)? };
>>> + // SAFETY: The above call initialized `slot` and we still have unique access.
>>> + let val = unsafe { &mut *slot };
>>> + // SAFETY: `slot` is considered pinned
>>> + let val = unsafe { Pin::new_unchecked(val) };
>>> + (self.1)(val).map_err(|e| {
>>> + // SAFETY: `slot` was initialized above.
>>> + unsafe { core::ptr::drop_in_place(slot) };
>>> + e
>>
>> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
>> means `slot` will be dropped. So my recommendation is to either not drop
>> the value or detail in `pin_chain`'s doc comment that the closure will
>> drop on error.
>
> This is a bit confusing to me, because dropping the value on returning `Err`
> is a safety requirement of `PinInit`. Could you elaborate why this is
> surprising? I can of course add it to the documentation, but I do not see
> how it could be implemented differently. Since if you do not drop the value
> here, nobody would know that it is still initialized.
I knew about the requirement of dropping on `Err`, but what has caught my
attention is that `{pin_}chain` might not abide with it per the doc
comment as it says that `self` is initialized before calling `f`...
/// First initializes the value using `self` then calls the function
/// `f` with the initialized value.
But one can not know what would happen when `f` fails, specially if
such failure can be ignored or it's only temporarily.
So then, the best course IMO is to mention that in some way the value is
still being initialized, kinda setting it up, and that it will be dropped
when an error is returned. WDYT?
On 7/24/23 18:07, Martin Rodriguez Reboredo wrote:
> On 7/24/23 11:08, Benno Lossin wrote:
>> This is a bit confusing to me, because dropping the value on returning `Err`
>> is a safety requirement of `PinInit`. Could you elaborate why this is
>> surprising? I can of course add it to the documentation, but I do not see
>> how it could be implemented differently. Since if you do not drop the value
>> here, nobody would know that it is still initialized.
>
> I knew about the requirement of dropping on `Err`, but what has caught my
> attention is that `{pin_}chain` might not abide with it per the doc
> comment as it says that `self` is initialized before calling `f`...
>
> /// First initializes the value using `self` then calls the function
> /// `f` with the initialized value.
>
> But one can not know what would happen when `f` fails, specially if
> such failure can be ignored or it's only temporarily.
>
> So then, the best course IMO is to mention that in some way the value is
> still being initialized, kinda setting it up, and that it will be dropped
> when an error is returned. WDYT?
I see, then I will just expand the documentation.
--
Cheers,
Benno