On Thu, Jul 3, 2025 at 2:55 PM Tamir Duberstein tamird@gmail.com wrote:
On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin lossin@kernel.org wrote:
On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote:
On Thu, Jul 3, 2025 at 5:32 AM Benno Lossin lossin@kernel.org wrote:
On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `core::fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
Replace all implementations of `core::fmt::Display` with implementations of `kernel::fmt::Display`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/block/rnull.rs | 2 +- drivers/gpu/nova-core/gpu.rs | 4 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ rust/kernel/kunit.rs | 6 +-- rust/kernel/lib.rs | 1 + rust/kernel/prelude.rs | 3 +- rust/kernel/print.rs | 4 +- rust/kernel/seq_file.rs | 2 +- rust/kernel/str.rs | 22 ++++------ rust/macros/fmt.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++++ rust/macros/quote.rs | 7 ++++ scripts/rustdoc_test_gen.rs | 2 +- 15 files changed, 236 insertions(+), 28 deletions(-)
This would be a lot easier to review if he proc-macro and the call replacement were different patches.
Also the `kernel/fmt.rs` file should be a different commit.
Can you help me understand why? The changes you ask to be separated would all be in different files, so why would separate commits make it easier to review?
It takes less time to go through the entire patch and give a RB. I can take smaller time chunks and don't have to get back into the entire context of the patch when I don't have 30-60min available.
Ah, I see what you mean. Yeah, the requirement to RB the entire patch does mean there's a benefit to smaller patches.
In this patch the biggest problem is the rename & addition of new things, maybe just adding 200 lines in those files could be okay to go together, see below for more.
After implementing your suggestion of re-exporting things from `kernel::fmt` the diffstat is
26 files changed, 253 insertions(+), 51 deletions(-)
so I guess I could do all the additions in one patch, but then *everything* else has to go in a single patch together because the formatting macros either want core::fmt::Display or kernel::fmt::Display; they can't work in a halfway state.
I prefer to keep things in one commit because the changes are highly interdependent. The proc macro doesn't make sense without kernel/fmt.rs and kernel/fmt.rs is useless without the proc macro.
I think that `Adapter`, the custom `Display` and their impl blocks don't need to be in the same commit as the proc-macro. They are related, but maybe someone is not well-versed in proc-macros and thus doesn't want to review that part.
Sure, I guess I will split them. But as noted above: changing the formatting macros and all the types' trait implementations has to be a "flag day" change.
diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs new file mode 100644 index 000000000000..348d16987de6 --- /dev/null +++ b/rust/kernel/fmt.rs @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: GPL-2.0
+//! Formatting utilities.
+use core::fmt;
I think we should pub export all types that we are still using from `core::fmt`. For example `Result`, `Formatter`, `Debug` etc.
That way I can still use the same pattern of importing `fmt` and then writing
impl fmt::Display for MyType { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {} }Great idea, done for the next spin. It would be nice to be able to lint against references to `core::fmt` outside of kernel/fmt.rs.
I think there was something in clippy that can do that globally and we could allow that in this file?
I didn't find anything suitable. Do you have one in mind?
I think we want https://github.com/rust-lang/rust-clippy/issues/14807.