Message-ID: <CANiq72mhgL-eidEBhxkzMKFztAjRjAFEdeO5Oe6Uv1mVMtEdoA@mail.gmail.com>
Date: Sat, 9 Nov 2024 22:33:54 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Christian dos Santos de Lima <christiansantoslima21@...il.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, 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>, ~lkcamp/patches@...ts.sr.ht
Subject: Re: [PATCH v3] rust: transmute: Add implementation for FromBytes trait

On Sat, Nov 9, 2024 at 6:54 AM Christian dos Santos de Lima
<christiansantoslima21@...il.com> wrote:
>
> Add implementation and documentation for FromBytes trait.

Thanks for the patch! Some comments below...

The title does not need to be duplicated in the message.

More importantly, the commit message should explain what you are
changing and why.

In particular, "Add implementation and documentation for FromBytes
trait." seems wrong, because we already have the implementation and
the documentation for the `FromBytes` trait.

> Add new feature block in order to allow using ToBytes
> and bound to from_bytes_mut function. I'm adding this feature
> because is possible create a value with disallowed bit pattern
> and as_byte_mut could create such value by mutating the array and
> accessing the original value. So adding ToBytes this can be avoided.

I understand that you want to add those two features, but you need to
justify more precisely why they are needed, e.g. is there a way to do
it without them? If yes, why cannot use that alternative way? e.g. is
it too complicated? Or is it that it cannot be achieved without the
feature at all? Please try to be explicit and mention both features by
name.

Also, you should mention their status if you know about it, since we
should avoid adding new features, given that we are trying to get
Linux into stable Rust:

    https://rust-for-linux.com/unstable-features#usage-in-the-kernel

>  //! Traits for transmuting types.
>
> +use core::simd::ToBytes;
>  /// Types for which any bit pattern is valid.

Please try to be consistent with the rest of the code in its style,
e.g. newline here, whitespace elsewhere, safety comments, use
Markdown, etc. (but we can discuss that in a later version).

> +pub unsafe trait FromBytes {
> +    /// Get an imutable slice of bytes and converts to a reference to Self

Typo (`scripts/checkpatch.pl` has spell checking capabilities).

More importantly, this `unsafe fn` does not have a `# Safety` section.

> +    /// # Safety
> +    ///
> +    /// Bound ToBytes in order to avoid use with disallowed bit patterns

This `# Safety` section does not explain the safety preconditions; and
the bound is anyway in the signature already, i.e. this section is not
about explaining how you implemented something, but for users to learn
how to use it properly.

> +// Get a reference of slice of bytes and converts into a reference of integer or a slice with a defined size

This seems misplaced?

> +/// Get a reference of slice of bytes and converts into a reference of an array of integers
> +///
> +/// Types for which any bit pattern is valid.
> +///
> +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
> +/// reading arbitrary bytes into something that contains a `bool` is not okay.
> +///
> +/// It's okay for the type to have padding, as initializing those bytes has no effect.
> +///

This also seems misplaced, and apparently is a mixture of the
`FromBytes` docs (?).

> +unsafe impl<T: FromBytes> FromBytes for [T] {
> +    unsafe fn from_bytes(slice_of_bytes: &[u8]) -> &Self {
> +        // Safety: Guarantee that all values are initialized

Please see how other safety comments are written: we need to justify
the preconditions of the unsafe operations within the block. So, for
instance, indeed, the values need to be initialized to call
`from_raw_parts_mut`, but why they are so?

Some operations here do not need to be in the block -- we try to minimize them.

>  pub unsafe trait AsBytes {}
> -
>  macro_rules! impl_asbytes {
>      ($($({$($generics:tt)*})? $t:ty, )*) => {
>          // SAFETY: Safety comments written in the macro invocation.
> @@ -63,7 +141,6 @@ macro_rules! impl_asbytes {
>      bool,
>      char,
>      str,
> -
>      // SAFETY: If individual values in an array have no uninitialized portions, then the array

Please avoid spurious/unrelated changes -- these should not be in the patch.

Thanks!

Cheers,
Miguel

Powered by blists - more mailing lists