linux-kernel - Re: [PATCH v5 2/3] rust: macros: add macro to easily run KUnit tests

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=Mn=N5LJ-9P6YSbOYGZDXeAGnEGcMY8GjDdmPKS=841A@mail.gmail.com>
Date: Fri, 3 Jan 2025 16:39:40 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: David Gow <davidgow@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, José Expósito <jose.exposito89@...il.com>, 
	Rae Moar <rmoar@...gle.com>, Boqun Feng <boqun.feng@...il.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Benno Lossin <benno.lossin@...ton.me>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Matt Gilbride <mattgilbride@...gle.com>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, kunit-dev@...glegroups.com, 
	linux-kselftest@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] rust: macros: add macro to easily run KUnit tests

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@...gle.com> wrote:
>
> +            #[allow(unused_unsafe)]

Should this be in the first patch?

> -                unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
> +                unsafe {
> +                    core::ptr::addr_of_mut!(KUNIT_TEST_SUITE)
> +                };

Spurious change?

I thought this should perhaps have been in the previous patch
directly, but running `rustfmt` shows the previous patch is the
correct formatting.

`rustfmt` also needs to be run for these files -- it reveals a few
more changes needed.

> +//! Copyright (c) 2023 José Expósito <jose.exposito89@...il.com>

(This is not something we need to change now, since it is orthogonal
and debatable, but I think copyright lines should not be in the
generated documentation unless we really want contact points in docs.
I think it could go in a `//` comment after the `SPDX` line instead.)

> +pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
> +    if attr.to_string().is_empty() {
> +        panic!("Missing test name in #[kunit_tests(test_name)] macro")
> +    }
> +
> +    if attr.to_string().as_bytes().len() > 255 {
> +        panic!("The test suite name `{}` exceeds the maximum length of 255 bytes.", attr)
> +    }

We could do the `to_string()` step once creating a single string (and
we can keep it as a `String`, too):

    let attr = attr.to_string();

> +    // Scan for the "mod" keyword.

Formatting: `mod`

> +        .expect("#[kunit_tests(test_name)] attribute should only be applied to modules");

Formatting: `#[kunit_tests(test_name)]`

> +        _ => panic!("cannot locate main body of module"),

Formatting: Cannot

> +    //     #[test]
> +    //     fn bar() {
> +    //         assert_eq!(2, 2);
> +    //     }
> +    // ```

Missing closing `}` of the `mod`.

> +    // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
> +    //     kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);

I think this is not in sync with the generation, i.e. missing
kernel::c_str!("foo")

(and ditto for the other one)

> +    // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {

Formatting: extra space and missing space.

> +    //     &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
> +    // };
> +    //
> +    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
> +    // ```

With the suggestions from the previous patch (i.e. avoiding
unused/duplicated `static`s, intermediate mutable references, unstable
feature and `unsafe`), we can simplify the entire example down to:

    // ```
    // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut
kernel::bindings::kunit) { foo(); }
    // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut
kernel::bindings::kunit) { bar(); }
    //
    // static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [
    //     kernel::kunit::kunit_case(kernel::c_str!("foo"),
kunit_rust_wrapper_foo),
    //     kernel::kunit::kunit_case(kernel::c_str!("bar"),
kunit_rust_wrapper_bar),
    //     kernel::kunit::kunit_case_null()
    // ];
    //
    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
    // ```

...with the corresponding removals in the actual generation code
below, of course.

Cheers,
Miguel