linux-kernel - Re: [RFC PATCH 2/2] rust: add maple tree abstractions

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72=KqZ6pOj62ofmv0yRfqy3HjZkDHanyvFbDvb+ECcjgTQ@mail.gmail.com>
Date: Sat, 5 Apr 2025 17:23:39 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Andrew Ballance <andrewjballance@...il.com>
Cc: Liam.Howlett@...cle.com, ojeda@...nel.org, alex.gaynor@...il.com, 
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	benno.lossin@...ton.me, a.hindborg@...nel.org, aliceryhl@...gle.com, 
	tmgross@...ch.edu, dakr@...nel.org, akpm@...ux-foundation.org, 
	gregkh@...uxfoundation.org, wedsonaf@...il.com, brauner@...nel.org, 
	dingxiangfei2009@...il.com, linux-kernel@...r.kernel.org, 
	maple-tree@...ts.infradead.org, linux-mm@...ck.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] rust: add maple tree abstractions

Hi Andrew,

Some doc-related notes for future submissions... I hope this helps.

On Sat, Apr 5, 2025 at 8:03 AM Andrew Ballance
<andrewjballance@...il.com> wrote:
>
> +#include <linux/maple_tree.h>
> +
> +

Nit: double newline.

> +//! # Examples
> +//! ```

There should be an empty doc line between these.

> +// TODO and missing features
> +// - the C version suports external locking this only supports the internal spinlock

(Multiple instances) Typo -- you may want to use e.g. `checkpatch.pl`
with `--codespell`.

> +// SAFETY:
> +// we cannot derefrence any pointers without holding the spinlock because
> +// all returned pointers are guaranteed to have been inserted by the user
> +// but the pointers are not guaranteed to be still be valid
> +// another thread may have already removed and dropped the pointers
> +// so to safely deref the returned pointers the user must
> +// have exclusive write access to the `MapleTree`
> +// or rcu_read_lock() is held and updater side uses a synchronize_rcu()

`SAFETY` comments are intended to go attached to an `unsafe` block or
implementation, explaining why they satisfy the requirements.

Clippy should be warning here, but it doesn't likely due to this
Clippy issue I just filed:

    https://github.com/rust-lang/rust-clippy/issues/14554

(plus other three I filed while looking into this one).

> +/// A `MapleTree` is a tree like data structure that is optimized for storing

(Multiple instances) Please use intra-doc links wherever they work.

> +// # Invariants

(Multiple instances) Please make this section part of the docs. We may
change how we document private invariants in the future (or we may
expose private fields so that it is clearer) -- for the moment, please
add a note if you really want users to never rely on a particular
invariant.

> +    /// creates a new `MapleTree` with the specified `flags`

(Multiple instances) Please follow the usual style, e.g. Markdown and
intra-doc links where possible, start with uppercase, period at the
end of sentences, etc.

> +                // SAFETY:
> +                // - mt is valid because of ffi_init
> +                // - maple_tree contains a lock which should be pinned

(Multiple instances) Please format with Markdown in comments.

> +    /// helper for internal use.
> +    /// returns an iterator through the maple tree
> +    /// # Safety

(Multiple instances) Newlines are needed here -- `rustdoc` uses the
first paragraph as the "short description", too.

> +#[derive(Clone, Copy, PartialEq)]
> +/// flags to be used with [`MapleTree::new`].

We write attributes below the documentation.

> +        if ptr.is_null() {
> +            return None;
> +        }
> +        // SAFETY:

We typically leave a newline after a loop or a branch.

Thanks for the patch!

Cheers,
Miguel