Message-ID: <CANiq72mjf51e=dNzx-P1=m7Ns=7yAV5Ky6GumtRDBeoo46mLyQ@mail.gmail.com>
Date: Fri, 3 Jan 2025 16:39:28 +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 1/3] rust: kunit: add KUnit case and suite macros

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@...gle.com> wrote:
>
> +/// The test case should have the signature
> +/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`.

I guess this is here so that people can copy-paste it (i.e. users)?
However, given it is private (i.e. users are not expected to manually
use this function) and that it is already in the signature
(`run_case`), I wonder if we need to mention it this paragraph.

Moreover, does it need to be `unsafe fn`? Safe ones coerce into unsafe
ones (i.e. not in the parameter, but in the one that the user defines)

> +/// Use `kunit_case_null` to generate such a delimeter.

Typo: delimiter

> +/// function retuns such a delimiter.

Typo: returns

> +/// Registers a KUnit test suite.
> +///
> +/// # Safety
> +///
> +/// `test_cases` must be a NULL terminated array of test cases.

I guess "test cases" here also means "valid ones", i.e. without
invalid pointers and so on inside, right? Perhaps it is good to at
least mention "valid" clearly. Otherwise, one can read it as "any
possible instance of `kunit_case`", which would be unsound, no?

We could go finer-grained, and explain exactly what needs to be valid,
which would be good.

A third alternative would be to mention that these test cases must be
created using the two functions above (`kunit_case*()`), and make the
`kunit_case()` one require a suitable `run_case` function pointer
(i.e. making it `unsafe`). That way the requirements are a bit more
localized, even if it means making that one `unsafe fn`.

> +/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) {

Does this function need to be `unsafe`? (please see above for the
comment on the docs of `kunit_case`). If so, then we would need a `#
Safety` section if the example was built (see below).

> +/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn);

Shouldn't this `name` be a `CStr` for this example? (The example is
ignored, but it would be ideal to keep it up to date).

> +/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe {
> +///     &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE]
> +/// };

These should have a space after `&mut`. However, why do we need the
mutable reference here anyway? (please see discussion below and in the
next patch)

In addition, doesn't this end up with duplicated statics? i.e. one in
the array, and an independent one. So we can instead put them directly
into the array, which would avoid `unsafe` in the initializer too.
(This applies to the generation in the next patch, too).

Finally, should we make this documentation `#[doc(hidden)]` (but
keeping the docs)? i.e. it is not expected to be used by users
directly anyway (and currently the example wouldn't work, since e.g.
`kunit_case` is private).

Speaking of the example, we could fix it and make it non-`ignore`
(assuming we made `kunit_case*` public which we will probably
eventually need, in which case they should also be `#[doc(hidden)]`),
e.g.:

    /// ```
    /// extern "C" fn test_fn(_test: *mut kernel::bindings::kunit) {
    ///     let actual = 1 + 1;
    ///     let expected = 2;
    ///     assert_eq!(actual, expected);
    /// }
    ///
    /// static mut KUNIT_TEST_CASES: [kernel::bindings::kunit_case; 2] = [
    ///     kernel::kunit::kunit_case(kernel::c_str!("name"), test_fn),
    ///     kernel::kunit::kunit_case_null(),
    /// ];
    /// kernel::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES);
    /// ```

(If so, we should then use explicit names, since otherwise the KUnit
output in the log is confusing.)

> +            static KUNIT_TEST_SUITE_NAME: [i8; 256] = {

This should probably be `::kernel::ffi::c_char` instead (and then the
cast below needs a change too).

And I think we can make this a `const` instead, since we just use it
to initialize the array in the `static mut`, no?

> +                let name_u8 = core::stringify!($name).as_bytes();

Since it is a macro, I would use absolute paths, e.g. `::core::...`.

> +            static mut KUNIT_TEST_SUITE: $crate::bindings::kunit_suite =
> +                $crate::bindings::kunit_suite {
> +                    name: KUNIT_TEST_SUITE_NAME,
> +                    // SAFETY: User is expected to pass a correct `test_cases`, hence this macro

Nit: usually we say "by the safety preconditions" or similar, instead
of "User is expected to pass...".

> +                    // named 'unsafe'.

`unsafe`. (Also, not an English speaker, but should this be "is named" instead?)

> +                    test_cases: unsafe { $test_cases.as_mut_ptr() },

This warns due to `static_mut_refs` starting Rust 1.83:

    error: creating a mutable reference to mutable static is discouraged
    --> rust/kernel/kunit.rs:261:42
        |
    261 |                     test_cases: unsafe { $test_cases.as_mut_ptr() },
        |                                          ^^^^^^^^^^^ mutable
reference to mutable static

    https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html

It can still be `allow`ed; however, doesn't the call to `as_mut_ptr()`
create an intermediate mutable reference that we should avoid anyway?

Instead, can we have an array static directly, rather than a mutable
reference static to an array, and use `addr_of_mut!` here? Then we
also avoid adding the `const_mut_refs` feature (even if it is stable
now).

That is, we would have (with all the feedback in place) something like:

    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(),
    ];

And then:

    test_cases: unsafe {
        ::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()
    },

So no mutable references in sight.

>  #![feature(unsize)]
> +#![feature(const_mut_refs)]

The list should be kept sorted.

However, we can avoid enabling the feature I think, if we avoid the
`&mut`s (please see above).

Cheers,
Miguel

Powered by blists - more mailing lists