On Thu, Jul 3, 2025 at 2:55 PM Tamir Duberstein [off-list ref] wrote:

On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin [off-list ref] wrote:

quoted

On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein wrote:

quoted

On Thu, Jul 3, 2025 at 5:32 AM Benno Lossin [off-list ref] wrote:

quoted

On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:

quoted

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/Custom.20formatting/with/516476467
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <redacted>
---
 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.

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.