linux-kernel - Re: [PATCH v2 2/5] rust: maple

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: <CANiq72nJiJ4K6jy17x-YRYnJpjqTnohYWvoFrLkYQp0X4tLL=w@mail.gmail.com>
Date: Fri, 22 Aug 2025 23:22:24 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Alice Ryhl <aliceryhl@...gle.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	"Liam R. Howlett" <Liam.Howlett@...cle.com>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	Miguel Ojeda <ojeda@...nel.org>, Andrew Ballance <andrewjballance@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, linux-kernel@...r.kernel.org, 
	maple-tree@...ts.infradead.org, rust-for-linux@...r.kernel.org, 
	linux-mm@...ck.org
Subject: Re: [PATCH v2 2/5] rust: maple_tree: add MapleTree

On Fri, Aug 22, 2025 at 1:44 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> Why not? I mean, the above is cleaner and more idiomatic with or without the
> lint enabled. As mentioned, it's even the showcase that has been picked for the
> documentation of Result::is_err_and().

Is it idiomatic, though? The example you mention comes from the method
itself, which is of course the obvious use case for the method, but it
doesn't imply other ways are now not idiomatic anymore or that people
have stopped using `assert_eq!` or `unwrap_err()`.

It has just been 2 years in Rust, after all. And, from a quick grep in
the Rust repo, it doesn't seem they have migrated what they had to the
new one in that time.

Also, do we expect to use that method in normal code, and not just
within asserts? We haven't used it yet

As for being clearer, what we want to assert is that the cause is a
given one, so `assert_eq!` seems like a natural choice (and it isn't a
case like `is_none()` where there is an advantage). Plus it means it
is able to show both sides if it fails. So it is not a clear win-win
in my eyes.

> But especially people new to the kernel starting to write Rust drivers may not
> even be aware of this fact. If they see some unwrap_err() calls in examples they
> might not even thing about it a lot before starting to use them, e.g. because
> they're used to it from userspace projects anyways.

We still have the issue that they will see the `assert!` anyway and
thus can easily think panics are OK. I understand what you are saying:
you want to minimize those cases anyway.

> I think we should do this; I really think otherwise we gonna see a lot of them
> once we get more drivers. It's just too convinient. :)

A potential middle ground is to apply the lint in normal code, but not
in examples.

Or, even better, we can actually only allow it within `assert!`s,
since it is a custom macro I wrote for KUnit and not the real one,
i.e. enforcing what I suggested above [1].

Another one is a lint that just affects `unwrap()` and not
`unwrap_err()` -- I think the Clippy one doesn't allow it now, but we
could request it. It could be combined with the above so that only
`unwrap_err()` is allowed within `assert!`s.

We could also go the C way, warning in `checkpatch.pl` about it like
it does `BUG_ON`.

What I like about the Clippy approach is that it can be locally `expect`ed.

Cheers,
Miguel

[1]

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 41efd87595d6..86ea37319f7b 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -160,6 +160,7 @@ unsafe impl Sync for UnaryAssert {}
 #[macro_export]
 macro_rules! kunit_assert_eq {
     ($name:literal, $file:literal, $diff:expr, $left:expr,
$right:expr $(,)?) => {{
+        #![allow(clippy::unwrap_used)]
         // For the moment, we just forward to the expression assert
because, for binary asserts,
         // KUnit supports only a few types (e.g. integers).
         $crate::kunit_assert!($name, $file, $diff, $left == $right);