linux-kernel - Re: [PATCH 2/7] rust: kunit: support checked `-> Result`s in KUnit `#[test]`s

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: <CABVgOSnqtxwYQBbed_-TYr6D3BZJ=MT3KVv-eXUFbnRm1UyETw@mail.gmail.com>
Date: Tue, 6 May 2025 14:32:43 +0800
From: David Gow <davidgow@...gle.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	Alex Gaynor <alex.gaynor@...il.com>, Rae Moar <rmoar@...gle.com>, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.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>, 
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH 2/7] rust: kunit: support checked `-> Result`s in KUnit `#[test]`s

On Tue, 6 May 2025 at 03:34, Boqun Feng <boqun.feng@...il.com> wrote:
>
> On Mon, May 05, 2025 at 02:02:09PM +0800, David Gow wrote:
> > On Sat, 3 May 2025 at 05:51, Miguel Ojeda <ojeda@...nel.org> wrote:
> > >
> > > Currently, return values of KUnit `#[test]` functions are ignored.
> > >
> > > Thus introduce support for `-> Result` functions by checking their
> > > returned values.
> > >
> > > At the same time, require that test functions return `()` or `Result<T,
> > > E>`, which should avoid mistakes, especially with non-`#[must_use]`
> > > types. Other types can be supported in the future if needed.
> > >
> > > With this, a failing test like:
> > >
> > >     #[test]
> > >     fn my_test() -> Result {
> > >         f()?;
> > >         Ok(())
> > >     }
> > >
> > > will output:
> > >
> > >     [    3.744214]     KTAP version 1
> > >     [    3.744287]     # Subtest: my_test_suite
> > >     [    3.744378]     # speed: normal
> > >     [    3.744399]     1..1
> > >     [    3.745817]     # my_test: ASSERTION FAILED at rust/kernel/lib.rs:321
> > >     [    3.745817]     Expected is_test_result_ok(my_test()) to be true, but is false
> > >     [    3.747152]     # my_test.speed: normal
> > >     [    3.747199]     not ok 1 my_test
> > >     [    3.747345] not ok 4 my_test_suite
> > >
> > > Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
> > > ---
> >
> > This is a neat idea. I think a lot of tests will still want to go with
> > the () return value, but having both as an option seems like a good
> > idea to me.
> >
>
> Handling the return value of a test is definitely a good thing, but I'm
> not sure returning `Err` should be treated as assertion failure
> though. Considering:
>
>     #[test]
>     fn foo() -> Result {
>         let b = KBox::new(...)?; // need to allocate memory for the test
>         use_box(b);
>         assert!(...);
>     }
>
> if the test runs without enough memory, then KBox::new() would return a
> Err(ENOMEM), and technically, it's not a test failure rather the test
> has been skipped because of no enough resource.
>
> I would suggest we handle the `Err` returns specially (mark as "SKIPPED"
> maybe?).
>
> Thoughts?
>
> Regards,
> Boqun
>

FWIW, having out-of-memory situations trigger a test failure is
consistent with what other KUnit tests (written in C) do.

There's both advantages and disadvantages to this: on the one hand,
it's prone to false positives (as you mention), on the other, it
catches cases where the test is using an unusually large amount of
memory (which could indeed be a test issues).

My go-to rule is that tests should fail if small allocations (which,
in the normal course of events, _should_ succeed) fail, but if they
have unusual resource requirements (beyond "enough memory for the
system to run normally") these should be checked separately when the
test starts.

That being said, I definitely think that, by default, an `Err` return
should map to a FAILED test result, as not all Err Results are a
resource exhaustion issue, and we definitely don't want to mark a test
as skipped if there's a real error occurring. If test authors wish to
skip a test when an out-of-memory condition occurs, they probably
should handle that explicitly. (But I'd not be opposed to helpers to
make it easier.)

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)