[PATCH v3] rust: types: Add `try_from_foreign()` method

[PATCH v3] rust: types: Add `try_from_foreign()` method

[Obei Sideg]

Currently `ForeignOwnable::from_foreign()` only works for non-null
pointers for the existing impls (e.g. Box, Arc). It may create a few
duplicate code like:

```rust
// `p` is a maybe null pointer
if p.is_null() {
  None
} else {
  Some(unsafe { T::from_foreign(p) })
}
```

Link: https://github.com/Rust-for-Linux/linux/issues/1057
Signed-off-by: Obei Sideg <linux@obei.io>
---
 rust/kernel/types.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index fdb778e65d79..f27f007ca543 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -46,6 +46,26 @@ pub trait ForeignOwnable: Sized {
     /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
     /// this object must have been dropped.
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;
+
+    /// Tries to convert a foreign-owned object back to a Rust-owned one.
+    ///
+    /// A convenience wrapper over [`from_foreign`] that returns [`None`] if `ptr` is null.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
+    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
+    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
+    /// this object must have been dropped.
+    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
+        if ptr.is_null() {
+            None
+        } else {
+            // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+            // call to `Self::into_foreign`.
+            unsafe { Some(Self::from_foreign(ptr)) }
+        }
+    }
 }
 
 impl<T: 'static> ForeignOwnable for Box<T> {
-- 
2.39.2

Re: [PATCH v3] rust: types: Add `try_from_foreign()` method

[Alice Ryhl]

On Wed, Jan 24, 2024 at 12:32 PM Obei Sideg <linux@obei.io> wrote:
> +    /// # Safety
> +    ///
> +    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
> +    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
> +    /// this object must have been dropped.

The safety comment should say something along the lines of "If `ptr`
is not null, then ...".

This was mentioned on v1 and v2 as well. Before you send another
version, please either make this change or explain why you don't think
it is appropriate.

Alice

Re: [PATCH v3] rust: types: Add `try_from_foreign()` method

[Obei Sideg]

I will change it to: `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].

Re: [PATCH v3] rust: types: Add `try_from_foreign()` method

[Alice Ryhl]

On Wed, Jan 24, 2024 at 3:17 PM Obei Sideg <linux@obei.io> wrote:
>
> I will change it to: `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].
>

Okay, thanks. With that change, you may add

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH v3] rust: types: Add `try_from_foreign()` method

[Boqun Feng]

On Wed, Jan 24, 2024 at 11:32:53AM +0000, Obei Sideg wrote:
> Currently `ForeignOwnable::from_foreign()` only works for non-null
> pointers for the existing impls (e.g. Box, Arc). It may create a few
> duplicate code like:
> 
> ```rust
> // `p` is a maybe null pointer
> if p.is_null() {
>   None
> } else {
>   Some(unsafe { T::from_foreign(p) })
> }
> ```

In the commit log, you explain the current status, which is good for
explaining "why the patch is needed", however, you should also add a few
sentenses explaining how your patch would help/improve the current
situation, otherwise it looks like an unfinished sentense to me.

Thanks!

Regards,
Boqun

> 
> Link: https://github.com/Rust-for-Linux/linux/issues/1057
> Signed-off-by: Obei Sideg <linux@obei.io>
> ---
>  rust/kernel/types.rs | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index fdb778e65d79..f27f007ca543 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -46,6 +46,26 @@ pub trait ForeignOwnable: Sized {
>      /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
>      /// this object must have been dropped.
>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;
> +
> +    /// Tries to convert a foreign-owned object back to a Rust-owned one.
> +    ///
> +    /// A convenience wrapper over [`from_foreign`] that returns [`None`] if `ptr` is null.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
> +    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
> +    /// this object must have been dropped.
> +    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
> +        if ptr.is_null() {
> +            None
> +        } else {
> +            // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
> +            // call to `Self::into_foreign`.
> +            unsafe { Some(Self::from_foreign(ptr)) }
> +        }
> +    }
>  }
>  
>  impl<T: 'static> ForeignOwnable for Box<T> {
> -- 
> 2.39.2
> 
> 

[PATCH v4] rust: types: Add `try_from_foreign()` method

[Obei Sideg]

Currently `ForeignOwnable::from_foreign()` only works for non-null
pointers for the existing impls (e.g. Box, Arc). It may create a few
duplicate code like:

```rust
// `p` is a maybe null pointer
if p.is_null() {
  None
} else {
  unsafe { Some(Self::from_foreign(ptr)) }
}
```

Adding a `try_from_foreign()` method that will return null if `ptr`
is null, otherwsie return `from_foreign(ptr)`.

Link: https://github.com/Rust-for-Linux/linux/issues/1057
Signed-off-by: Obei Sideg <linux@obei.io>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/types.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index fdb778e65d79..04a356ec3534 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -46,6 +46,23 @@ pub trait ForeignOwnable: Sized {
     /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] for
     /// this object must have been dropped.
     unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;
+
+    /// Tries to convert a foreign-owned object back to a Rust-owned one.
+    ///
+    /// A convenience wrapper over [`from_foreign`] that returns [`None`] if `ptr` is null.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].
+    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
+        if ptr.is_null() {
+            None
+        } else {
+            // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+            // call to [`into_foreign`].
+            unsafe { Some(Self::from_foreign(ptr)) }
+        }
+    }
 }
 
 impl<T: 'static> ForeignOwnable for Box<T> {
-- 
2.39.2