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: <29fcdb16-5931-4189-91c4-57d947cdd5c1@gmail.com>
Date: Mon, 20 Jan 2025 01:27:37 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...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

On 20.01.25 12:04 AM, Miguel Ojeda wrote:
> 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

Fixed the typos and missing newlines `--codespell` did not catch them.

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

Fixed

> 
>> +/// 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.

I added some more to the documentation and also mentioned it in the 
`Opaque` docs.

> 
>> +    }
>> +    /// 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.

Changed it, also fixed the wrapping.

> 
> Also, please format comments with Markdown as well (no need for
> intra-doc links, of course).
> 
>> +    }
>> +    /// Returns a raw pointer to the opaque data.
Changed this to contained data.

> 
> Missing newline?
> 
> Cheers,
> Miguel

In case you want to take a look the current version of the patch is available at:
https://github.com/onestacked/linux/commits/b4/rust_miscdevice_registrationdata/

Cheers
Christian