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

[PATCH] 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: Rust-for-Linux/linux#1059

Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Matt Gilbride <mattgilbride@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
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..946067cd9c00 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -46,6 +46,18 @@ 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.
+    ///
+    /// Like [`from_foreign`], but 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>;
 }
 
 impl<T: 'static> ForeignOwnable for Box<T> {
@@ -68,6 +80,14 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
         // call to `Self::into_foreign`.
         unsafe { Box::from_raw(ptr as _) }
     }
+
+    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
+        if ptr.is_null() {
+            None
+        } else {
+            Some(Self::from_foreign(ptr))
+        }
+    }
 }
 
 impl ForeignOwnable for () {
-- 
2.39.2

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

[Martin Rodriguez Reboredo]

On 1/22/24 13:26, 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) })
> }
> ``
> 
> Link: Rust-for-Linux/linux#1059
> 
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Matt Gilbride <mattgilbride@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Obei Sideg <linux@obei.io>
> ---
> [...]
> +    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self>;
>   }
>   
>   impl<T: 'static> ForeignOwnable for Box<T> {
> @@ -68,6 +80,14 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
>           // call to `Self::into_foreign`.
>           unsafe { Box::from_raw(ptr as _) }
>       }
> +
> +    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
> +        if ptr.is_null() {
> +            None
> +        } else {
> +            Some(Self::from_foreign(ptr))
> +        }
> +    }
> [...]

Shouldn't have `Box::try_from_foreign` been the default implementation
of `ForeignOwnable::try_from_foreign`?

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

[Trevor Gross]

On Mon, Jan 22, 2024 at 2:11 PM Obei Sideg <linux@obei.io> 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:

Minor nit: commit messages are wrapped to 72 characters, looks like this is 50.

> ```rust
> // `p` is a maybe null pointer
> if p.is_null() {
>   None
> } else {
>   Some(unsafe { T::from_foreign(p) })
> }
> ``
>
> Link: Rust-for-Linux/linux#1059

You should link the issue
(https://github.com/Rust-for-Linux/linux/issues/1057) instead of or in
addition to the PR.

>
> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Matt Gilbride <mattgilbride@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Obei Sideg <linux@obei.io>

Optional docs update below, in either case you may add:

Reviewed-by: Trevor Gross <tmgross@umich.edu>

> ---
>  rust/kernel/types.rs | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index fdb778e65d79..946067cd9c00 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -46,6 +46,18 @@ 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.
> +    ///
> +    /// Like [`from_foreign`], but returns [`None`] if `ptr` is null.

Maybe

    A convenience wrapper over [`from_foreign`] that returns [`None`]
if `ptr` is null.

Just to be clear that this introduces no additional functionality.

> +    /// # 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>;
>  }
>
>  impl<T: 'static> ForeignOwnable for Box<T> {
> @@ -68,6 +80,14 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
>          // call to `Self::into_foreign`.
>          unsafe { Box::from_raw(ptr as _) }
>      }
> +
> +    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
> +        if ptr.is_null() {
> +            None
> +        } else {
> +            Some(Self::from_foreign(ptr))
> +        }
> +    }
>  }
>
>  impl ForeignOwnable for () {
> --
> 2.39.2
>
>
>

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

[Alice Ryhl]

On 1/22/24 17:26, Obei Sideg wrote:
> +    /// Tries to convert a foreign-owned object back to a Rust-owned one.
> +    ///
> +    /// Like [`from_foreign`], but 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>;

The safety comment should probably say "If `ptr` is not null, then ...".

>   impl<T: 'static> ForeignOwnable for Box<T> {
> @@ -68,6 +80,14 @@ unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
>           // call to `Self::into_foreign`.
>           unsafe { Box::from_raw(ptr as _) }
>       }
> +
> +    unsafe fn try_from_foreign(ptr: *const core::ffi::c_void) -> Option<Self> {
> +        if ptr.is_null() {
> +            None
> +        } else {
> +            Some(Self::from_foreign(ptr))
> +        }
> +    }

I would just add a default implementation in the trait declaration. That 
way, you can avoid adding the method to every implementation.

Alice

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

[Trevor Gross]

On Mon, Jan 22, 2024 at 3:33 PM Trevor Gross <tmgross@umich.edu> wrote:
>
> On Mon, Jan 22, 2024 at 2:11 PM Obei Sideg <linux@obei.io> wrote:
> >
> > [...]
> >
> > Cc: Alice Ryhl <aliceryhl@google.com>
> > Cc: Matt Gilbride <mattgilbride@google.com>
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Obei Sideg <linux@obei.io>
>
> Optional docs update below, in either case you may add:
>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>

The others have a good point, please resend with a default
implementation on ForeignOwnable and I will re-review.

[PATCH] 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

Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Matt Gilbride <mattgilbride@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Signed-off-by: Obei Sideg <linux@obei.io>
---
 rust/kernel/types.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index fdb778e65d79..cfee102ab7b9 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -46,6 +46,25 @@ 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 {
+            Some(Self::from_foreign(ptr))
+        }
+    }
 }
 
 impl<T: 'static> ForeignOwnable for Box<T> {
-- 
2.39.2

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

[Jarkko Sakkinen]

On Mon Jan 22, 2024 at 8:33 PM UTC, Trevor Gross wrote:
> On Mon, Jan 22, 2024 at 2:11 PM Obei Sideg <linux@obei.io> 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:
>
> Minor nit: commit messages are wrapped to 72 characters, looks like this is 50.

It is actually 75 characters max:

"The body of the explanation, line wrapped at 75 columns, which will be
copied to the permanent changelog to describe this patch."

See https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

BR, Jarkko

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

[Miguel Ojeda]

Hi Obei,

Some extra nits below that you can fix for v3 (since a v3 will be
needed anyway due to the compilation error reported in Zulip).

On Tue, Jan 23, 2024 at 9:28 AM Obei Sideg <linux@obei.io> wrote:
>
> Currently `ForeignOwnable::from_foreign()` only works for non-null
> pointers for the existing impls (e.g. Box, Arc). It may create a few

Markdown may be used here for `Box` and `Arc` since you use it for the other.

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

Triple-backquote here.

Also please do the usual indentation for the code.

> Link: https://github.com/Rust-for-Linux/linux/issues/1057
>

No newline between the tags.

> Cc: Alice Ryhl <aliceryhl@google.com>
> Cc: Matt Gilbride <mattgilbride@google.com>
> Cc: Miguel Ojeda <ojeda@kernel.org>

Normally you don't need to add a Cc tag with the maintainer that is
supposed to pick the patch.

For Alice and Matt, did you have a particular reason to explicitly list them?

> Reviewed-by: Trevor Gross <tmgross@umich.edu>

Since the patch changed substantially, you should not have picked the
`Reviewed-by`, i.e. he did not approve this patch (in particular, he
mentioned this in Zulip himself, so... :)

Also, please note that you should indicate v2/v3/... in the title
(please see https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format).

Finally, I think Alice's comment should be applied, i.e. the
precondition on `ptr` only applies if `ptr` is non-null for this
function, so I think we should add it.

Thanks!

Cheers,
Miguel