linux-kernel - Re: [PATCH RFC WIP] bitflag_options macro

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: <CANiq72=zMKKBTXmePj69=fq8qUSOFRNknW=teAdE9M1GxYuAVg@mail.gmail.com>
Date: Sat, 25 Jan 2025 19:54:31 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: gtimothy-dev@...tonmail.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>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC WIP] bitflag_options macro_rules implementation

Hi Timothy,

Thanks for the patch! A few quick notes below.

On Sat, Jan 25, 2025 at 7:09 PM GTimothy via B4 Relay
<devnull+gtimothy-dev.protonmail.com@...nel.org> wrote:
>
> Signed-off-by: GTimothy <gtimothy-dev@...tonmail.com>

The kernel requires a "known identity" for signatures. For instance,
the name you use to sign documents in real life. Please let us know if
that is a problem -- see:

    https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

>  - would this better be placed in rust/macro/ or rust/kernel/?

`rust/macros` is meant for proc macros. So this is fine in the
`kernel` crate. :)

> Thanks in advance for any feedback,

The docs/examples you wrote are very nice, but typically you also want
to showcase an actual use case within the kernel. You can do that, for
instance, in a separate patch that does it for hrtimer or something
else (even if it may not be intended for merge).

This can also allow reviewers and potential users to give suggestions
and to guide the design.

> +    /// The bitfield type, usually [u8] or [u32]

There are some nits with formatting which are non-important at this
stage, but please try to follow a consistent style with other files.
For instance, we try to use intra-doc links and code spans where
possible, avoid double blank lines, etc.

I also noticed a couple typos -- I usually recommend using at least
`--codespell` in `scripts/checkpatch.pl`.

> +/// The `SizeAndColor` type is now available in this scope.

For macros, it can be a good idea to showcase how the generated code
for a simple case would look like. It is painful sometimes to maintain
that over time, but it can really help others understand what the
macro does from another perspective.

This one can also help reviewers and to guide the design, similar to
the doc suggestion above.

Cheers,
Miguel