Hi all,
After much delay, v6 of the KUnit/Rust integration patchset is here. This change incorporates most of Miguels suggestions from v5 (save for some of the copyright headers I wasn't comfortable unilaterally changing). This means the documentation is much improved, and it should work more cleanly on Rust 1.83 and 1.84, no longer requiring static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all of the details of this, but I'm comfortable enough with how it's ended up.)
This has been rebased against 6.14-rc1/rust-next, and should be able to comfortably go in via either the KUnit or Rust trees. My suspicion is that there's more likely to be conflicts with the Rust work (due to the changes in rust/macros/lib.rs) than with KUnit, where there are no current patches which would break the API, so maybe it makes the most sense for it to go in via Rust for 6.15.
This series was originally written by José Expósito, and has been modified and updated by Matt Gilbride, Miguel Ojeda, and myself. The original version can be found here: https://github.com/Rust-for-Linux/linux/pull/950
Add support for writing KUnit tests in Rust. While Rust doctests are already converted to KUnit tests and run, they're really better suited for examples, rather than as first-class unit tests.
This series implements a series of direct Rust bindings for KUnit tests, as well as a new macro which allows KUnit tests to be written using a close variant of normal Rust unit test syntax. The only change required is replacing '#[cfg(test)]' with '#[kunit_tests(kunit_test_suite_name)]'
An example test would look like: #[kunit_tests(rust_kernel_hid_driver)] mod tests { use super::*; use crate::{c_str, driver, hid, prelude::*}; use core::ptr;
struct SimpleTestDriver; impl Driver for SimpleTestDriver { type Data = (); }
#[test] fn rust_test_hid_driver_adapter() { let mut hid = bindings::hid_driver::default(); let name = c_str!("SimpleTestDriver"); static MODULE: ThisModule = unsafe { ThisModule::from_ptr(ptr::null_mut()) };
let res = unsafe { <hid::Adapter<SimpleTestDriver> as driver::DriverOps>::register(&mut hid, name, &MODULE) }; assert_eq!(res, Err(ENODEV)); // The mock returns -19 } }
Please give this a go, and make sure I haven't broken it! There's almost certainly a lot of improvements which can be made -- and there's a fair case to be made for replacing some of this with generated C code which can use the C macros -- but this is hopefully an adequate implementation for now, and the interface can (with luck) remain the same even if the implementation changes.
A few small notable missing features: - Attributes (like the speed of a test) are hardcoded to the default value. - Similarly, the module name attribute is hardcoded to NULL. In C, we use the KBUILD_MODNAME macro, but I couldn't find a way to use this from Rust which wasn't more ugly than just disabling it. - Assertions are not automatically rewritten to use KUnit assertions.
---
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-1-davidgow@google.com/ - Rebased against 6.14-rc1 - Fixed a bunch of warnings / clippy lints introduced in Rust 1.83 and 1.84. - No longer needs static_mut_refs / const_mut_refs, and is much cleaned up as a result. (Thanks, Miguel) - Major documentation and example fixes. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-1-davidgow@go... - Rebased against 6.13-rc1 - Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel) - "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning due to a redundant assertion. (Thanks Boqun, Miguel) - Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun) - Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-2-davidgow@go... - The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!) - The #[kunit_tests()] macro now preserves span information, so errors can be better reported. (Thanks, Boqun!) - The example tests have been updated to no longer use assert_eq!() with a constant bool argument (which triggered a clippy warning now we have the span info).
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-1-davidgow@go... - Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!) - The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!) - The proc macro now emits an error if the suite name is too long. - We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-0-c80db349e3b5@google.com/... - Rebase on top of the latest rust-next (commit 718c4069896c) - Make kunit_case a const fn, rather than a macro (Thanks Boqun) - As a result, the null terminator is now created with kernel::kunit::kunit_case_null() - Use the C kunit_get_current_test() function to implement in_kunit_test(), rather than re-implementing it (less efficiently) ourselves.
Changes since the GitHub PR: - Rebased on top of kselftest/kunit - Add const_mut_refs feature This may conflict with https://lore.kernel.org/lkml/20230503090708.2524310-6-nmi@metaspace.dk/ - Add rust/macros/kunit.rs to the KUnit MAINTAINERS entry
---
José Expósito (3): rust: kunit: add KUnit case and suite macros rust: macros: add macro to easily run KUnit tests rust: kunit: allow to know if we are in a test
MAINTAINERS | 1 + rust/kernel/kunit.rs | 199 +++++++++++++++++++++++++++++++++++++++++++ rust/macros/kunit.rs | 161 ++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 29 +++++++ 4 files changed, 390 insertions(+) create mode 100644 rust/macros/kunit.rs
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below). - The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function. - The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com ---
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/ - Rebased against 6.14-rc1 - Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel) - Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel) - While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway. - Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go... - Rebased against 6.13-rc1 - Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel) - Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go... - The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go... - The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!) - We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/ - Rebase on top of rust-next - As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name. - Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
--- rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; } + +/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter. +#[doc(hidden)] +pub 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 returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite { + ($name:ident, $test_cases:ident) => { + const _: () = { + const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 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 ::kernel::ffi::c_char; + i += 1; + } + + ret + }; + + #[allow(unused_unsafe)] + static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite = + ::kernel::bindings::kunit_suite { + name: KUNIT_TEST_SUITE_NAME, + // SAFETY: User is expected to pass a correct `test_cases`, given the safety + // precondition; hence this macro named `unsafe`. + test_cases: unsafe { + ::core::ptr::addr_of_mut!($test_cases) + .cast::<::kernel::bindings::kunit_case>() + }, + suite_init: None, + suite_exit: None, + init: None, + exit: None, + attr: ::kernel::bindings::kunit_attributes { + speed: ::kernel::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 ::kernel::bindings::kunit_suite = + // SAFETY: `KUNIT_TEST_SUITE` is static. + unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) }; + }; + }; +}
Very excited to see this progress.
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below).
- The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function.
- The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel)
- While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go...
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go...
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go...
- The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; }
+/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter.
Can both of these be linkified? [`kunit_unsafe_test_suite!`] and [`kunit_case_null`]. There are more instances below.
+#[doc(hidden)] +pub 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(),
These members, after `name`, can be spelled `..kunit_case_null()` to avoid some repetition.
- }
+}
+/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite {
- ($name:ident, $test_cases:ident) => {
const _: () = {const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {let name_u8 = ::core::stringify!($name).as_bytes();
This can be a little simpler:
let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
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 ::kernel::ffi::c_char;i += 1;}
I'd suggest `ret[..name.len()].copy_from_slice(name)` but `copy_from_slice` isn't `const`. This can stay the same with the now-unnecessary cast removed, or it can be the body of `copy_from_slice`:
// SAFETY: `name` is valid for `name.len()` elements by definition, and `ret` was // checked to be at least as large as `name`. The buffers are statically know to not // overlap. unsafe { ::core::ptr::copy_nonoverlapping(name.as_ptr(), ret.as_mut_ptr(), name.len());
}
ret};#[allow(unused_unsafe)]static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =::kernel::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, given the safety// precondition; hence this macro named `unsafe`.test_cases: unsafe {::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()},
This safety comment seems to be referring to the safety of `addr_of_mut!` but this was just a compiler limitation until Rust 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
Can we also narrow the `#[allow]`? This seems to work:
#[allow(unused_unsafe)] // SAFETY: ... test_cases: unsafe { ::core::ptr::addr_of_mut!($test_cases) .cast::<::kernel::bindings::kunit_case>() },
suite_init: None,suite_exit: None,init: None,exit: None,attr: ::kernel::bindings::kunit_attributes {speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,},status_comment: [0; 256usize],
I don't think you need the usize hint here, there's always a usize in the length position.
debugfs: ::core::ptr::null_mut(),log: ::core::ptr::null_mut(),suite_init_err: 0,is_init: false,};#[used]#[link_section = ".kunit_test_suites"]
This attribute causes the same problem I describe in https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail..... That's because the KUnit tests are generated both on target and on host (even though they won't run on host). I don't think you need to deal with that here, just pointing it out. I think we'll need a cfg to indicate we're building for host to avoid emitting these attributes that only make sense in the kernel.
static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =// SAFETY: `KUNIT_TEST_SUITE` is static.unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
This is missing the `#[allow(unused_unsafe)]` (it appears in the next patch). This means this commit will not compile on bisection.
};- };
+}
2.48.1.601.g30ceb7b040-goog
Global note: it seems more customary to use crate:: and $crate:: instead of kernel:: and ::kernel in functions and macros, respectively.
Cheers.
Tamir
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
Very excited to see this progress.
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below).
- The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function.
- The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel)
- While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go...
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go...
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go...
- The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; }
+/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter.
Can both of these be linkified? [`kunit_unsafe_test_suite!`] and [`kunit_case_null`]. There are more instances below.
I've done this here. I've not linkified absolutely everything, but the most obvious/prominent ones should be done.
+#[doc(hidden)] +pub 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(),These members, after `name`, can be spelled `..kunit_case_null()` to avoid some repetition.
I'm going to leave this as-is, as logically, the NULL terminator case and the 'default' case are two different things (even if, in practice, they have the same values). And personally, I find it clearer to have them more explicitly set here for now.
It is a nice thought, though, so I reserve the right to change my mind in the future. :-)
- }
+}
+/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite {
- ($name:ident, $test_cases:ident) => {
const _: () = {const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {let name_u8 = ::core::stringify!($name).as_bytes();This can be a little simpler:
let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
I'm not sure this ends up being much simpler: it makes it (possible?, obvious?) to get rid of the cast below, but we don't actually need to copy the null byte at the end, so it seems wasteful to bother.
So after playing around both ways, I think this is probably best.
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 ::kernel::ffi::c_char;i += 1;}I'd suggest `ret[..name.len()].copy_from_slice(name)` but `copy_from_slice` isn't `const`. This can stay the same with the now-unnecessary cast removed, or it can be the body of `copy_from_slice`:
// SAFETY: `name` is valid for `name.len()` elementsby definition, and `ret` was // checked to be at least as large as `name`. The buffers are statically know to not // overlap. unsafe { ::core::ptr::copy_nonoverlapping(name.as_ptr(), ret.as_mut_ptr(), name.len());
}
I think I'll keep this as the loop for now, as that's more obvious to me, and avoids the extra unsafe block.
If copy_from_slice ends up working in a const context, we can consider that later (though, personally, I still find the loop easier to understand).
ret};#[allow(unused_unsafe)]static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =::kernel::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, given the safety// precondition; hence this macro named `unsafe`.test_cases: unsafe {::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()},This safety comment seems to be referring to the safety of `addr_of_mut!` but this was just a compiler limitation until Rust 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
Yeah, this comment was originally written prior to 1.82 existing.
But I think we probably should keep the safety comment here anyway, as -- if I understand it -- 1.82 only makes this safe if $test_cases is static, so it's still worth documenting the preconditions here.
If we eventually stop supporting rust < 1.82, though, I'd be happy to remove the unsafe and thereby enforce the use of this macro only on statics at compile time.
Can we also narrow the `#[allow]`? This seems to work:
#[allow(unused_unsafe)] // SAFETY: ... test_cases: unsafe { ::core::ptr::addr_of_mut!($test_cases) .cast::<::kernel::bindings::kunit_case>() },
We hit problems before with #[allow] not working on expressions, so assumed we couldn't narrow it further. Seems to be working here, though, so I've changed it.
suite_init: None,suite_exit: None,init: None,exit: None,attr: ::kernel::bindings::kunit_attributes {speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,},status_comment: [0; 256usize],I don't think you need the usize hint here, there's always a usize in the length position.
debugfs: ::core::ptr::null_mut(),log: ::core::ptr::null_mut(),suite_init_err: 0,is_init: false,};#[used]#[link_section = ".kunit_test_suites"]This attribute causes the same problem I describe in https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail..... That's because the KUnit tests are generated both on target and on host (even though they won't run on host). I don't think you need to deal with that here, just pointing it out. I think we'll need a cfg to indicate we're building for host to avoid emitting these attributes that only make sense in the kernel.
I've added the same exclusion for macos here, but don't have a macos machine to test it on. If it's broken, we can fix it in a follow-up.
static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =// SAFETY: `KUNIT_TEST_SUITE` is static.unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };This is missing the `#[allow(unused_unsafe)]` (it appears in the next patch). This means this commit will not compile on bisection.
Whoops. This must have snuck in during one of the many squashes and rebases. I'll fix that now.
Annoyingly, I'm no longer able to actually reproduce the warning, which is strange.
};- };
+}
2.48.1.601.g30ceb7b040-goog
Global note: it seems more customary to use crate:: and $crate:: instead of kernel:: and ::kernel in functions and macros, respectively.
Hmm... I think I had that at one point and was told to change it to kernel. Personally, I think kernel:: makes more sense. So if it's not breaking anything, and I'll only get yelled at a little bit, let's keep it as is.
In any case, I'm sending out v7 with the changes above. My hope is that this is then probably "good enough" to let us get started, and future changes can be done as independent patches, so we're both not holding this up for too long, and can better parallelise any fixes, rather than having to re-spin the whole series each time.
Thanks a lot, -- David
On Sat, Feb 15, 2025 at 4:03 AM David Gow davidgow@google.com wrote:
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
Very excited to see this progress.
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below).
- The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function.
- The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel)
- While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go...
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go...
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go...
- The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; }
+/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter.
Can both of these be linkified? [`kunit_unsafe_test_suite!`] and [`kunit_case_null`]. There are more instances below.
I've done this here. I've not linkified absolutely everything, but the most obvious/prominent ones should be done.
+#[doc(hidden)] +pub 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(),These members, after `name`, can be spelled `..kunit_case_null()` to avoid some repetition.
I'm going to leave this as-is, as logically, the NULL terminator case and the 'default' case are two different things (even if, in practice, they have the same values). And personally, I find it clearer to have them more explicitly set here for now.
It is a nice thought, though, so I reserve the right to change my mind in the future. :-)
- }
+}
+/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite {
- ($name:ident, $test_cases:ident) => {
const _: () = {const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {let name_u8 = ::core::stringify!($name).as_bytes();This can be a little simpler:
let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
I'm not sure this ends up being much simpler: it makes it (possible?, obvious?) to get rid of the cast below, but we don't actually need to copy the null byte at the end, so it seems wasteful to bother.
So after playing around both ways, I think this is probably best.
If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.
The main thing is that `as` casts in Rust are a rare instance of unchecked coercion in the language, so I encourage folks to avoid them. The rest I don't feel strongly about.
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 ::kernel::ffi::c_char;i += 1;}I'd suggest `ret[..name.len()].copy_from_slice(name)` but `copy_from_slice` isn't `const`. This can stay the same with the now-unnecessary cast removed, or it can be the body of `copy_from_slice`:
// SAFETY: `name` is valid for `name.len()` elementsby definition, and `ret` was // checked to be at least as large as `name`. The buffers are statically know to not // overlap. unsafe { ::core::ptr::copy_nonoverlapping(name.as_ptr(), ret.as_mut_ptr(), name.len());
}I think I'll keep this as the loop for now, as that's more obvious to me, and avoids the extra unsafe block.
If copy_from_slice ends up working in a const context, we can consider that later (though, personally, I still find the loop easier to understand).
ret};#[allow(unused_unsafe)]static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =::kernel::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, given the safety// precondition; hence this macro named `unsafe`.test_cases: unsafe {::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()},This safety comment seems to be referring to the safety of `addr_of_mut!` but this was just a compiler limitation until Rust 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
Yeah, this comment was originally written prior to 1.82 existing.
But I think we probably should keep the safety comment here anyway, as -- if I understand it -- 1.82 only makes this safe if $test_cases is static, so it's still worth documenting the preconditions here.
I don't think the safety guarantees changed. In the Rust 1.82 release notes:
In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.
https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-uns...
That's why I flagged this; the SAFETY comment is not actually correct.
If we eventually stop supporting rust < 1.82, though, I'd be happy to remove the unsafe and thereby enforce the use of this macro only on statics at compile time.
Can we also narrow the `#[allow]`? This seems to work:
#[allow(unused_unsafe)] // SAFETY: ... test_cases: unsafe { ::core::ptr::addr_of_mut!($test_cases) .cast::<::kernel::bindings::kunit_case>() },We hit problems before with #[allow] not working on expressions, so assumed we couldn't narrow it further. Seems to be working here, though, so I've changed it.
suite_init: None,suite_exit: None,init: None,exit: None,attr: ::kernel::bindings::kunit_attributes {speed: ::kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL,},status_comment: [0; 256usize],I don't think you need the usize hint here, there's always a usize in the length position.
debugfs: ::core::ptr::null_mut(),log: ::core::ptr::null_mut(),suite_init_err: 0,is_init: false,};#[used]#[link_section = ".kunit_test_suites"]This attribute causes the same problem I describe in https://lore.kernel.org/all/20250210-macros-section-v2-1-3bb9ff44b969@gmail..... That's because the KUnit tests are generated both on target and on host (even though they won't run on host). I don't think you need to deal with that here, just pointing it out. I think we'll need a cfg to indicate we're building for host to avoid emitting these attributes that only make sense in the kernel.
I've added the same exclusion for macos here, but don't have a macos machine to test it on. If it's broken, we can fix it in a follow-up.
static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite =// SAFETY: `KUNIT_TEST_SUITE` is static.unsafe { ::core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };This is missing the `#[allow(unused_unsafe)]` (it appears in the next patch). This means this commit will not compile on bisection.
Whoops. This must have snuck in during one of the many squashes and rebases. I'll fix that now.
Annoyingly, I'm no longer able to actually reproduce the warning, which is strange.
};- };
+}
2.48.1.601.g30ceb7b040-goog
Global note: it seems more customary to use crate:: and $crate:: instead of kernel:: and ::kernel in functions and macros, respectively.
Hmm... I think I had that at one point and was told to change it to kernel. Personally, I think kernel:: makes more sense. So if it's not breaking anything, and I'll only get yelled at a little bit, let's keep it as is.
In any case, I'm sending out v7 with the changes above. My hope is that this is then probably "good enough" to let us get started, and future changes can be done as independent patches, so we're both not holding this up for too long, and can better parallelise any fixes, rather than having to re-spin the whole series each time.
Thanks a lot, -- David
On Sat, 15 Feb 2025 at 21:01, Tamir Duberstein tamird@gmail.com wrote:
On Sat, Feb 15, 2025 at 4:03 AM David Gow davidgow@google.com wrote:
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
Very excited to see this progress.
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below).
- The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function.
- The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel)
- While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go...
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go...
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go...
- The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; }
+/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter.
Can both of these be linkified? [`kunit_unsafe_test_suite!`] and [`kunit_case_null`]. There are more instances below.
I've done this here. I've not linkified absolutely everything, but the most obvious/prominent ones should be done.
+#[doc(hidden)] +pub 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(),These members, after `name`, can be spelled `..kunit_case_null()` to avoid some repetition.
I'm going to leave this as-is, as logically, the NULL terminator case and the 'default' case are two different things (even if, in practice, they have the same values). And personally, I find it clearer to have them more explicitly set here for now.
It is a nice thought, though, so I reserve the right to change my mind in the future. :-)
- }
+}
+/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite {
- ($name:ident, $test_cases:ident) => {
const _: () = {const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {let name_u8 = ::core::stringify!($name).as_bytes();This can be a little simpler:
let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
I'm not sure this ends up being much simpler: it makes it (possible?, obvious?) to get rid of the cast below, but we don't actually need to copy the null byte at the end, so it seems wasteful to bother.
So after playing around both ways, I think this is probably best.
If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.
The main thing is that `as` casts in Rust are a rare instance of unchecked coercion in the language, so I encourage folks to avoid them. The rest I don't feel strongly about.
As far as I can tell, as_bytes() returns a u8 slice anyway, so shouldn't we need the cast anyway? Or is there something I'm missing?
(Also, is there a difference between this and the Rust stdlib's to_bytes()? Or is the name difference just a glitch?)
Either way, I don't personally feel too strongly about it: the 'as' cast doesn't worry me particularly (particularly out-on-the open like this, rather than hidden behind lots of macros/indirection), but I'm happy to bow to stronger opinions for now.
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 ::kernel::ffi::c_char;i += 1;}I'd suggest `ret[..name.len()].copy_from_slice(name)` but `copy_from_slice` isn't `const`. This can stay the same with the now-unnecessary cast removed, or it can be the body of `copy_from_slice`:
// SAFETY: `name` is valid for `name.len()` elementsby definition, and `ret` was // checked to be at least as large as `name`. The buffers are statically know to not // overlap. unsafe { ::core::ptr::copy_nonoverlapping(name.as_ptr(), ret.as_mut_ptr(), name.len());
}I think I'll keep this as the loop for now, as that's more obvious to me, and avoids the extra unsafe block.
If copy_from_slice ends up working in a const context, we can consider that later (though, personally, I still find the loop easier to understand).
ret};#[allow(unused_unsafe)]static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =::kernel::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, given the safety// precondition; hence this macro named `unsafe`.test_cases: unsafe {::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()},This safety comment seems to be referring to the safety of `addr_of_mut!` but this was just a compiler limitation until Rust 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
Yeah, this comment was originally written prior to 1.82 existing.
But I think we probably should keep the safety comment here anyway, as -- if I understand it -- 1.82 only makes this safe if $test_cases is static, so it's still worth documenting the preconditions here.
I don't think the safety guarantees changed. In the Rust 1.82 release notes:
In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.
https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-uns...
That's why I flagged this; the SAFETY comment is not actually correct.
I won't pretend to be an expert on the exact semantics of Rust references / place expressions / etc here -- I still don't totally understand why this needs the cast for a start -- but I do still think there's more to the story than "this is just a compiler limitation".
The reason is that, whilst -- as you suggest -- this is always safe when $test_cases is static, that's not actually guaranteed anywhere. And with the unsafe block, it's up to the _user_ of kunit_unsafe_test_suite() to guarantee that $test_cases is safe here.
Now, I don't think the current documentation is particularly clear about this, so I'm definitely open to it changing, though I think we'd need to change the overall documentation for the macro, not just the safety comment. And maybe this will be something which, presumably, we can have enforced by the compiler in the future, should we be able to depend on rustc>1.82 and remove the 'unsafe' entirely. (But support for older compilers is important to me, so I don't want to push that point too much.)
(Also, does anyone else find the whole 'lets change the unsafe rules in a minor compiler version, and require a weird attribute to avoid a warning' thing incredibly frustrating? I've read that it's not what the formal purpose of Rust editions is, but it _feels_ like this sort of change should be in an edition intuitively to me.)
Anyway, I've got a few higher-priority non-Rust things to do by the end of the month, so I'm unlikely to have time to spin up a v8 myself for a few weeks. So if folks want to either send out an updated version or the series, or accept it as-is and send out a follow-up fix, I'm happy to review it, but otherwise it'll have to wait a little bit.
Cheers, -- David
On Tue, Feb 18, 2025 at 3:51 AM David Gow davidgow@google.com wrote:
On Sat, 15 Feb 2025 at 21:01, Tamir Duberstein tamird@gmail.com wrote:
On Sat, Feb 15, 2025 at 4:03 AM David Gow davidgow@google.com wrote:
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
Very excited to see this progress.
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below).
- The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function.
- The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-2-davidgow@google.com/
- Rebased against 6.14-rc1
- Several documentation touch-ups, including noting that the example test function need not be unsafe. (Thanks, Miguel)
- Remove the need for static_mut_refs, by using core::addr_of_mut!() combined with a cast. (Thanks, Miguel)
- While this should also avoid the need for const_mut_refs, it seems to have been enabled for other users anyway.
- Use ::kernel::ffi::c_char for C strings, rather than i8 directly. (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go...
- Rebased against 6.13-rc1
- Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel)
- Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go...
- The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go...
- The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!)
- We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/
- Rebase on top of rust-next
- As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name.
- Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
rust/kernel/kunit.rs | 121 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..d34a92075174 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,124 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; }
+/// Represents an individual test case. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of valid test cases. +/// Use `kunit_case_null` to generate such a delimiter.
Can both of these be linkified? [`kunit_unsafe_test_suite!`] and [`kunit_case_null`]. There are more instances below.
I've done this here. I've not linkified absolutely everything, but the most obvious/prominent ones should be done.
+#[doc(hidden)] +pub 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(),These members, after `name`, can be spelled `..kunit_case_null()` to avoid some repetition.
I'm going to leave this as-is, as logically, the NULL terminator case and the 'default' case are two different things (even if, in practice, they have the same values). And personally, I find it clearer to have them more explicitly set here for now.
It is a nice thought, though, so I reserve the right to change my mind in the future. :-)
- }
+}
+/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function returns such a delimiter. +#[doc(hidden)] +pub 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 valid test cases. +/// +/// # Examples +/// +/// ```ignore +/// 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); +/// ``` +#[doc(hidden)] +#[macro_export] +macro_rules! kunit_unsafe_test_suite {
- ($name:ident, $test_cases:ident) => {
const _: () = {const KUNIT_TEST_SUITE_NAME: [::kernel::ffi::c_char; 256] = {let name_u8 = ::core::stringify!($name).as_bytes();This can be a little simpler:
let name = $crate::c_str!(::core::stringify!($name)).as_bytes_with_nul();
I'm not sure this ends up being much simpler: it makes it (possible?, obvious?) to get rid of the cast below, but we don't actually need to copy the null byte at the end, so it seems wasteful to bother.
So after playing around both ways, I think this is probably best.
If you don't want to copy the null byte, you can s/as_bytes_with_nul/as_bytes.
The main thing is that `as` casts in Rust are a rare instance of unchecked coercion in the language, so I encourage folks to avoid them. The rest I don't feel strongly about.
As far as I can tell, as_bytes() returns a u8 slice anyway, so shouldn't we need the cast anyway? Or is there something I'm missing?
Ah, we don't need the cast at all, I think. `kernel::ffi::c_char` is u8.
(Also, is there a difference between this and the Rust stdlib's to_bytes()? Or is the name difference just a glitch?)
It's just an inconsistency. I believe our CStr predates some necessary features in the standard library. There is https://lore.kernel.org/all/20250203-cstr-core-v8-0-cb3f26e78686@gmail.com/t... to replace our custom implementation with upstream's.
Either way, I don't personally feel too strongly about it: the 'as' cast doesn't worry me particularly (particularly out-on-the open like this, rather than hidden behind lots of macros/indirection), but I'm happy to bow to stronger opinions for now.
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 ::kernel::ffi::c_char;i += 1;}I'd suggest `ret[..name.len()].copy_from_slice(name)` but `copy_from_slice` isn't `const`. This can stay the same with the now-unnecessary cast removed, or it can be the body of `copy_from_slice`:
// SAFETY: `name` is valid for `name.len()` elementsby definition, and `ret` was // checked to be at least as large as `name`. The buffers are statically know to not // overlap. unsafe { ::core::ptr::copy_nonoverlapping(name.as_ptr(), ret.as_mut_ptr(), name.len());
}I think I'll keep this as the loop for now, as that's more obvious to me, and avoids the extra unsafe block.
If copy_from_slice ends up working in a const context, we can consider that later (though, personally, I still find the loop easier to understand).
ret};#[allow(unused_unsafe)]static mut KUNIT_TEST_SUITE: ::kernel::bindings::kunit_suite =::kernel::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, given the safety// precondition; hence this macro named `unsafe`.test_cases: unsafe {::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>()},This safety comment seems to be referring to the safety of `addr_of_mut!` but this was just a compiler limitation until Rust 1.82, right? Same thing below on `KUNIT_TEST_SUITE_ENTRY`.
Yeah, this comment was originally written prior to 1.82 existing.
But I think we probably should keep the safety comment here anyway, as -- if I understand it -- 1.82 only makes this safe if $test_cases is static, so it's still worth documenting the preconditions here.
I don't think the safety guarantees changed. In the Rust 1.82 release notes:
In an expression context, STATIC_MUT and EXTERN_STATIC are place expressions. Previously, the compiler's safety checks were not aware that the raw ref operator did not actually affect the operand's place, treating it as a possible read or write to a pointer. No unsafety is actually present, however, as it just creates a pointer.
https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-uns...
That's why I flagged this; the SAFETY comment is not actually correct.
I won't pretend to be an expert on the exact semantics of Rust references / place expressions / etc here -- I still don't totally understand why this needs the cast for a start -- but I do still think there's more to the story than "this is just a compiler limitation".
The reason is that, whilst -- as you suggest -- this is always safe when $test_cases is static, that's not actually guaranteed anywhere. And with the unsafe block, it's up to the _user_ of kunit_unsafe_test_suite() to guarantee that $test_cases is safe here.
It's the other way around. In Rust, it is unsafe to take mutable references to statics. What the compiler didn't understand until 1.82 is that taking a raw pointer to the static did not require forming a reference, and thus wasn't unsafe (later dereferencing that pointer to create a reference would be unsafe, static or not).
Now, I don't think the current documentation is particularly clear about this, so I'm definitely open to it changing, though I think we'd need to change the overall documentation for the macro, not just the safety comment. And maybe this will be something which, presumably, we can have enforced by the compiler in the future, should we be able to depend on rustc>1.82 and remove the 'unsafe' entirely. (But support for older compilers is important to me, so I don't want to push that point too much.)
I think there's a bit of confusion going on here because the same word is being used to describe the Rust notion of memory safety as well as the preconditions expected by KUnit. That's why this comment is so misleading; the compiler says "please tell me why it's safe to form a mutable reference to this static here" and the answer comes "it's safe because the user pinky promised to end the array with a null test case". It doesn't make sense.
(Also, does anyone else find the whole 'lets change the unsafe rules in a minor compiler version, and require a weird attribute to avoid a warning' thing incredibly frustrating? I've read that it's not what the formal purpose of Rust editions is, but it _feels_ like this sort of change should be in an edition intuitively to me.)
Anyway, I've got a few higher-priority non-Rust things to do by the end of the month, so I'm unlikely to have time to spin up a v8 myself for a few weeks. So if folks want to either send out an updated version or the series, or accept it as-is and send out a follow-up fix, I'm happy to review it, but otherwise it'll have to wait a little bit.
Cheers, -- David
From: José Expósito jose.exposito89@gmail.com
Add a new procedural macro (`#[kunit_tests(kunit_test_suit_name)]`) to run KUnit tests using a user-space like syntax.
The macro, that should be used on modules, transforms every `#[test]` in a `kunit_case!` and adds a `kunit_unsafe_test_suite!` registering all of them.
The only difference with user-space tests is that instead of using `#[cfg(test)]`, `#[kunit_tests(kunit_test_suit_name)]` is used.
Note that `#[cfg(CONFIG_KUNIT)]` is added so the test module is not compiled when `CONFIG_KUNIT` is set to `n`.
Reviewed-by: David Gow davidgow@google.com Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Boqun Feng boqun.feng@gmail.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: David Gow davidgow@google.com --- Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-3-davidgow@google.com/ - Fix some rustfmt-related formatting shenanigans. (Thanks, Miguel) - Fix some documentation comment formatting as well. (Thanks, Miguel) - Tidy up the generated code to avoid unneeded &mut[] everywhere. (Thanks, Miguel) - Fix a new clippy warning for using .as_bytes().len() instead of .len() directly.
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-3-davidgow@go... - Rebased against 6.13-rc1 - "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning due to a redundant assertion. (Thanks Boqun, Miguel)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-6-davidgow@go... - The #[kunit_tests()] macro now preserves span information, so errors can be better reported. (Thanks, Boqun!) - The example test has been replaced to no longer use assert_eq!() with a constant bool argument (which triggered a clippy warning now we have the span info). It now checks 1 + 1 == 2, to match the C example. - (The in_kunit_test() test in the next patch uses assert!() to check a bool, so having something different here seemed to make a better example.)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-3-davidgow@go... - Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!) - The proc macro now emits an error if the suite name is too long.
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-2-c80db349e3b5@google.com/ - Rebased on top of rust-next - Make use of the new const functions, rather than the kunit_case!() macro.
--- MAINTAINERS | 1 + rust/kernel/kunit.rs | 12 ++++ rust/macros/kunit.rs | 161 +++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 29 ++++++++ 4 files changed, 203 insertions(+) create mode 100644 rust/macros/kunit.rs
diff --git a/MAINTAINERS b/MAINTAINERS index c8d9e8187eb0..4e7a6d2f2c49 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12677,6 +12677,7 @@ F: Documentation/dev-tools/kunit/ F: include/kunit/ F: lib/kunit/ F: rust/kernel/kunit.rs +F: rust/macros/kunit.rs F: scripts/rustdoc_test_* F: tools/testing/kunit/
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index d34a92075174..9e27b74a605b 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -40,6 +40,8 @@ pub fn info(args: fmt::Arguments<'_>) { } }
+use macros::kunit_tests; + /// Asserts that a boolean expression is `true` at runtime. /// /// Public but hidden since it should only be used from generated tests. @@ -275,6 +277,7 @@ macro_rules! kunit_unsafe_test_suite { };
#[used] + #[allow(unused_unsafe)] #[link_section = ".kunit_test_suites"] static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite = // SAFETY: `KUNIT_TEST_SUITE` is static. @@ -282,3 +285,12 @@ macro_rules! kunit_unsafe_test_suite { }; }; } + +#[kunit_tests(rust_kernel_kunit)] +mod tests { + #[test] + fn rust_test_kunit_example_test() { + #![expect(clippy::eq_op)] + assert_eq!(1 + 1, 2); + } +} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs new file mode 100644 index 000000000000..4f553ecf40c0 --- /dev/null +++ b/rust/macros/kunit.rs @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Procedural macro to run KUnit tests using a user-space like syntax. +//! +//! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com + +use proc_macro::{Delimiter, Group, TokenStream, TokenTree}; +use std::fmt::Write; + +pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { + let attr = attr.to_string(); + + if attr.is_empty() { + panic!("Missing test name in `#[kunit_tests(test_name)]` macro") + } + + if attr.len() > 255 { + panic!( + "The test suite name `{}` exceeds the maximum length of 255 bytes", + attr + ) + } + + let mut tokens: Vec<_> = ts.into_iter().collect(); + + // Scan for the `mod` keyword. + tokens + .iter() + .find_map(|token| match token { + TokenTree::Ident(ident) => match ident.to_string().as_str() { + "mod" => Some(true), + _ => None, + }, + _ => None, + }) + .expect("`#[kunit_tests(test_name)]` attribute should only be applied to modules"); + + // Retrieve the main body. The main body should be the last token tree. + let body = match tokens.pop() { + Some(TokenTree::Group(group)) if group.delimiter() == Delimiter::Brace => group, + _ => panic!("Cannot locate main body of module"), + }; + + // Get the functions set as tests. Search for `[test]` -> `fn`. + let mut body_it = body.stream().into_iter(); + let mut tests = Vec::new(); + while let Some(token) = body_it.next() { + match token { + TokenTree::Group(ident) if ident.to_string() == "[test]" => match body_it.next() { + Some(TokenTree::Ident(ident)) if ident.to_string() == "fn" => { + let test_name = match body_it.next() { + Some(TokenTree::Ident(ident)) => ident.to_string(), + _ => continue, + }; + tests.push(test_name); + } + _ => continue, + }, + _ => (), + } + } + + // Add `#[cfg(CONFIG_KUNIT)]` before the module declaration. + let config_kunit = "#[cfg(CONFIG_KUNIT)]".to_owned().parse().unwrap(); + tokens.insert( + 0, + TokenTree::Group(Group::new(Delimiter::None, config_kunit)), + ); + + // Generate the test KUnit test suite and a test case for each `#[test]`. + // The code generated for the following test module: + // + // ``` + // #[kunit_tests(kunit_test_suit_name)] + // mod tests { + // #[test] + // fn foo() { + // assert_eq!(1, 1); + // } + // + // #[test] + // fn bar() { + // assert_eq!(2, 2); + // } + // } + // ``` + // + // Looks like: + // + // ``` + // 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); + // ``` + let mut kunit_macros = "".to_owned(); + let mut test_cases = "".to_owned(); + for test in &tests { + let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test); + let kunit_wrapper = format!( + "unsafe extern "C" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}", + kunit_wrapper_fn_name, test + ); + writeln!(kunit_macros, "{kunit_wrapper}").unwrap(); + writeln!( + test_cases, + " kernel::kunit::kunit_case(kernel::c_str!("{}"), {}),", + test, kunit_wrapper_fn_name + ) + .unwrap(); + } + + writeln!(kunit_macros).unwrap(); + writeln!( + kunit_macros, + "static mut TEST_CASES: [kernel::bindings::kunit_case; {}] = [\n{test_cases} kernel::kunit::kunit_case_null(),\n];", + tests.len() + 1 + ) + .unwrap(); + + writeln!( + kunit_macros, + "kernel::kunit_unsafe_test_suite!({attr}, TEST_CASES);" + ) + .unwrap(); + + // Remove the `#[test]` macros. + // We do this at a token level, in order to preserve span information. + let mut new_body = vec![]; + let mut body_it = body.stream().into_iter(); + + while let Some(token) = body_it.next() { + match token { + TokenTree::Punct(ref c) if c.as_char() == '#' => match body_it.next() { + Some(TokenTree::Group(group)) if group.to_string() == "[test]" => (), + Some(next) => { + new_body.extend([token, next]); + } + _ => { + new_body.push(token); + } + }, + _ => { + new_body.push(token); + } + } + } + + let mut new_body = TokenStream::from_iter(new_body); + new_body.extend::<TokenStream>(kunit_macros.parse().unwrap()); + + tokens.push(TokenTree::Group(Group::new(Delimiter::Brace, new_body))); + + tokens.into_iter().collect() +} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d61bc6a56425..50b58259c577 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod helpers; +mod kunit; mod module; mod paste; mod pin_data; @@ -492,3 +493,31 @@ pub fn paste(input: TokenStream) -> TokenStream { pub fn derive_zeroable(input: TokenStream) -> TokenStream { zeroable::derive(input) } + +/// Registers a KUnit test suite and its test cases using a user-space like syntax. +/// +/// This macro should be used on modules. If `CONFIG_KUNIT` (in `.config`) is `n`, the target module +/// is ignored. +/// +/// # Examples +/// +/// ```ignore +/// # use macros::kunit_tests; +/// +/// #[kunit_tests(kunit_test_suit_name)] +/// mod tests { +/// #[test] +/// fn foo() { +/// assert_eq!(1, 1); +/// } +/// +/// #[test] +/// fn bar() { +/// assert_eq!(2, 2); +/// } +/// } +/// ``` +#[proc_macro_attribute] +pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { + kunit::kunit_tests(attr, ts) +}
On Fri, Feb 14, 2025 at 2:41 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
Add a new procedural macro (`#[kunit_tests(kunit_test_suit_name)]`) to run KUnit tests using a user-space like syntax.
The macro, that should be used on modules, transforms every `#[test]` in a `kunit_case!` and adds a `kunit_unsafe_test_suite!` registering all of them.
The only difference with user-space tests is that instead of using `#[cfg(test)]`, `#[kunit_tests(kunit_test_suit_name)]` is used.
Note that `#[cfg(CONFIG_KUNIT)]` is added so the test module is not compiled when `CONFIG_KUNIT` is set to `n`.
Reviewed-by: David Gow davidgow@google.com Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Boqun Feng boqun.feng@gmail.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: David Gow davidgow@google.com
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-3-davidgow@google.com/
- Fix some rustfmt-related formatting shenanigans. (Thanks, Miguel)
- Fix some documentation comment formatting as well. (Thanks, Miguel)
- Tidy up the generated code to avoid unneeded &mut[] everywhere. (Thanks, Miguel)
- Fix a new clippy warning for using .as_bytes().len() instead of .len() directly.
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-3-davidgow@go...
- Rebased against 6.13-rc1
- "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning due to a redundant assertion. (Thanks Boqun, Miguel)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-6-davidgow@go...
- The #[kunit_tests()] macro now preserves span information, so errors can be better reported. (Thanks, Boqun!)
- The example test has been replaced to no longer use assert_eq!() with a constant bool argument (which triggered a clippy warning now we have the span info). It now checks 1 + 1 == 2, to match the C example.
- (The in_kunit_test() test in the next patch uses assert!() to check a bool, so having something different here seemed to make a better example.)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-3-davidgow@go...
- Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!)
- The proc macro now emits an error if the suite name is too long.
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-2-c80db349e3b5@google.com/
- Rebased on top of rust-next
- Make use of the new const functions, rather than the kunit_case!() macro.
MAINTAINERS | 1 + rust/kernel/kunit.rs | 12 ++++ rust/macros/kunit.rs | 161 +++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 29 ++++++++ 4 files changed, 203 insertions(+) create mode 100644 rust/macros/kunit.rs
diff --git a/MAINTAINERS b/MAINTAINERS index c8d9e8187eb0..4e7a6d2f2c49 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12677,6 +12677,7 @@ F: Documentation/dev-tools/kunit/ F: include/kunit/ F: lib/kunit/ F: rust/kernel/kunit.rs +F: rust/macros/kunit.rs F: scripts/rustdoc_test_* F: tools/testing/kunit/
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index d34a92075174..9e27b74a605b 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -40,6 +40,8 @@ pub fn info(args: fmt::Arguments<'_>) { } }
+use macros::kunit_tests;
/// Asserts that a boolean expression is `true` at runtime. /// /// Public but hidden since it should only be used from generated tests. @@ -275,6 +277,7 @@ macro_rules! kunit_unsafe_test_suite { };
#[used]
#[allow(unused_unsafe)] #[link_section = ".kunit_test_suites"] static mut KUNIT_TEST_SUITE_ENTRY: *const ::kernel::bindings::kunit_suite = // SAFETY: `KUNIT_TEST_SUITE` is static.@@ -282,3 +285,12 @@ macro_rules! kunit_unsafe_test_suite { }; }; }
+#[kunit_tests(rust_kernel_kunit)] +mod tests {
- #[test]
- fn rust_test_kunit_example_test() {
#![expect(clippy::eq_op)]assert_eq!(1 + 1, 2);- }
+} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs new file mode 100644 index 000000000000..4f553ecf40c0 --- /dev/null +++ b/rust/macros/kunit.rs @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0
+//! Procedural macro to run KUnit tests using a user-space like syntax. +//! +//! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com
+use proc_macro::{Delimiter, Group, TokenStream, TokenTree}; +use std::fmt::Write;
+pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
- let attr = attr.to_string();
- if attr.is_empty() {
panic!("Missing test name in `#[kunit_tests(test_name)]` macro")- }
- if attr.len() > 255 {
panic!("The test suite name `{}` exceeds the maximum length of 255 bytes",attr)- }
- let mut tokens: Vec<_> = ts.into_iter().collect();
- // Scan for the `mod` keyword.
- tokens
.iter().find_map(|token| match token {TokenTree::Ident(ident) => match ident.to_string().as_str() {"mod" => Some(true),_ => None,},_ => None,}).expect("`#[kunit_tests(test_name)]` attribute should only be applied to modules");- // Retrieve the main body. The main body should be the last token tree.
- let body = match tokens.pop() {
Some(TokenTree::Group(group)) if group.delimiter() == Delimiter::Brace => group,_ => panic!("Cannot locate main body of module"),- };
- // Get the functions set as tests. Search for `[test]` -> `fn`.
- let mut body_it = body.stream().into_iter();
- let mut tests = Vec::new();
- while let Some(token) = body_it.next() {
match token {TokenTree::Group(ident) if ident.to_string() == "[test]" => match body_it.next() {Some(TokenTree::Ident(ident)) if ident.to_string() == "fn" => {let test_name = match body_it.next() {Some(TokenTree::Ident(ident)) => ident.to_string(),_ => continue,};tests.push(test_name);}_ => continue,},_ => (),}- }
- // Add `#[cfg(CONFIG_KUNIT)]` before the module declaration.
- let config_kunit = "#[cfg(CONFIG_KUNIT)]".to_owned().parse().unwrap();
- tokens.insert(
0,TokenTree::Group(Group::new(Delimiter::None, config_kunit)),- );
- // Generate the test KUnit test suite and a test case for each `#[test]`.
- // The code generated for the following test module:
- //
- // ```
- // #[kunit_tests(kunit_test_suit_name)]
- // mod tests {
- // #[test]
- // fn foo() {
- // assert_eq!(1, 1);
- // }
- //
- // #[test]
- // fn bar() {
- // assert_eq!(2, 2);
- // }
- // }
- // ```
- //
- // Looks like:
- //
- // ```
- // 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);
- // ```
This is a really helpful comment. It got me wondering: can we have host-side unit tests for our proc macros? Code is better than comments, of course.
- let mut kunit_macros = "".to_owned();
- let mut test_cases = "".to_owned();
- for test in &tests {
let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test);let kunit_wrapper = format!("unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",kunit_wrapper_fn_name, test);writeln!(kunit_macros, "{kunit_wrapper}").unwrap();writeln!(test_cases," kernel::kunit::kunit_case(kernel::c_str!(\"{}\"), {}),",test, kunit_wrapper_fn_name).unwrap();- }
- writeln!(kunit_macros).unwrap();
- writeln!(
kunit_macros,"static mut TEST_CASES: [kernel::bindings::kunit_case; {}] = [\n{test_cases} kernel::kunit::kunit_case_null(),\n];",tests.len() + 1- )
- .unwrap();
- writeln!(
kunit_macros,"kernel::kunit_unsafe_test_suite!({attr}, TEST_CASES);"- )
- .unwrap();
- // Remove the `#[test]` macros.
This makes sense. I wonder if we should think about being able to declare a test that runs both on host and in KUnit.
- // We do this at a token level, in order to preserve span information.
- let mut new_body = vec![];
- let mut body_it = body.stream().into_iter();
- while let Some(token) = body_it.next() {
match token {TokenTree::Punct(ref c) if c.as_char() == '#' => match body_it.next() {Some(TokenTree::Group(group)) if group.to_string() == "[test]" => (),Some(next) => {new_body.extend([token, next]);}_ => {new_body.push(token);}},_ => {new_body.push(token);}}- }
- let mut new_body = TokenStream::from_iter(new_body);
- new_body.extend::<TokenStream>(kunit_macros.parse().unwrap());
- tokens.push(TokenTree::Group(Group::new(Delimiter::Brace, new_body)));
- tokens.into_iter().collect()
+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d61bc6a56425..50b58259c577 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod helpers; +mod kunit; mod module; mod paste; mod pin_data; @@ -492,3 +493,31 @@ pub fn paste(input: TokenStream) -> TokenStream { pub fn derive_zeroable(input: TokenStream) -> TokenStream { zeroable::derive(input) }
+/// Registers a KUnit test suite and its test cases using a user-space like syntax. +/// +/// This macro should be used on modules. If `CONFIG_KUNIT` (in `.config`) is `n`, the target module +/// is ignored. +/// +/// # Examples +/// +/// ```ignore +/// # use macros::kunit_tests; +/// +/// #[kunit_tests(kunit_test_suit_name)] +/// mod tests { +/// #[test] +/// fn foo() { +/// assert_eq!(1, 1); +/// } +/// +/// #[test] +/// fn bar() { +/// assert_eq!(2, 2); +/// } +/// } +/// ``` +#[proc_macro_attribute] +pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
- kunit::kunit_tests(attr, ts)
+}
2.48.1.601.g30ceb7b040-goog
On Fri, Feb 14, 2025 at 3:41 PM Tamir Duberstein tamird@gmail.com wrote:
This is a really helpful comment. It got me wondering: can we have host-side unit tests for our proc macros? Code is better than comments, of course.
That makes sense (in fact, e.g. Benno wanted them for pinned-init), but I will defer that until we have the new build system to avoid adding more things to our plate.
This makes sense. I wonder if we should think about being able to declare a test that runs both on host and in KUnit.
Yeah, when we originally discussed `#[test]`s (years ago), we wanted to have "attributes" or "tags" like `#[test(host, kernel)]`.
But, again, something for later -- I would rather we finally land `#[test]`s.
Cheers, Miguel
On Fri, Feb 14, 2025 at 1:38 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Fri, Feb 14, 2025 at 3:41 PM Tamir Duberstein tamird@gmail.com wrote:
This is a really helpful comment. It got me wondering: can we have host-side unit tests for our proc macros? Code is better than comments, of course.
That makes sense (in fact, e.g. Benno wanted them for pinned-init), but I will defer that until we have the new build system to avoid adding more things to our plate.
This makes sense. I wonder if we should think about being able to declare a test that runs both on host and in KUnit.
Yeah, when we originally discussed `#[test]`s (years ago), we wanted to have "attributes" or "tags" like `#[test(host, kernel)]`.
But, again, something for later -- I would rather we finally land `#[test]`s.
Cheers, Miguel
Works for me. Could you link to issues tracking these, if they exist?
Reviewed-by: Tamir Duberstein tamird@gmail.com
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because: - a test could be running in another thread, - some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org ---
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/ - Greatly improved documentation, which is both clearer and better matches the rustdoc norm. (Thanks, Miguel) - The examples and safety comments are also both more idiomatic an cleaner. (Thanks, Miguel) - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go... - Rebased against 6.13-rc1 - Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go... - The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/ - Rebased on top of rust-next. - Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own. - (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.) --- rust/kernel/kunit.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 9e27b74a605b..3aad7a281b6d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// Returns whether we are currently running a KUnit test. +/// +/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function allows to change behavior depending on whether we are +/// currently running a KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// return 100; +/// } +/// +/// n + 1 +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked: +/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// #[cfg(not(CONFIG_KUNIT))] +/// use kernel::bindings; +/// +/// // This module mocks `bindings`. +/// #[cfg(CONFIG_KUNIT)] +/// mod bindings_mock_example { +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { +/// 1234 +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. +/// unsafe { bindings::ktime_get_boot_fast_ns() } +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ``` +pub fn in_kunit_test() -> bool { + // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for + // when KUnit is not enabled). + unsafe { !bindings::kunit_get_current_test().is_null() } +} + #[kunit_tests(rust_kernel_kunit)] mod tests { + use super::*; + #[test] fn rust_test_kunit_example_test() { #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); } + + #[test] + fn rust_test_kunit_in_kunit_test() { + assert!(in_kunit_test()); + } }
On Fri, Feb 14, 2025 at 2:42 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because:
- a test could be running in another thread,
- some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go...
- Rebased against 6.13-rc1
- Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go...
- The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/
- Rebased on top of rust-next.
- Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own.
- (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.)
rust/kernel/kunit.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 9e27b74a605b..3aad7a281b6d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// Returns whether we are currently running a KUnit test. +/// +/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function allows to change behavior depending on whether we are +/// currently running a KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// return 100; +/// } +/// +/// n + 1 +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked:
[`bindings`] here, please. There are two more instances below but those aren't doc comments, so I don't think bracketing them will do anything.
+/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// #[cfg(not(CONFIG_KUNIT))] +/// use kernel::bindings; +/// +/// // This module mocks `bindings`. +/// #[cfg(CONFIG_KUNIT)] +/// mod bindings_mock_example { +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { +/// 1234 +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. +/// unsafe { bindings::ktime_get_boot_fast_ns() } +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ```
Isn't this swapping out the bindings module at compile time, and for the whole build? In other words cfg(CONFIG_KUNIT) will apply to all code, both test and non-test.
+pub fn in_kunit_test() -> bool {
- // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
- // when KUnit is not enabled).
- unsafe { !bindings::kunit_get_current_test().is_null() }
Nit if you care about reducing unsafe blocks:
!unsafe { bindings::kunit_get_current_test() }.is_null()
+}
#[kunit_tests(rust_kernel_kunit)] mod tests {
- use super::*;
- #[test] fn rust_test_kunit_example_test() { #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); }
- #[test]
- fn rust_test_kunit_in_kunit_test() {
assert!(in_kunit_test());- }
}
2.48.1.601.g30ceb7b040-goog
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
On Fri, Feb 14, 2025 at 2:42 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because:
- a test could be running in another thread,
- some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go...
- Rebased against 6.13-rc1
- Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go...
- The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/
- Rebased on top of rust-next.
- Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own.
- (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.)
rust/kernel/kunit.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 9e27b74a605b..3aad7a281b6d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// Returns whether we are currently running a KUnit test. +/// +/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function allows to change behavior depending on whether we are +/// currently running a KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// return 100; +/// } +/// +/// n + 1 +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked:
[`bindings`] here, please. There are two more instances below but those aren't doc comments, so I don't think bracketing them will do anything.
Done in v7. Alas, I'll have to keep getting used to the differences between kerneldoc and rustdoc...
+/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// #[cfg(not(CONFIG_KUNIT))] +/// use kernel::bindings; +/// +/// // This module mocks `bindings`. +/// #[cfg(CONFIG_KUNIT)] +/// mod bindings_mock_example { +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { +/// 1234 +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. +/// unsafe { bindings::ktime_get_boot_fast_ns() } +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ```
Isn't this swapping out the bindings module at compile time, and for the whole build? In other words cfg(CONFIG_KUNIT) will apply to all code, both test and non-test.
I believe so, so this is probably something best done only in test files.
Ideally, we'd have support for something like the KUnit function mocking features here, but that's horribly C-specific at the moment.
+pub fn in_kunit_test() -> bool {
- // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
- // when KUnit is not enabled).
- unsafe { !bindings::kunit_get_current_test().is_null() }
Nit if you care about reducing unsafe blocks:
!unsafe { bindings::kunit_get_current_test() }.is_null()
Huh, I thought this wouldn't work, but it's working fine for me here, so I've made the change.
Thanks!
+}
#[kunit_tests(rust_kernel_kunit)] mod tests {
- use super::*;
- #[test] fn rust_test_kunit_example_test() { #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); }
- #[test]
- fn rust_test_kunit_in_kunit_test() {
assert!(in_kunit_test());- }
}
2.48.1.601.g30ceb7b040-goog
Thanks a lot, these should be fixed in v7.
Cheers, -- David
On Sat, Feb 15, 2025 at 4:03 AM David Gow davidgow@google.com wrote:
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
On Fri, Feb 14, 2025 at 2:42 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because:
- a test could be running in another thread,
- some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go...
- Rebased against 6.13-rc1
- Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go...
- The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/
- Rebased on top of rust-next.
- Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own.
- (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.)
rust/kernel/kunit.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 9e27b74a605b..3aad7a281b6d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// Returns whether we are currently running a KUnit test. +/// +/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function allows to change behavior depending on whether we are +/// currently running a KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// return 100; +/// } +/// +/// n + 1 +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked:
[`bindings`] here, please. There are two more instances below but those aren't doc comments, so I don't think bracketing them will do anything.
Done in v7. Alas, I'll have to keep getting used to the differences between kerneldoc and rustdoc...
+/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// #[cfg(not(CONFIG_KUNIT))] +/// use kernel::bindings; +/// +/// // This module mocks `bindings`. +/// #[cfg(CONFIG_KUNIT)] +/// mod bindings_mock_example { +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { +/// 1234 +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. +/// unsafe { bindings::ktime_get_boot_fast_ns() } +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ```
Isn't this swapping out the bindings module at compile time, and for the whole build? In other words cfg(CONFIG_KUNIT) will apply to all code, both test and non-test.
I believe so, so this is probably something best done only in test files.
Why would you need conditional compilation of this kind in test files?
What I was getting at with this comment is that this example might mislead a user to think that this is how they should imbue their code with test-specific behavior, which is not what this will do. Instead this would break kernels built with CONFIG_KUNIT, which I think is not where we want to be going. Right?
Ideally, we'd have support for something like the KUnit function mocking features here, but that's horribly C-specific at the moment.
+pub fn in_kunit_test() -> bool {
- // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
- // when KUnit is not enabled).
- unsafe { !bindings::kunit_get_current_test().is_null() }
Nit if you care about reducing unsafe blocks:
!unsafe { bindings::kunit_get_current_test() }.is_null()
Huh, I thought this wouldn't work, but it's working fine for me here, so I've made the change.
Thanks!
+}
#[kunit_tests(rust_kernel_kunit)] mod tests {
- use super::*;
- #[test] fn rust_test_kunit_example_test() { #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); }
- #[test]
- fn rust_test_kunit_in_kunit_test() {
assert!(in_kunit_test());- }
}
2.48.1.601.g30ceb7b040-goog
Thanks a lot, these should be fixed in v7.
Cheers, -- David
On Sat, 15 Feb 2025 at 21:04, Tamir Duberstein tamird@gmail.com wrote:
On Sat, Feb 15, 2025 at 4:03 AM David Gow davidgow@google.com wrote:
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein tamird@gmail.com wrote:
On Fri, Feb 14, 2025 at 2:42 AM David Gow davidgow@google.com wrote:
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because:
- a test could be running in another thread,
- some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com Co-developed-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Miguel Ojeda ojeda@kernel.org
Changes since v5: https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go...
- Rebased against 6.13-rc1
- Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go...
- The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/
- Rebased on top of rust-next.
- Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own.
- (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.)
rust/kernel/kunit.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 9e27b74a605b..3aad7a281b6d 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// Returns whether we are currently running a KUnit test. +/// +/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function allows to change behavior depending on whether we are +/// currently running a KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// return 100; +/// } +/// +/// n + 1 +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked:
[`bindings`] here, please. There are two more instances below but those aren't doc comments, so I don't think bracketing them will do anything.
Done in v7. Alas, I'll have to keep getting used to the differences between kerneldoc and rustdoc...
+/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// #[cfg(not(CONFIG_KUNIT))] +/// use kernel::bindings; +/// +/// // This module mocks `bindings`. +/// #[cfg(CONFIG_KUNIT)] +/// mod bindings_mock_example { +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { +/// 1234 +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. +/// unsafe { bindings::ktime_get_boot_fast_ns() } +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ```
Isn't this swapping out the bindings module at compile time, and for the whole build? In other words cfg(CONFIG_KUNIT) will apply to all code, both test and non-test.
I believe so, so this is probably something best done only in test files.
Why would you need conditional compilation of this kind in test files?
What I was getting at with this comment is that this example might mislead a user to think that this is how they should imbue their code with test-specific behavior, which is not what this will do. Instead this would break kernels built with CONFIG_KUNIT, which I think is not where we want to be going. Right?
Hmm... I think I get this now. (I confess, I'm still wrapping my head around the crate/module/file boundaries in Rust, so assumed there must be something clever going on.)
I'd definitely rather CONFIG_KUNIT not break a kernel, particularly since some distros do enable it (at least as a module) by default. So, at the very least, this should be behind a more specific kconfig. But it'd be better still to not break anything at all.
José: am I missing something here, or does this need either changing or removing?
Maybe we should just have the 'mock' version include something like: if in_kunit_test() { 1234 } else { bindings::ktime_get_boot_fast_ns() }
And in the long term, we can try to hook into the static stubbing framework to allow this to be turned on and off for individual functions. (I imagine we could get close with macros, which is what we do in C, though the thought of trying to deal with function pointers and Rust's lack of stable ABI here scares me.)
Either way, we're definitely going to need a follow-up documentation patch for this whole feature, so if we want to get rid of or update this example, we can either do it then, or now.
I'm afraid I'm unlikely to have time to work on Rust stuff probably for the rest of the month, so I'll let the discussion play out (and if people desperately want the patches before then, I'd be happy to fix them in follow-ups).
Cheers, -- David
On Fri, Feb 14, 2025 at 8:41 AM David Gow davidgow@google.com wrote:
After much delay, v6 of the KUnit/Rust integration patchset is here. This change incorporates most of Miguels suggestions from v5 (save for some of the copyright headers I wasn't comfortable unilaterally changing). This means the documentation is much improved, and it should work more cleanly on Rust 1.83 and 1.84, no longer requiring static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all of the details of this, but I'm comfortable enough with how it's ended up.)
Thanks a lot, David, really.
I am happy to take it through Rust or KUnit -- I am not sure yet how many conflicts we will have on our side.
I will give it a go when you think it is ready (since I saw comments from Tamir, though not sure how many you may want to address -- perhaps the sooner we land this, the easier it may be...).
Cheers, Miguel
On Sat, 15 Feb 2025 at 04:49, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Fri, Feb 14, 2025 at 8:41 AM David Gow davidgow@google.com wrote:
After much delay, v6 of the KUnit/Rust integration patchset is here. This change incorporates most of Miguels suggestions from v5 (save for some of the copyright headers I wasn't comfortable unilaterally changing). This means the documentation is much improved, and it should work more cleanly on Rust 1.83 and 1.84, no longer requiring static_mut_refs or const_mut_refs. (I'm not 100% sure I understand all of the details of this, but I'm comfortable enough with how it's ended up.)
Thanks a lot, David, really.
I am happy to take it through Rust or KUnit -- I am not sure yet how many conflicts we will have on our side.
I will give it a go when you think it is ready (since I saw comments from Tamir, though not sure how many you may want to address -- perhaps the sooner we land this, the easier it may be...).
Thanks!
I've sent out v7: https://lore.kernel.org/rust-for-linux/20250215090622.2381038-1-davidgow@goo...
This should address (most of) Tamir's comments. If that looks good to you, feel free to apply it: it'd certainly be easier on my end to have any nonessential fixes done separately, and if we can have people start to use it in the rust tree concurrently, that'd be a bonus.
Thanks again for your patience -- I think we're finally nearly there!
-- David
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Miguel Ojeda -
Tamir Duberstein