[RFC PATCH] rust: types: extend `Opaque` documentation

[RFC PATCH] rust: types: extend `Opaque` documentation

[Dirk Behme]

In the discussion linked below some `Opaque` documentation
updates have been proposed. Add them.

Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

---

This is marked as RFC as it mainly takes some statements from
the linked discussion. Let us take this as a starting point
to discuss how we want to extend the `Opaque` documentation.

In that discussion it was mentioned

"Because `Opaque` implies the value may not be initialized,
 it's similar to `MaybeUninit`."

I wonder if we want to add some explanation for the *difference*
between `Opaque` and `MaybeUninit`?
---
 rust/kernel/types.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 994e504ca9075..a2d78a39a9b14 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -244,6 +244,13 @@ fn drop(&mut self) {
 /// Using `Opaque<T>` allows to continue to use references on the Rust side even for values shared
 /// with C.
 ///
+/// An `Opaque<T>` means that "this is a blob of bytes and don't touch it". It *might* or
+/// *might not* contain a valid `T`. By dereferencing a raw pointer to the inner value, you
+/// are unsafely asserting that it does in fact contain a valid `T` *right now*. As `T` can
+/// be uninitialized, no meaningful action can be performed when `T` is dropped. This means
+/// the destructor of `Opaque<T>` does *not* run the destructor of `T` as that can't work
+/// on potentially invalid `T` data.
+///
 /// # Examples
 ///
 /// ```
-- 
2.48.0

Re: [RFC PATCH] rust: types: extend `Opaque` documentation

[Miguel Ojeda]

On Wed, Jan 22, 2025 at 6:54 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>
> This is marked as RFC as it mainly takes some statements from
> the linked discussion. Let us take this as a starting point
> to discuss how we want to extend the `Opaque` documentation.

Thanks Dirk for this sort of patch -- I think it is important to lift
some comments from the mailing list and distill them into
documentation as time permits, so thanks.

The docs already list the requirements that `Opaque` lifts. What we
could do, perhaps, is expand each bullet point on that list to mention
e.g. how that is done and link to the relevant docs.

To try to explain what I mean, for instance, in the bullet point:

    * The value is allowed to be uninitialized (for example have
invalid bit patterns: `3` for a [`bool`]).

perhaps we could add some of what you wrote here, or simply mention
and link to `MaybeUninit`'s relevant docs on it.

The destruction part of the paragraph could also be there in that
bullet point as another paragraph, or could be kept at the end of the
docs as a separate note to highlight it a bit more, e.g.

    Note that the destructor of [`Opaque<T>`] does _not_ run the destructor
    of `T`, as `T` may be uninitialized, as described above.

Cheers,
Miguel

Re: [RFC PATCH] rust: types: extend `Opaque` documentation

[Dirk Behme]

On 22/01/2025 10:41, Miguel Ojeda wrote:
> On Wed, Jan 22, 2025 at 6:54 AM Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>
>> This is marked as RFC as it mainly takes some statements from
>> the linked discussion. Let us take this as a starting point
>> to discuss how we want to extend the `Opaque` documentation.
> 
> Thanks Dirk for this sort of patch -- I think it is important to lift
> some comments from the mailing list and distill them into
> documentation as time permits, so thanks.
> 
> The docs already list the requirements that `Opaque` lifts. What we
> could do, perhaps, is expand each bullet point on that list to mention
> e.g. how that is done and link to the relevant docs.
> 
> To try to explain what I mean, for instance, in the bullet point:
> 
>     * The value is allowed to be uninitialized (for example have
> invalid bit patterns: `3` for a [`bool`]).
> 
> perhaps we could add some of what you wrote here, or simply mention
> and link to `MaybeUninit`'s relevant docs on it.
> 
> The destruction part of the paragraph could also be there in that
> bullet point as another paragraph, or could be kept at the end of the
> docs as a separate note to highlight it a bit more, e.g.
> 
>     Note that the destructor of [`Opaque<T>`] does _not_ run the destructor
>     of `T`, as `T` may be uninitialized, as described above.

Thanks!

Next proposal [1] (with some `Opaque<T>` -> [`Opaque<T>`] replacements).

Cheers

Dirk

[1]

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 994e504ca9075..60c5f6b9f402d 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -226,22 +226,27 @@ fn drop(&mut self) {

 /// Stores an opaque value.
 ///
-/// `Opaque<T>` is meant to be used with FFI objects that are never
interpreted by Rust code.
+/// [`Opaque<T>`] is meant to be used with FFI objects that are never
interpreted by Rust code.
 ///
 /// It is used to wrap structs from the C side, like for example
`Opaque<bindings::mutex>`.
-/// It gets rid of all the usual assumptions that Rust has for a value:
-///
-/// * The value is allowed to be uninitialized (for example have
invalid bit patterns: `3` for a
-///   [`bool`]).
-/// * The value is allowed to be mutated, when a `&Opaque<T>` exists on
the Rust side.
+/// This is useful for C structs that are not fully initialized (yet)
or might change their
+/// content from C side at runtime. [`Opaque<T>`] gets rid of all the
usual assumptions that
+/// Rust has for a value of type `T`:
+///
+/// * `T` is allowed to be uninitialized or invalid (for example have
invalid bit patterns:
+///   `3` for a [`bool`]). By dereferencing a raw pointer to `T` you
are unsafely asserting
+///    that `T` is valid *right now*.
+/// * `T` is allowed to be mutated, when a `&Opaque<T>` exists on the
Rust side.
 /// * No uniqueness for mutable references: it is fine to have multiple
`&mut Opaque<T>` point to
 ///   the same value.
-/// * The value is not allowed to be shared with other threads (i.e. it
is `!Sync`).
+/// * `T` is not allowed to be shared with other threads (i.e. it is
`!Sync`).
+/// * The destructor of [`Opaque<T>`] does *not* run the destructor of
`T`, as `T` may
+///   be uninitialized, as described above.
 ///
 /// This has to be used for all values that the C side has access to,
because it can't be ensured
 /// that the C side is adhering to the usual constraints that Rust needs.
 ///
-/// Using `Opaque<T>` allows to continue to use references on the Rust
side even for values shared
+/// Using [`Opaque<T>`] allows to continue to use references on the
Rust side even for values shared
 /// with C.
 ///
 /// # Examples