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

[PATCH v6] 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 `None` if `ptr`
is null, otherwsie return `Some(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

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

[Greg KH]

On Mon, Jan 29, 2024 at 06:45:27AM +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 {
>   unsafe { Some(Self::from_foreign(ptr)) }
> }
> ```
> 
> Adding a `try_from_foreign()` method that will return `None` if `ptr`
> is null, otherwsie return `Some(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(+)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Trevor Gross]

On Mon, Jan 29, 2024 at 1:52 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
> duplicate code like:

"may create a few duplicate code like" should be reworded to

> ```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 `None` if `ptr`
> is null, otherwsie return `Some(from_foreign(ptr))`.

s/Adding/Add (commit messages use the imperative mood)
s/otherwsie/otherwise

>
> 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>
> ---

Content looks good:so:

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

You don't need to resend the patch to address my above comments on the
commit message or add that RB, Miguel usually fixes up wording before
applying and adds relevant tags.

As Greg's bot mentioned, you should have a small version changelog
with links to previous versions of the patch after the `---` and
before the diffstat (this part doesn't make it into the commit
message). Example in [1]. Again, you don't need to resend for this but
you could reply to this thread with one so we know what happened
through v3/v4/v5/v6 (and include it if you wind up needing to send
another).

[1]: https://lore.kernel.org/rust-for-linux/20240118-alice-file-v3-0-9694b6f9580c@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
>
>
>

Re: [PATCH v6] 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)) }
}
```

Add a `try_from_foreign()` method that will return `None` if `ptr`
is null, otherwise return `Some(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>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
---
  V5 -> V6: Remove duplicate `Signed-off-by` and Add changelog
  V4 -> V5: Add the solution description to the commit message
  V3 -> V4: Improve safety comment
  V2 -> V3: Improve method comment
  V1 -> V2: Add the trait default implementation for `try_from_foreign()`

 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

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

[Boqun Feng]

Is this a version 6 or version 7?

As Trevor mentioned, if you accidentally made a mistake but it doesn't
affect reviews, you can reply an email mentioning that. Do NOT send a
new version simply because of that except that you're requested by
maintainers or reviewers, since that would confuse people.

Regards,
Boqun

On Sun, Feb 04, 2024 at 07:58:36AM +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 {
>   unsafe { Some(Self::from_foreign(ptr)) }
> }
> ```
> 
> Add a `try_from_foreign()` method that will return `None` if `ptr`
> is null, otherwise return `Some(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>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> ---
>   V5 -> V6: Remove duplicate `Signed-off-by` and Add changelog
>   V4 -> V5: Add the solution description to the commit message
>   V3 -> V4: Improve safety comment
>   V2 -> V3: Improve method comment
>   V1 -> V2: Add the trait default implementation for `try_from_foreign()`
> 
>  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
> 
> 

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

[Miguel Ojeda]

On Sun, Feb 4, 2024 at 8:58 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
> duplicate code like:
>
> ```rust
> // `p` is a maybe null pointer
> if p.is_null() {
>   None
> } else {
>   unsafe { Some(Self::from_foreign(ptr)) }
> }
> ```
>
> Add a `try_from_foreign()` method that will return `None` if `ptr`
> is null, otherwise return `Some(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>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>

Applied to `rust-next` -- thanks everyone!

[ Fixed intra-doc links, improved `SAFETY` comment and reworded commit. ]

Cheers,
Miguel

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

[Obei Sideg]

It's version 6.

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

[Miguel Ojeda]

On Sun, Feb 4, 2024 at 11:11 AM Obei Sideg <linux@obei.io> wrote:
>
> It's version 6.

It cannot be, since you sent v6 already:

    https://lore.kernel.org/rust-for-linux/0100018d53f737f8-80c1fe97-0019-40d7-ab69-b1b192785cd7-000000@email.amazonses.com/

I guess you mean that the diff (i.e. the code changes) did not change,
but please note that even if you only change the commit message/title,
you should increase the version number, because it is still a change
that will end up in the repository.

Having said that, if the changes are trivial (e.g. a typo fix), it is
probably not worth it to send another version -- it is all about a
balance. You can also simply point the issue as a reply and the
maintainer can fix them for you if it happens to be the version that
gets picked up.

In addition, some mistakes can be prevented before submitting the
patch -- please use `scripts/checkpatch.pl --codespell` on your
patches to e.g. prevent typos.

Thanks!

Cheers,
Miguel