From:   Martin Rodriguez Reboredo <yakoyoku@gmail.com>
To:     wedsonaf@gmail.com
Cc:     alex.gaynor@gmail.com, bjorn3_gh@protonmail.com,
        boqun.feng@gmail.com, gary@garyguo.net,
        linux-kernel@vger.kernel.org, ojeda@kernel.org,
        rust-for-linux@vger.kernel.org
Subject: Re: [PATCH 2/5] rust: types: introduce `ForeignOwnable`
Date:   Sat, 28 Jan 2023 11:53:45 -0300
Message-Id: <20230128145345.1770912-1-yakoyoku@gmail.com>
In-Reply-To: <20230119174036.64046-2-wedsonaf@gmail.com>
References: <20230119174036.64046-2-wedsonaf@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jan 19, 2023 at 02:40:33PM -0300, Wedson Almeida Filho wrote:=0D
> It was originally called `PointerWrapper`. It is used to convert=0D
> a Rust object to a pointer representation (void *) that can be=0D
> stored on the C side, used, and eventually returned to Rust.=0D
> =0D
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>=0D
> ---=0D
>  rust/kernel/lib.rs   |  1 +=0D
>  rust/kernel/types.rs | 54 ++++++++++++++++++++++++++++++++++++++++++++=0D
>  2 files changed, 55 insertions(+)=0D
> =0D
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs=0D
> index e0b0e953907d..223564f9f0cc 100644=0D
> --- a/rust/kernel/lib.rs=0D
> +++ b/rust/kernel/lib.rs=0D
> @@ -16,6 +16,7 @@=0D
>  #![feature(coerce_unsized)]=0D
>  #![feature(core_ffi_c)]=0D
>  #![feature(dispatch_from_dyn)]=0D
> +#![feature(generic_associated_types)]=0D
>  #![feature(receiver_trait)]=0D
>  #![feature(unsize)]=0D
>  =0D
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs=0D
> index f0ad4472292d..5475f6163002 100644=0D
> --- a/rust/kernel/types.rs=0D
> +++ b/rust/kernel/types.rs=0D
> @@ -9,6 +9,60 @@ use core::{=0D
>      ops::{Deref, DerefMut},=0D
>  };=0D
>  =0D
> +/// Used to transfer ownership to and from foreign (non-Rust) languages.=
=0D
> +///=0D
> +/// Ownership is transferred from Rust to a foreign language by calling =
[`Self::into_foreign`] and=0D
> +/// later may be transferred back to Rust by calling [`Self::from_foreig=
n`].=0D
> +///=0D
> +/// This trait is meant to be used in cases when Rust objects are stored=
 in C objects and=0D
> +/// eventually "freed" back to Rust.=0D
> +pub trait ForeignOwnable {=0D
> +    /// Type of values borrowed between calls to [`ForeignOwnable::into_=
foreign`] and=0D
> +    /// [`ForeignOwnable::from_foreign`].=0D
> +    type Borrowed<'a>;=0D
=0D
Is it there a possibility that this could make use of borrowing features=0D
like AsRef/Borrowed/Deref?=0D
=0D
> +=0D
> +    /// Converts a Rust-owned object to a foreign-owned one.=0D
> +    ///=0D
> +    /// The foreign representation is a pointer to void.=0D
> +    fn into_foreign(self) -> *const core::ffi::c_void;=0D
> +=0D
> +    /// Borrows a foreign-owned object.=0D
> +    ///=0D
> +    /// # Safety=0D
> +    ///=0D
> +    /// `ptr` must have been returned by a previous call to [`ForeignOwn=
able::into_foreign`] for=0D
> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn'=
t been called yet.=0D
> +    /// Additionally, all instances (if any) of values returned by [`For=
eignOwnable::borrow_mut`]=0D
> +    /// for this object must have been dropped.=0D
> +    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowe=
d<'a>;=0D
> +=0D
> +    /// Mutably borrows a foreign-owned object.=0D
> +    ///=0D
> +    /// # Safety=0D
> +    ///=0D
> +    /// `ptr` must have been returned by a previous call to [`ForeignOwn=
able::into_foreign`] for=0D
> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn'=
t been called yet.=0D
> +    /// Additionally, all instances (if any) of values returned by [`For=
eignOwnable::borrow`] and=0D
> +    /// [`ForeignOwnable::borrow_mut`] for this object must have been dr=
opped.=0D
> +    unsafe fn borrow_mut<T: ForeignOwnable>(ptr: *const core::ffi::c_voi=
d) -> ScopeGuard<T, fn(T)> {=0D
> +        // SAFETY: The safety requirements ensure that `ptr` came from a=
 previous call to=0D
> +        // `into_foreign`.=0D
> +        ScopeGuard::new_with_data(unsafe { T::from_foreign(ptr) }, |d| {=
=0D
> +            d.into_foreign();=0D
> +        })=0D
> +    }=0D
=0D
Could these three methods have a borrowing equivalent? When I was=0D
working on some features for the USB module I've stumbled upon the case=0D
of having to encode a pointer (with a pivot) and I cannot do it without=0D
taking ownership of the pointer.=0D
=0D
> +=0D
> +    /// Converts a foreign-owned object back to a Rust-owned one.=0D
> +    ///=0D
> +    /// # Safety=0D
> +    ///=0D
> +    /// `ptr` must have been returned by a previous call to [`ForeignOwn=
able::into_foreign`] for=0D
> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn'=
t been called yet.=0D
> +    /// Additionally, all instances (if any) of values returned by [`For=
eignOwnable::borrow`] and=0D
> +    /// [`ForeignOwnable::borrow_mut`] for this object must have been dr=
opped.=0D
> +    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;=0D
> +}=0D
> +=0D
>  /// Runs a cleanup function/closure when dropped.=0D
>  ///=0D
>  /// The [`ScopeGuard::dismiss`] function prevents the cleanup function f=
rom running.=0D
> -- =0D
> 2.34.1=0D
=0D
Aside from these comments I observe that there's a possibility to make=0D
ForeignOwnable a const trait and have non const implementors. Otherwise=0D
if these things are out of scope, no problem whatsoever and this has my=0D
OK.=0D
=0D
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>=0D