linux-kernel - Re: [PATCH 5/7] rust: str: take advantage of the `-> Result` support 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJ-ks9krSxo_DWFJdccP+SYJhX4WF0hXdq-20Rtf3doMp_2zDw@mail.gmail.com>
Date: Sun, 4 May 2025 14:22:26 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	David Gow <davidgow@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, Rae Moar <rmoar@...gle.com>, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.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>, 
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH 5/7] rust: str: take advantage of the `-> Result` support
 in KUnit `#[test]`'s

On Sun, May 4, 2025 at 2:15 PM Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
>
> On Sun, May 4, 2025 at 7:30 PM Tamir Duberstein <tamird@...il.com> wrote:
> >
> > Alice pointed this out in another thread but: one of the downsides of
> > returning Result is that in the event of failure the line number where
> > the error occurred is no longer contained in the test output. I'm 👎
> > on this change for that reason.
>
> We could perhaps customize `?` to help here, e.g. printing a trace or
> panic, with the `Try` trait or similar.
>
> Related to this: I thought about saying in the guidelines that `?` in
> tests is intended for things that you would normally use `?` in
> similar kernel code, i.e. things that the test is not "testing",
> rather than things that you would want to assert explicitly. Thus the
> actual code under test should still have `assert!`s in the right
> places. I did that in the sample. That way, having `?` would still
> simplify a lot of test code and yet allow us to differentiate between
> code under test vs. other code failing.

I see. Up to you, obviously, but ISTM that this degree of freedom is
unnecessary, but perhaps there's a benefit I'm underappreciating?

>
> > These changes don't depend on returning `Result` from the tests
> > AFAICT. Can they be in a separate patch?
>
> Not sure what you mean. The change below uses `?`, which is what
> allows this to be removed.

Even without this change, couldn't you apply

     macro_rules! format {
         ($($f:tt)*) => ({
-            &*String::from_fmt(kernel::fmt!($($f)*))
+            CString::try_from_fmt(kernel::fmt!($($f)*)).unwrap().to_str().unwrap()
         })
     }

and achieve roughly the same reduction in line count in the test module?

Cheers.
Tamir