| Message ID | 20241213081035.2069066-2-davidgow@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | rust: kunit: Support KUnit tests with a user-space like syntax | expand |
On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@google.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
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..5003282cb4c4 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,121 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; } + +/// Represents an individual test case. +/// +/// The test case should have the signature +/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. +/// Use `kunit_case_null` to generate such a delimeter. +const fn kunit_case( + name: &'static kernel::str::CStr, + run_case: unsafe extern "C" fn(*mut kernel::bindings::kunit), +) -> kernel::bindings::kunit_case { + kernel::bindings::kunit_case { + run_case: Some(run_case), + name: name.as_char_ptr(), + attr: kernel::bindings::kunit_attributes { + speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + generate_params: None, + status: kernel::bindings::kunit_status_KUNIT_SUCCESS, + module_name: core::ptr::null_mut(), + log: core::ptr::null_mut(), + } +} + +/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function retuns such a delimiter. +const fn kunit_case_null() -> kernel::bindings::kunit_case { + kernel::bindings::kunit_case { + run_case: None, + name: core::ptr::null_mut(), + generate_params: None, + attr: kernel::bindings::kunit_attributes { + speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + status: kernel::bindings::kunit_status_KUNIT_SUCCESS, + module_name: core::ptr::null_mut(), + log: core::ptr::null_mut(), + } +} + +/// Registers a KUnit test suite. +/// +/// # Safety +/// +/// `test_cases` must be a NULL terminated array of test cases. +/// +/// # Examples +/// +/// ```ignore +/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) { +/// let actual = 1 + 1; +/// let expected = 2; +/// assert_eq!(actual, expected); +/// } +/// +/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn); +/// static mut KUNIT_NULL_CASE: crate::bindings::kunit_case = crate::kunit_case_null(); +/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe { +/// &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE] +/// }; +/// crate::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES); +/// ``` +#[macro_export] +macro_rules! kunit_unsafe_test_suite { + ($name:ident, $test_cases:ident) => { + const _: () = { + static KUNIT_TEST_SUITE_NAME: [i8; 256] = { + let name_u8 = core::stringify!($name).as_bytes(); + let mut ret = [0; 256]; + + if name_u8.len() > 255 { + panic!(concat!( + "The test suite name `", + core::stringify!($name), + "` exceeds the maximum length of 255 bytes." + )); + } + + let mut i = 0; + while i < name_u8.len() { + ret[i] = name_u8[i] as i8; + i += 1; + } + + ret + }; + + 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 + // named 'unsafe'. + test_cases: unsafe { $test_cases.as_mut_ptr() }, + suite_init: None, + suite_exit: None, + init: None, + exit: None, + attr: $crate::bindings::kunit_attributes { + speed: $crate::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + status_comment: [0; 256usize], + debugfs: core::ptr::null_mut(), + log: core::ptr::null_mut(), + suite_init_err: 0, + is_init: false, + }; + + #[used] + #[link_section = ".kunit_test_suites"] + static mut KUNIT_TEST_SUITE_ENTRY: *const $crate::bindings::kunit_suite = + // SAFETY: `KUNIT_TEST_SUITE` is static. + unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) }; + }; + }; +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e1065a7551a3..02a70c5ad8ee 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -18,6 +18,7 @@ #![feature(inline_const)] #![feature(lint_reasons)] #![feature(unsize)] +#![feature(const_mut_refs)] // Ensure conditional compilation based on the kernel configuration works; // otherwise we may silently break things like initcall handling.