linux-kernel - Re: [PATCH 1/3] rust: add Aliased type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72mqhTA+LQ5d6Sd-E24qc1_k46UZ6sbgGZRj07GZ9TR5Bg@mail.gmail.com>
Date: Mon, 20 Jan 2025 00:04:56 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Arnd Bergmann <arnd@...db.de>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] rust: add Aliased type

Hi Christian,

Thanks for the series! A few quick comments on the documentation...

On Sun, Jan 19, 2025 at 11:11 PM Christian Schrefl
<chrisi.schrefl@...il.com> wrote:
>
> This type is useful for cases where a value might be shared with C code
> but not interpreted by it.
> In partiquarly this is added to for data that is shared between a Driver
> and a MiscDevice implementation.

Typo: partiquarly

Also, normally you would leave an empty line between paragraphs, if
that is what you intended.

> Similar to Opaque but guarantees that the value is initialized and the
> inner value is dropped when Aliased is dropped.
>
> This was origianally proposed for the IRQ abstractions [0], but also

Typo: origianally

I usually recommend using `checkpatch.pl` with `--codespell` (it may
have caught these).

> +/// Stores a value that may be aliased.
> +///
> +/// This is similar to `Opaque<T>` but is guaranteed to contain valid data and will

Would an intra-doc link be possible? Please use then wherever reasonable.

> +/// Call the Drop implementation of T when dropped.

"Call" -> "call"?

Also, please format `Drop` and `T` and add the intra-doc link for the former.

Could we have an example or two for the type? More generally, you have
some nice comments in the commit message, so it is best to put some of
that in the actual documentation, which is what most people will read,
so that they don't need to search for the commit that introduced it.

> +    }
> +    /// Create an `Aliased` pin-initializer from the given pin-initializer.

Newline between methods/functions/etc.

> +    pub fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        // SAFETY:
> +        // In case of an error in value the error is returned, otherwise the slot is fully initialized,
> +        // since value is initialized and _pin is a Zero sized type.
> +        // The pin invariants of value are upheld, since no moving occurs.

For lists of requirements, we typically use bullets, e.g. `  - `. I
think this applies to another patch too.

Also, please format comments with Markdown as well (no need for
intra-doc links, of course).

> +    }
> +    /// Returns a raw pointer to the opaque data.

Missing newline?

Cheers,
Miguel