[4/5] rust: types: implement `ForeignOwnable` for the unit type

Message ID 20230119174036.64046-4-wedsonaf@gmail.com
State New
Headers
Series [1/5] rust: types: introduce `ScopeGuard` |

Commit Message

Wedson Almeida Filho Jan. 19, 2023, 5:40 p.m. UTC
  This allows us to use the unit type `()` when we have no object whose
ownership must be managed but one implementing the `ForeignOwnable`
trait is needed.

Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
---
 rust/kernel/types.rs | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

Comments

Gary Guo Jan. 27, 2023, 2:03 p.m. UTC | #1
On Thu, 19 Jan 2023 14:40:35 -0300
Wedson Almeida Filho <wedsonaf@gmail.com> wrote:

> This allows us to use the unit type `()` when we have no object whose
> ownership must be managed but one implementing the `ForeignOwnable`
> trait is needed.
> 
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> ---
>  rust/kernel/types.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index e037c262f23e..8f80cffbff59 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -85,6 +85,19 @@ impl<T: 'static> ForeignOwnable for Box<T> {
>      }
>  }
>  
> +impl ForeignOwnable for () {
> +    type Borrowed<'a> = ();
> +
> +    fn into_foreign(self) -> *const core::ffi::c_void {
> +        // We use 1 to be different from a null pointer.
> +        1usize as _

this should really be `core::ptr::invalid(1)`. That's currently
unstable, but can be equivalently written as
`NonNull::<()>::dangling().as_ptr()`.

This has a different semantic meaning from `as` since it explicitly
suggests an invalid provenance and thus will not alias with other
pointers. (Although I don't think compiler currently can take advantage
of this fact yet)

> +    }
> +
> +    unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {}
> +
> +    unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
> +}
> +
>  /// Runs a cleanup function/closure when dropped.
>  ///
>  /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
  
Miguel Ojeda Jan. 27, 2023, 2:11 p.m. UTC | #2
On Fri, Jan 27, 2023 at 3:03 PM Gary Guo <gary@garyguo.net> wrote:
>
> this should really be `core::ptr::invalid(1)`. That's currently
> unstable, but can be equivalently written as
> `NonNull::<()>::dangling().as_ptr()`.
>
> This has a different semantic meaning from `as` since it explicitly
> suggests an invalid provenance and thus will not alias with other
> pointers. (Although I don't think compiler currently can take advantage
> of this fact yet)

We talked about starting to use `strict_provenance` when it came out
-- what is the latest status? i.e. do you know if it is expected that
it will pass FCP etc.? (my understanding originally was that it was an
experiment).

If it is likely to become stable, then I agree it could be nice to
start using it before a lot of kernel code gets written without it.

Thanks Gary!

Cheers,
Miguel
  
Vincenzo Palazzo Jan. 28, 2023, 11:13 a.m. UTC | #3
On Fri Jan 27, 2023 at 3:11 PM CET, Miguel Ojeda wrote:
> On Fri, Jan 27, 2023 at 3:03 PM Gary Guo <gary@garyguo.net> wrote:
> >
> > this should really be `core::ptr::invalid(1)`. That's currently
> > unstable, but can be equivalently written as
> > `NonNull::<()>::dangling().as_ptr()`.
> >
> > This has a different semantic meaning from `as` since it explicitly
> > suggests an invalid provenance and thus will not alias with other
> > pointers. (Although I don't think compiler currently can take advantage
> > of this fact yet)
>
> We talked about starting to use `strict_provenance` when it came out
> -- what is the latest status? i.e. do you know if it is expected that
> it will pass FCP etc.? (my understanding originally was that it was an
> experiment).
From what I remember the feeling was positing into hace `strict_provenance`

Here is the last meeting  that was back in August  
https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Stabilizing.20strict.20provenance.20APIs.3F

I guess, we could just put a fix me around the actual code, I feel that the Gary change deserve a 
own patch with the own description.

Cheers!

Vincent.
  
Vincenzo Palazzo Jan. 28, 2023, 11:14 a.m. UTC | #4
On Thu Jan 19, 2023 at 6:40 PM CET, Wedson Almeida Filho wrote:
> This allows us to use the unit type `()` when we have no object whose
> ownership must be managed but one implementing the `ForeignOwnable`
> trait is needed.
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>

Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

> ---
>  rust/kernel/types.rs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index e037c262f23e..8f80cffbff59 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -85,6 +85,19 @@ impl<T: 'static> ForeignOwnable for Box<T> {
>      }
>  }
>  
> +impl ForeignOwnable for () {
> +    type Borrowed<'a> = ();
> +
> +    fn into_foreign(self) -> *const core::ffi::c_void {
> +        // We use 1 to be different from a null pointer.
> +        1usize as _
> +    }
> +
> +    unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {}
> +
> +    unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
> +}
> +
>  /// Runs a cleanup function/closure when dropped.
>  ///
>  /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
> -- 
> 2.34.1
  
Wedson Almeida Filho Jan. 30, 2023, 5:55 a.m. UTC | #5
On Fri, 27 Jan 2023 at 11:03, Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 19 Jan 2023 14:40:35 -0300
> Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
>
> > This allows us to use the unit type `()` when we have no object whose
> > ownership must be managed but one implementing the `ForeignOwnable`
> > trait is needed.
> >
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > ---
> >  rust/kernel/types.rs | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index e037c262f23e..8f80cffbff59 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -85,6 +85,19 @@ impl<T: 'static> ForeignOwnable for Box<T> {
> >      }
> >  }
> >
> > +impl ForeignOwnable for () {
> > +    type Borrowed<'a> = ();
> > +
> > +    fn into_foreign(self) -> *const core::ffi::c_void {
> > +        // We use 1 to be different from a null pointer.
> > +        1usize as _
>
> this should really be `core::ptr::invalid(1)`. That's currently
> unstable, but can be equivalently written as
> `NonNull::<()>::dangling().as_ptr()`.

This has the wrong type, but I agree with the spirit of the
suggestion. I'll add it to V2.

> This has a different semantic meaning from `as` since it explicitly
> suggests an invalid provenance and thus will not alias with other
> pointers. (Although I don't think compiler currently can take advantage
> of this fact yet)
>
> > +    }
> > +
> > +    unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {}
> > +
> > +    unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
> > +}
> > +
> >  /// Runs a cleanup function/closure when dropped.
> >  ///
> >  /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
>
  
Miguel Ojeda Jan. 30, 2023, 5:21 p.m. UTC | #6
On Sat, Jan 28, 2023 at 12:13 PM Vincenzo Palazzo
<vincenzopalazzodev@gmail.com> wrote:
>
> From what I remember the feeling was positing into hace `strict_provenance`
>
> Here is the last meeting  that was back in August
> https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Stabilizing.20strict.20provenance.20APIs.3F

Thanks Vincent! Yeah, as far as I understand Gary was in that meeting
(in October).

If there has not been anything after that, then I guess it is still
the case that provenance will probably happen (for better or worse,
depending on the perspective :) and that a subset of the APIs may get
stabilized first.

Cheers,
Miguel
  

Patch

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index e037c262f23e..8f80cffbff59 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -85,6 +85,19 @@  impl<T: 'static> ForeignOwnable for Box<T> {
     }
 }
 
+impl ForeignOwnable for () {
+    type Borrowed<'a> = ();
+
+    fn into_foreign(self) -> *const core::ffi::c_void {
+        // We use 1 to be different from a null pointer.
+        1usize as _
+    }
+
+    unsafe fn borrow<'a>(_: *const core::ffi::c_void) -> Self::Borrowed<'a> {}
+
+    unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
+}
+
 /// Runs a cleanup function/closure when dropped.
 ///
 /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.