Message-Id: <DCDUIFACG791.3SCF7302W8JZ8@nvidia.com>
Date: Thu, 28 Aug 2025 15:26:47 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>, "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

Thanks for the feedback Miguel! For the record, here are the changes I am
planning on applying on top of this version:

On Mon Aug 25, 2025 at 11:51 PM JST, Miguel Ojeda wrote:
> 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.

Reworded this to:

Also add `#[allow(clippy::incompatible_msrv)]` where needed until the
MSRV is updated to `1.79`.

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

Changed this to call the function and hide the unneeded parts:

--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -16,19 +16,20 @@
 /// ```
 /// use kernel::transmute::FromBytes;
 ///
-/// fn test() -> Option<()> {
-///    let raw = [1, 2, 3, 4];
+/// # fn test() -> Option<()> {
+/// let raw = [1, 2, 3, 4];
 ///
-///    let result = u32::from_bytes(&raw)?;
+/// let result = u32::from_bytes(&raw)?;
 ///
-///    #[cfg(target_endian = "little")]
-///    assert_eq!(*result, 0x4030201);
+/// #[cfg(target_endian = "little")]
+/// assert_eq!(*result, 0x4030201);
 ///
-///    #[cfg(target_endian = "big")]
-///    assert_eq!(*result, 0x1020304);
+/// #[cfg(target_endian = "big")]
+/// assert_eq!(*result, 0x1020304);
 ///
-///    Some(())
-/// }
+/// # Some(()) }
+/// # test().ok_or(EINVAL)?;
+/// # Ok::<(), Error>(())

>
>> +    /// Converts a slice of bytes to a reference to `Self`.
>
> An intra-doc link may work here.

Wasn't sure what a link to `Self` might link to in a trait declaration, so left
this one as-is. :)

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

Moved this closer to where it is needed:

--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -41,13 +41,14 @@ pub unsafe trait FromBytes {
     /// and is different from zero.
     ///
     /// In another case, it will return `None`.
-    #[allow(clippy::incompatible_msrv)]
     fn from_bytes(bytes: &[u8]) -> Option<&Self>
     where
         Self: Sized,
     {
         let slice_ptr = bytes.as_ptr().cast::<Self>();
         let size = size_of::<Self>();
+
+        #[allow(clippy::incompatible_msrv)]
         if bytes.len() == size && slice_ptr.is_aligned() {
             // SAFETY: Checking for size and alignment ensure
             // that the conversion to a type is valid
@@ -63,13 +64,14 @@ fn from_bytes(bytes: &[u8]) -> Option<&Self>
     /// is equal to that of `T`and is different from zero.
     ///
     /// In another case, it will return `None`.
-    #[allow(clippy::incompatible_msrv)]
     fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
     where
         Self: AsBytes + Sized,
     {
         let slice_ptr = bytes.as_mut_ptr().cast::<Self>();
         let size = size_of::<Self>();
+
+        #[allow(clippy::incompatible_msrv)]
         if bytes.len() == size && slice_ptr.is_aligned() {
             // SAFETY: Checking for size and alignment ensure


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

Changed the doc/comments as follows:

@@ -38,10 +38,10 @@
 pub unsafe trait FromBytes {
     /// Converts a slice of bytes to a reference to `Self`.
     ///
-    /// When the reference is properly aligned and the size of slice is equal to that of `T`
-    /// and is different from zero.
+    /// Succeeds if the reference is properly aligned, and the size of `bytes` is equal to that of
+    /// `T` and different from zero.
     ///
-    /// In another case, it will return `None`.
+    /// Otherwise, returns [`None`].
     fn from_bytes(bytes: &[u8]) -> Option<&Self>
     where
         Self: Sized,
@@ -51,20 +51,19 @@ fn from_bytes(bytes: &[u8]) -> Option<&Self>

         #[allow(clippy::incompatible_msrv)]
         if bytes.len() == size && slice_ptr.is_aligned() {
-            // SAFETY: Checking for size and alignment ensure
-            // that the conversion to a type is valid
+            // SAFETY: Size and alignment were just checked.
             unsafe { Some(&*slice_ptr) }
         } else {
             None
         }
     }

-    /// Converts a mutable slice of bytes to a reference to `Self`
+    /// Converts a mutable slice of bytes to a reference to `Self`.
     ///
-    /// When the reference is properly aligned and the size of slice
-    /// is equal to that of `T`and is different from zero.
+    /// Succeeds if the reference is properly aligned, and the size of `bytes` is equal to that of
+    /// `T` and different from zero.
     ///
-    /// In another case, it will return `None`.
+    /// Otherwise, returns [`None`].
     fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
     where
         Self: AsBytes + Sized,
@@ -74,8 +73,7 @@ fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>

         #[allow(clippy::incompatible_msrv)]
         if bytes.len() == size && slice_ptr.is_aligned() {
-            // SAFETY: Checking for size and alignment ensure
-            // that the conversion to a type is valid
+            // SAFETY: Size and alignment were just checked.
             unsafe { Some(&mut *slice_ptr) }
         } else {
             None

Also, as suggested by Alice I am re-enabling the implementation on slices:

--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -94,6 +94,7 @@ macro_rules! impl_frombytes {

     // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
     // patterns are also acceptable for arrays of that type.
+    {<T: FromBytes>} [T],
     {<T: FromBytes, const N: usize>} [T; N],
 }

Unless someone says something by then, I plan on pushing this to nova-next
tomorrow.

Powered by blists - more mailing lists