linux-kernel - Re: [PATCH v10] rust: transmute: Add methods for FromBytes trait

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: <CANiq72kGh2PS-c1n2Q1g0t24J-b3ctB0kN3GK8RDXhtQMGAtTA@mail.gmail.com>
Date: Mon, 25 Aug 2025 16:51:45 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: "Christian S. Lima" <christiansantoslima21@...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>, 
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, ~lkcamp/patches@...ts.sr.ht, 
	richard120310@...il.com
Subject: Re: [PATCH v10] rust: transmute: Add methods for FromBytes trait

On Sun, Aug 24, 2025 at 11:31 PM Christian S. Lima
<christiansantoslima21@...il.com> wrote:
>
> Since the `is_aligned` method for pointer types has been stabilized in
> `1.79` version and is being used in this patch. I'm enabling the
> feature.

Period -> comma? No need to backquote version numbers.

> Even enabling in `rust/kernel/lib.rs` when compiling with `make LLVM=1
> CLIPPY=1` a warning is issued, so in order to compile, it was used
> locally the `#[allow(clippy::incompatible_msrv)]`.

I would perhaps try to reword this a bit.

> Link: https://github.com/Rust-for-Linux/linux/issues/1119
> Suggested-by: Alexandre Courbot <acourbot@...dia.com>

Please add a link to the original suggestion if possible. If it is the
link above, then the tags should be in the opposite order.

Did Benno and Alexandre both suggest it?

> +/// fn test() -> Option<()> {
> +///    let raw = [1, 2, 3, 4];
> +///
> +///    let result = u32::from_bytes(&raw)?;
> +///
> +///    #[cfg(target_endian = "little")]
> +///    assert_eq!(*result, 0x4030201);
> +///
> +///    #[cfg(target_endian = "big")]
> +///    assert_eq!(*result, 0x1020304);
> +///
> +///    Some(())
> +/// }

Should the function be called? Otherwise, we are only build-testing this.

Should we just remove the function and to it directly at the top-level?

> +    /// Converts a slice of bytes to a reference to `Self`.

An intra-doc link may work here.

> +    /// In another case, it will return `None`.

Ditto (also elsewhere).

I would perhaps just say "Otherwise," to simplify.

> +    #[allow(clippy::incompatible_msrv)]

If this can be made more local, then please do so; otherwise, no big deal.

> +            // SAFETY: Checking for size and alignment ensure
> +            // that the conversion to a type is valid

Period at the end. I would probably say "Size and alignment were just
checked, thus ...".

> +    /// Converts a mutable slice of bytes to a reference to `Self`

Period at the end, intra-doc link if possible (ditto elsewhere).

> +    /// When the reference is properly aligned and the size of slice
> +    /// is equal to that of `T`and is different from zero.

Missing space after `T`.

These are all mostly nits that I sometimes fixed on-apply, for
Alexandre in case it helps, i.e. no need to send a new version if it
is getting applied already.

The actually-run-the-test is probably the most important one.

Thanks!

Cheers,
Miguel