linux-kernel - Re: [PATCH v3] rust: add bindings and API for bitmap.h and bitops.h.

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=uBYDBn2CS9OW-+S0=rCZEJFdrcBAk8yBQdNB+0Yjq=A@mail.gmail.com>
Date: Mon, 10 Mar 2025 17:53:10 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Burak Emir <bqe@...gle.com>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Viresh Kumar <viresh.kumar@...aro.org>, 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>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rust: add bindings and API for bitmap.h and bitops.h.

Hi Burak,

Some quick notes...

On Mon, Mar 10, 2025 at 5:20 PM Burak Emir <bqe@...gle.com> wrote:
>
> +void rust_helper_bitmap_copy_and_extend(unsigned long *dst, const unsigned long *src, unsigned int count, unsigned int size)
> +{
> +       bitmap_copy_and_extend(dst, src, count, size);
> +}

Please use the same parameter names as the real one, i.e. `to` and `from`.

> +/// Wraps underlying C bitmap structure.

I am not sure I would say it "structure" here, i.e. it seems like it
actually wraps a C `struct bitmap`.

In general, we also try to mention the "wraps ..." (if it actually did
so) in a second paragraph, rather than doing so in the title.

> +/// # Invariants
> +///
> +/// * `ptr` is obtained from a successful call to `bitmap_zalloc` and

Also, you may remove the bullet list -- we currently do not enforce
that it is always a bullet list (though we may in the future).

> +    /// Pointer to an array of unsigned long.

`unsigned long`

> +    ptr: NonNull<usize>,
> +    /// How many bits this bitmap stores. Must be < 2^32.

Should the "Must be" be part of the invariants above?

> +        // SAFETY: `self.ptr` was returned by bitmap_zalloc.

"the C `bitmap_zalloc`"

> +impl Bitmap {
> +    /// Constructs a new [`Bitmap`].
> +    ///
> +    /// Fails with AllocError if `nbits` is greater than or equal to 2^32,

Intra-doc links where possible: [`AllocError`].

> +    /// # Example

We use plurals even if there is a single example (like for the invariants).

> +    /// ```
> +    /// # use kernel::bitmap::Bitmap;
> +    ///
> +    /// fn new_bitmap() -> Bitmap {
> +    ///   Bitmap::new(128, GFP_KERNEL).unwrap()
> +    /// }
> +    /// ```

Is there a reason why this example cannot be run? i.e. to not have it
wrapped in a function.

Also, please use the `?` operator if possible -- we try to write
examples as "real code", so we try to avoid `unwrap()` etc.

> +            Ok(Bitmap { ptr, nbits })

When we have invariants, we use an `// INVARIANT: ...` comment (please
grep for similar cases to see how it is usually done).

> +    /// Returns how many bits this bitmap holds.

We should have examples (which double as tests) for these.

One alternative, that may be simpler to showcase how the type works,
is to write a longer example in the documentation of the type itself.

> +        // SAFETY: nbits == 0 is supported and access is within bounds.

Markdown wherever possible, e.g. `nbits == 0` (also other instance).

Thanks!

Cheers,
Miguel