linux-kernel - Re: [PATCH RFC WIP v2] rust: add bitfield_options macro to create bitfield wrapper types

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: <79283554-ce12-4812-be1d-af8b420c1053@sedlak.dev>
Date: Tue, 28 Jan 2025 20:29:49 +0100
From: Daniel Sedlak <daniel@...lak.dev>
To: gtimothy-dev@...tonmail.com, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>
Cc: 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>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC WIP v2] rust: add bitfield_options macro to create
 bitfield wrapper types

Hello,
I do not have a strong opinion on whether this patch is actually needed 
thus I only do a quick review of the code/documentation part.

What I think is actually missing is an another patch integrating this or 
showing usage of this somewhere, for example in samples/drivers.

On 1/28/25 4:48 PM, Timothy Garwood via B4 Relay wrote:

> +//! Trait and macro to build bitfield wrappers.
> +//!
> +//! A trait and [`kernel::bitfield_options`] macro to build bitfield wrappers with
> +//! [`std::fs::OpenOptions`](https://doc.rust-lang.org/std/fs/struct.OpenOptions.html)-like methods to
> +//! set bits and _try to ensure an always-valid state_.
> +
> +/// A trait associating a bitfield type to a wrapper type to allow for shared trait implementations
> +/// and bitfield retrieval.
> +pub trait BitField {
> +    /// The bitfield type, usually `u8` or `u32`

Missing period at the end.

> +    type Bits;
> +
> +    /// returns the bits stored by the wrapper.
> +    fn bits(&self) -> Self::Bits;
> +}
> +
> +/// concatenate sequences using a comma separator. Uses a a `, and` (oxford comma) separation

Capitalization of the first word.

> +/// instead between the last two items.
> +#[doc(hidden)]
> +#[macro_export]
> +macro_rules! concat_with_oxford_comma {
> +    ($last:expr) => (concat!("and ", $last));
> +    ($left:expr, $($right:expr),+ ) => (
> +        concat!($left, ", ", $crate::concat_with_oxford_comma!($($right),+))
> +    )
> +}
> +
> +/// Builds a bitfield wrapper with
> +/// [`std::fs::OpenOptions`](https://doc.rust-lang.org/std/fs/struct.OpenOptions.html)-like methods to

This might be opinionated, but it may be better to use positional 
parameters [1] as it is not that noisy to read in the source code.

Link: 
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html#valid-links 
[1]

> +/// set bits.
> +///
> +/// This wrapper _tries to ensure an always-valid state_, even when created from a bitfield
> +/// directly.
> +///
> +/// The wrapper implements the [`BitField`] trait.
> +///
> +/// # argument types
> +///   - `name`: the name of the wrapper to build.
> +///   - `type`: the bit field type, usually `u32` or `u8`. Must be an int primitive.
> +///   - `options`: array of `{ name:ident, true:expr, false:expr }` options where name is the name
> +///     of a method that will set the `true:expr` bit(s) when called with `true` or else the
> +///     `false:expr` bit(s). In a standard bitfield, `false:expr` is set to `false:0`, but sometimes
> +///     different bit flags designate incompatible alternatives : in these cases, set `false` to
> +///     activate the alternative bit(s).
> +///
> +/// # Examples
> +///
> +/// Call the macro to create the bitflag wrapper.
> +///
> +/// Here we create a wrapper that allows to set `RED`, `BLUE`, `GREEN` bits independently,
> +/// and additionally either the `BIG` bit or the `SMALL` bit.
> +///
> +/// `BIG` and `SMALL` are incompatible alternatives.
> +///
> +/// ```rust

The rust is implicit.

> +/// use kernel::bitfield_options;
> +///
> +/// const BIG: u32 = 16;
> +/// const SMALL: u32 = 1;
> +///
> +/// const RED: u32 = 2;
> +/// const GREEN: u32 = 4;
> +/// const BLUE: u32 = 8;
> +///
> +/// bitfield_options! {
> +///     name: SizeAndColors,
> +///     type: u32,
> +///     options: [
> +///         {name:big, true:BIG, false:SMALL},
> +///         {name:red, true:RED, false:0u32},
> +///         {name:blue, true:BLUE, false:0u32},
> +///         {name:green, true:GREEN, false:0u32}
> +///     ]
> +/// };
> +/// ```
> +///
> +/// The `SizeAndColor` type is now available in this scope.
> +///
> +/// It implements the [`Default`] trait where all options are set to `false`:
> +///
> +/// ```rust

The rust is implicit.

> +/// # use kernel::bitfield_options;
> +/// # use kernel::bitfield::BitField; // to have access to the bits() method
> +/// #
> +/// # const BIG: u32 = 16;
> +/// # const SMALL: u32 = 1;
> +/// #
> +/// # const RED: u32 = 2;
> +/// # const GREEN: u32 = 4;
> +/// # const BLUE: u32 = 8;
> +/// #
> +/// # bitfield_options! {
> +/// #     name: SizeAndColors,
> +/// #     type: u32,
> +/// #     options: [
> +/// #         {name:big, true:BIG, false:SMALL},
> +/// #         {name:red, true:RED, false:0u32},
> +/// #         {name:blue, true:BLUE, false:0u32},
> +/// #         {name:green, true:GREEN, false:0u32}
> +/// #     ]
> +/// # };
> +/// let sac = SizeAndColors::default();
> +///
> +/// // the default state, with all the options set to false should be the field with only the
> +/// // `SMALL` bit(s) set
> +/// assert_eq!(
> +///     sac.bits(),
> +///     SMALL | 0u32 | 0u32 | 0u32)
> +/// ```
> +///
> +/// It implements the [`TryFrom`] trait which succeeds only if the passed bitfield is a valid state:
> +///
> +/// ```rust

The rust is implicit.

> +/// # use kernel::bitfield_options;
> +/// #
> +/// # const BIG: u32 = 16;
> +/// # const SMALL: u32 = 1;
> +/// #
> +/// # const RED: u32 = 2;
> +/// # const GREEN: u32 = 4;
> +/// # const BLUE: u32 = 8;
> +/// #
> +/// # bitfield_options! {
> +/// #     name: SizeAndColors,
> +/// #     type: u32,
> +/// #     options: [
> +/// #         {name:big, true:BIG, false:SMALL},
> +/// #         {name:red, true:RED, false:0u32},
> +/// #         {name:blue, true:BLUE, false:0u32},
> +/// #         {name:green, true:GREEN, false:0u32}
> +/// #     ]
> +/// # };
> +/// # let sac = SizeAndColors::default();
> +/// let small_and_no_colors = SizeAndColors::try_from(SMALL)
> +///    .expect("having only the SMALL bit to 1 should be a valid state");
> +///
> +/// // Having only the `SMALL` bit set to 1 is the default state.
> +/// assert_eq!(sac, small_and_no_colors);
> +///
> +/// let big_and_rgb = SizeAndColors::try_from(BIG | RED | GREEN | BLUE)
> +///     .expect("having the BIG, RED, GREEN and BLUE bits set to 1 should be a valid state");
> +///
> +/// assert_eq!(
> +///     SizeAndColors::default()
> +///         .big(true)
> +///         .red(true)
> +///         .blue(true)
> +///         .green(true),
> +///     big_and_rgb
> +/// );
> +///
> +/// let big_and_small = SizeAndColors::try_from(BIG | SMALL)
> +///     .expect_err("BIG and SMALL are incompatible, this should fail");
> +///
> +/// let intense:u32 = 1024;
> +/// let big_and_intense = SizeAndColors::try_from(BIG | intense)
> +///     .expect_err("BIG|intense is not a valid state, this should fail");
> +/// ```

Daniel