Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Oct 23, 2024 at 01:44:45AM +0300, Abdiel Janulgue wrote:
> Add the 'Owned' type, a simple smart pointer type that owns the
> underlying data.
> 
> An object implementing `Ownable' can constructed by wrapping it in
> `Owned`, which has the advantage of allowing fine-grained control
> over it's resource allocation and deallocation.
> 
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ced143600eb1..3f632916bd4d 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -429,3 +429,65 @@ pub enum Either<L, R> {
>      /// Constructs an instance of [`Either`] containing a value of type `R`.
>      Right(R),
>  }
> +
> +/// A smart pointer that owns the underlying data `T`.
> +///
> +/// This is a simple smart pointer that owns the underlying data. Typically, this would be
> +/// returned as a wrapper for `T` in `T`'s constructor.
> +/// When an object adds an option of being constructed this way, in addition to implementing
> +/// `Drop`, it implements `Ownable` as well, thus having finer-grained control in where
> +/// resource allocation and deallocation happens.
> +///
> +/// # Invariants
> +///
> +/// The pointer is always valid and owns the underlying data.
> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new smart pointer that owns `T`.
> +    ///
> +    /// # Safety
> +    /// `ptr` needs to be a valid pointer, and it should be the unique owner to the object,
> +    /// in other words, no other entity should free the underlying data.
> +    pub unsafe fn to_owned(ptr: *mut T) -> Self {
> +	// SAFETY: Per function safety requirement.
> +	Self { ptr: unsafe { NonNull::new_unchecked(ptr) } }
> +    }

I wonder if this should just be

   pub fn new(ptr: NonNull<T>) -> Self

This way the caller could decide whether to use the fallible variant
`NonNull::new` or `NonNull::new_unchecked`.

Alternatively, you could have your own `new` and `new_unchecked` methods, but
that seems a bit redundant.

Sometimes this might be more elegant. For instance in the page code, as it is
now, you have to give up on

   let page = NonNull::new(page).ok_or(AllocError)?;

and instead have to do a NULL check by hand for the subsequent unsafe call to
`Owned::to_owned`.

> +}
> +
> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: Ownable> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> +        // safe to dereference it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}
> +
> +/// An Ownable type is a type that can be put into `Owned<T>`, and when `Owned<T>` drops,
> +/// `ptr_drop` will be called.
> +pub unsafe trait Ownable {
> +    /// # Safety
> +    /// This could only be called in the `Owned::drop` function.
> +    unsafe fn ptr_drop(ptr: *mut Self);
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +	// SAFETY: In Owned<T>::drop.
> +	unsafe {
> +	    <T as Ownable>::ptr_drop(self.ptr.as_mut());
> +	}
> +    }
> +}
> -- 
> 2.43.0
>