Re: [PATCH v13 2/5] rust: support formatting of foreign types

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

On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin [off-list ref] wrote:

quoted

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

quoted

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.

I don't understand, can't you just do:

* add `rust/kernel/fmt.rs`,
* add `rust/macros/fmt.rs`,
* change all occurrences of `core::fmt` to `kernel::fmt` and
  `format_args!` to `fmt!`.

Yes, such a split could be done - I will do so in the next spin

quoted

The last one could be split by subsystem, no? Some subsystems might
interact and thus need simultaneous splitting, but there should be some
independent ones.

Yes, it probably can. As you say, some subsystems might interact - the
claimed benefit of doing this subsystem-by-subsystem split is that it
avoids conflicts with ongoing work that will conflict with a large
patch, but this is also the downside; if ongoing work changes the set
of interactions between subsystems then a maintainer may find
themselves unable to emit the log message they want (because one
subsystem is using kernel::fmt while another is still on core::fmt).

I gave this a try. I ran into the problem that `format_args!` (and,
after this patch, `fmt!`) is at the center of `print_macro!`, which
itself underpins various other formatting macros. This means we'd have
to bifurcate the formatting infrastructure to support an incremental
migration. That's quite a bit of code, and likely quite a mess in the
resulting git history -- and that's setting aside the toil required to
figure out the correct combinations of subsystems that must migrate
together.