This picks up from Michal Rostecki's work[0]. Per Michal's guidance I have omitted Co-authored tags, as the end result is quite different.
Link: https://lore.kernel.org/rust-for-linux/20240819153656.28807-2-vadorovsky@pro... [0] Closes: https://github.com/Rust-for-Linux/linux/issues/1075
Signed-off-by: Tamir Duberstein tamird@gmail.com --- Changes in v13: - Rebase on v6.16-rc4. - Link to v12: https://lore.kernel.org/r/20250619-cstr-core-v12-0-80c9c7b45900@gmail.com
Changes in v12: - Introduce `kernel::fmt::Display` to allow implementations on foreign types. - Tidy up doc comment on `str_to_cstr`. (Alice Ryhl). - Link to v11: https://lore.kernel.org/r/20250530-cstr-core-v11-0-cd9c0cbcb902@gmail.com
Changes in v11: - Use `quote_spanned!` to avoid `use<'a, T>` and generally reduce manual token construction. - Add a commit to simplify `quote_spanned!`. - Drop first commit in favor of https://lore.kernel.org/rust-for-linux/20240906164448.2268368-1-paddymills@p.... (Miguel Ojeda) - Correctly handle expressions such as `pr_info!("{a}", a = a = a)`. (Benno Lossin) - Avoid dealing with `}}` escapes, which is not needed. (Benno Lossin) - Revert some unnecessary changes. (Benno Lossin) - Rename `c_str_avoid_literals!` to `str_to_cstr!`. (Benno Lossin & Alice Ryhl). - Link to v10: https://lore.kernel.org/r/20250524-cstr-core-v10-0-6412a94d9d75@gmail.com
Changes in v10: - Rebase on cbeaa41dfe26b72639141e87183cb23e00d4b0dd. - Implement Alice's suggestion to use a proc macro to work around orphan rules otherwise preventing `core::ffi::CStr` to be directly printed with `{}`. - Link to v9: https://lore.kernel.org/r/20250317-cstr-core-v9-0-51d6cc522f62@gmail.com
Changes in v9: - Rebase on rust-next. - Restore `impl Display for BStr` which exists upstream[1]. - Link: https://doc.rust-lang.org/nightly/std/bstr/struct.ByteStr.html#impl-Display-... [1] - Link to v8: https://lore.kernel.org/r/20250203-cstr-core-v8-0-cb3f26e78686@gmail.com
Changes in v8: - Move `{from,as}_char_ptr` back to `CStrExt`. This reduces the diff some. - Restore `from_bytes_with_nul_unchecked_mut`, `to_cstring`. - Link to v7: https://lore.kernel.org/r/20250202-cstr-core-v7-0-da1802520438@gmail.com
Changes in v7: - Rebased on mainline. - Restore functionality added in commit a321f3ad0a5d ("rust: str: add {make,to}_{upper,lower}case() to CString"). - Used `diff.algorithm patience` to improve diff readability. - Link to v6: https://lore.kernel.org/r/20250202-cstr-core-v6-0-8469cd6d29fd@gmail.com
Changes in v6: - Split the work into several commits for ease of review. - Restore `{from,as}_char_ptr` to allow building on ARM (see commit message). - Add `CStrExt` to `kernel::prelude`. (Alice Ryhl) - Remove `CStrExt::from_bytes_with_nul_unchecked_mut` and restore `DerefMut for CString`. (Alice Ryhl) - Rename and hide `kernel::c_str!` to encourage use of C-String literals. - Drop implementation and invocation changes in kunit.rs. (Trevor Gross) - Drop docs on `Display` impl. (Trevor Gross) - Rewrite docs in the style of the standard library. - Restore the `test_cstr_debug` unit tests to demonstrate that the implementation has changed.
Changes in v5: - Keep the `test_cstr_display*` unit tests.
Changes in v4: - Provide the `CStrExt` trait with `display()` method, which returns a `CStrDisplay` wrapper with `Display` implementation. This addresses the lack of `Display` implementation for `core::ffi::CStr`. - Provide `from_bytes_with_nul_unchecked_mut()` method in `CStrExt`, which might be useful and is going to prevent manual, unsafe casts. - Fix a typo (s/preffered/prefered/).
Changes in v3: - Fix the commit message. - Remove redundant braces in `use`, when only one item is imported.
Changes in v2: - Do not remove `c_str` macro. While it's preferred to use C-string literals, there are two cases where `c_str` is helpful: - When working with macros, which already return a Rust string literal (e.g. `stringify!`). - When building macros, where we want to take a Rust string literal as an argument (for caller's convenience), but still use it as a C-string internally. - Use Rust literals as arguments in macros (`new_mutex`, `new_condvar`, `new_mutex`). Use the `c_str` macro to convert these literals to C-string literals. - Use `c_str` in kunit.rs for converting the output of `stringify!` to a `CStr`. - Remove `DerefMut` implementation for `CString`.
--- Tamir Duberstein (5): rust: macros: reduce collections in `quote!` macro rust: support formatting of foreign types rust: replace `CStr` with `core::ffi::CStr` rust: replace `kernel::c_str!` with C-Strings rust: remove core::ffi::CStr reexport
drivers/block/rnull.rs | 4 +- drivers/cpufreq/rcpufreq_dt.rs | 5 +- drivers/gpu/drm/drm_panic_qr.rs | 5 +- drivers/gpu/drm/nova/driver.rs | 10 +- drivers/gpu/nova-core/driver.rs | 6 +- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/gpu/nova-core/gpu.rs | 4 +- drivers/gpu/nova-core/nova_core.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 8 +- drivers/net/phy/qt2025.rs | 6 +- rust/kernel/auxiliary.rs | 6 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/clk.rs | 9 +- rust/kernel/configfs.rs | 14 +- rust/kernel/cpufreq.rs | 6 +- rust/kernel/device.rs | 9 +- rust/kernel/devres.rs | 2 +- rust/kernel/driver.rs | 4 +- rust/kernel/drm/device.rs | 4 +- rust/kernel/drm/driver.rs | 3 +- rust/kernel/drm/ioctl.rs | 2 +- rust/kernel/error.rs | 10 +- rust/kernel/faux.rs | 5 +- rust/kernel/firmware.rs | 16 +- rust/kernel/fmt.rs | 89 +++++++ rust/kernel/kunit.rs | 28 +-- rust/kernel/lib.rs | 3 +- rust/kernel/miscdevice.rs | 5 +- rust/kernel/net/phy.rs | 12 +- rust/kernel/of.rs | 5 +- rust/kernel/pci.rs | 2 +- rust/kernel/platform.rs | 6 +- rust/kernel/prelude.rs | 5 +- rust/kernel/print.rs | 4 +- rust/kernel/seq_file.rs | 6 +- rust/kernel/str.rs | 444 ++++++++++------------------------ rust/kernel/sync.rs | 7 +- rust/kernel/sync/completion.rs | 2 +- rust/kernel/sync/condvar.rs | 4 +- rust/kernel/sync/lock.rs | 4 +- rust/kernel/sync/lock/global.rs | 6 +- rust/kernel/sync/poll.rs | 1 + rust/kernel/workqueue.rs | 9 +- rust/macros/fmt.rs | 99 ++++++++ rust/macros/kunit.rs | 10 +- rust/macros/lib.rs | 19 ++ rust/macros/module.rs | 2 +- rust/macros/quote.rs | 111 ++++----- samples/rust/rust_configfs.rs | 9 +- samples/rust/rust_driver_auxiliary.rs | 7 +- samples/rust/rust_driver_faux.rs | 4 +- samples/rust/rust_driver_pci.rs | 4 +- samples/rust/rust_driver_platform.rs | 4 +- samples/rust/rust_misc_device.rs | 3 +- scripts/rustdoc_test_gen.rs | 6 +- 55 files changed, 543 insertions(+), 521 deletions(-) --- base-commit: 769e324b66b0d92d04f315d0c45a0f72737c7494 change-id: 20250201-cstr-core-d4b9b69120cf
Best regards, -- Tamir Duberstein tamird@gmail.com
Remove a handful of unncessary intermediate vectors and token streams; mainly the top-level stream can be directly extended with the notable exception of groups.
Remove an unnecessary `#[allow(dead_code)]` added in commit dbd5058ba60c ("rust: make pin-init its own crate").
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com --- rust/macros/quote.rs | 104 ++++++++++++++++++++++++--------------------------- 1 file changed, 49 insertions(+), 55 deletions(-)
diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs index 92cacc4067c9..acc140c18653 100644 --- a/rust/macros/quote.rs +++ b/rust/macros/quote.rs @@ -2,7 +2,6 @@
use proc_macro::{TokenStream, TokenTree};
-#[allow(dead_code)] pub(crate) trait ToTokens { fn to_tokens(&self, tokens: &mut TokenStream); } @@ -47,121 +46,116 @@ fn to_tokens(&self, tokens: &mut TokenStream) { /// `quote` crate but provides only just enough functionality needed by the current `macros` crate. macro_rules! quote_spanned { ($span:expr => $($tt:tt)*) => {{ - let mut tokens: ::std::vec::Vec<::proc_macro::TokenTree>; - #[allow(clippy::vec_init_then_push)] + let mut tokens = ::proc_macro::TokenStream::new(); { - tokens = ::std::vec::Vec::new(); let span = $span; quote_spanned!(@proc tokens span $($tt)*); } - ::proc_macro::TokenStream::from_iter(tokens) + tokens }}; (@proc $v:ident $span:ident) => {}; (@proc $v:ident $span:ident #$id:ident $($tt:tt)*) => { - let mut ts = ::proc_macro::TokenStream::new(); - $crate::quote::ToTokens::to_tokens(&$id, &mut ts); - $v.extend(ts); + $crate::quote::ToTokens::to_tokens(&$id, &mut $v); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident #(#$id:ident)* $($tt:tt)*) => { for token in $id { - let mut ts = ::proc_macro::TokenStream::new(); - $crate::quote::ToTokens::to_tokens(&token, &mut ts); - $v.extend(ts); + $crate::quote::ToTokens::to_tokens(&token, &mut $v); } quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident ( $($inner:tt)* ) $($tt:tt)*) => { #[allow(unused_mut)] - let mut tokens = ::std::vec::Vec::<::proc_macro::TokenTree>::new(); + let mut tokens = ::proc_macro::TokenStream::new(); quote_spanned!(@proc tokens $span $($inner)*); - $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new( + $v.extend([::proc_macro::TokenTree::Group(::proc_macro::Group::new( ::proc_macro::Delimiter::Parenthesis, - ::proc_macro::TokenStream::from_iter(tokens) - ))); + tokens, + ))]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident [ $($inner:tt)* ] $($tt:tt)*) => { - let mut tokens = ::std::vec::Vec::new(); + let mut tokens = ::proc_macro::TokenStream::new(); quote_spanned!(@proc tokens $span $($inner)*); - $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new( + $v.extend([::proc_macro::TokenTree::Group(::proc_macro::Group::new( ::proc_macro::Delimiter::Bracket, - ::proc_macro::TokenStream::from_iter(tokens) - ))); + tokens, + ))]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident { $($inner:tt)* } $($tt:tt)*) => { - let mut tokens = ::std::vec::Vec::new(); + let mut tokens = ::proc_macro::TokenStream::new(); quote_spanned!(@proc tokens $span $($inner)*); - $v.push(::proc_macro::TokenTree::Group(::proc_macro::Group::new( + $v.extend([::proc_macro::TokenTree::Group(::proc_macro::Group::new( ::proc_macro::Delimiter::Brace, - ::proc_macro::TokenStream::from_iter(tokens) - ))); + tokens, + ))]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident :: $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new(':', ::proc_macro::Spacing::Joint) - )); - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new(':', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::Spacing::Joint, ::proc_macro::Spacing::Alone].map(|spacing| { + ::proc_macro::TokenTree::Punct(::proc_macro::Punct::new(':', spacing)) + })); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident : $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new(':', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new(':', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident , $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new(',', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new(',', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident @ $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new('@', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('@', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident ! $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new('!', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('!', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident ; $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new(';', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new(';', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident + $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new('+', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('+', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident = $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new('=', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('=', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident # $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Punct( - ::proc_macro::Punct::new('#', ::proc_macro::Spacing::Alone) - )); + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('#', ::proc_macro::Spacing::Alone), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident _ $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new("_", $span))); + $v.extend([::proc_macro::TokenTree::Ident( + ::proc_macro::Ident::new("_", $span), + )]); quote_spanned!(@proc $v $span $($tt)*); }; (@proc $v:ident $span:ident $id:ident $($tt:tt)*) => { - $v.push(::proc_macro::TokenTree::Ident(::proc_macro::Ident::new(stringify!($id), $span))); + $v.extend([::proc_macro::TokenTree::Ident( + ::proc_macro::Ident::new(stringify!($id), $span), + )]); quote_spanned!(@proc $v $span $($tt)*); }; }
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(-)
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs index d07e76ae2c13..6366da12c5a5 100644 --- a/drivers/block/rnull.rs +++ b/drivers/block/rnull.rs @@ -51,7 +51,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> { .logical_block_size(4096)? .physical_block_size(4096)? .rotational(false) - .build(format_args!("rnullb{}", 0), tagset) + .build(fmt!("rnullb{}", 0), tagset) })();
try_pin_init!(Self { diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index 60b86f370284..e6b208175ff9 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -91,7 +91,7 @@ pub(crate) fn arch(&self) -> Architecture { // Hence, replace with something like strum_macros derive(Display). // // For now, redirect to fmt::Debug for convenience. -impl fmt::Display for Chipset { +impl kernel::fmt::Display for Chipset { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{self:?}") } @@ -132,7 +132,7 @@ fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self { } }
-impl fmt::Display for Revision { +impl kernel::fmt::Display for Revision { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{:x}.{:x}", self.major, self.minor) } diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index 831445d37181..61ea35bba7d5 100644 --- a/rust/kernel/block/mq.rs +++ b/rust/kernel/block/mq.rs @@ -82,7 +82,7 @@ //! Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?; //! let mut disk = gen_disk::GenDiskBuilder::new() //! .capacity_sectors(4096) -//! .build(format_args!("myblk"), tagset)?; +//! .build(fmt!("myblk"), tagset)?; //! //! # Ok::<(), kernel::error::Error>(()) //! ``` diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index 5c946af3a4d5..86b945576780 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -345,7 +345,7 @@ macro_rules! impl_device_context_into_aref { macro_rules! dev_printk { ($method:ident, $dev:expr, $($f:tt)*) => { { - ($dev).$method(::core::format_args!($($f)*)); + ($dev).$method($crate::prelude::fmt!($($f)*)); } } } 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; + +/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T); + +macro_rules! impl_fmt_adapter_forward { + ($($trait:ident),* $(,)?) => { + $( + impl<T: fmt::$trait> fmt::$trait for Adapter<T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(t) = self; + fmt::$trait::fmt(t, f) + } + } + )* + }; +} + +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp); + +/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display { + /// Same as [`fmt::Display::fmt`]. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result; +} + +impl<T: ?Sized + Display> Display for &T { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + Display::fmt(*self, f) + } +} + +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(t) = self; + Display::fmt(t, f) + } +} + +macro_rules! impl_display_forward { + ($( + $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )? + ),* $(,)?) => { + $( + impl$($($generics)*)? Display for $ty $(where $($where)*)? { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, f) + } + } + )* + }; +} + +impl_display_forward!( + bool, + char, + core::panic::PanicInfo<'_>, + fmt::Arguments<'_>, + i128, + i16, + i32, + i64, + i8, + isize, + str, + u128, + u16, + u32, + u64, + u8, + usize, + {<T: ?Sized>} crate::sync::Arc<T> {where crate::sync::Arc<T>: fmt::Display}, + {<T: ?Sized>} crate::sync::UniqueArc<T> {where crate::sync::UniqueArc<T>: fmt::Display}, +); diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 099a61bbb8f4..3539edbceaf5 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -74,14 +74,14 @@ macro_rules! kunit_assert { // mistake (it is hidden to prevent that). // // This mimics KUnit's failed assertion format. - $crate::kunit::err(format_args!( + $crate::kunit::err($crate::prelude::fmt!( " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", $name )); - $crate::kunit::err(format_args!( + $crate::kunit::err($crate::prelude::fmt!( " Expected {CONDITION} to be true, but is false\n" )); - $crate::kunit::err(format_args!( + $crate::kunit::err($crate::prelude::fmt!( " Failure not reported to KUnit since this is a non-KUnit task\n" )); break 'out; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 6b4774b2b1c3..aadcfaa5c759 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -77,6 +77,7 @@ pub mod faux; #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware; +pub mod fmt; pub mod fs; pub mod init; pub mod io; diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index 2f30a398dddd..41cebd906c4c 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -22,7 +22,7 @@ pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)] -pub use macros::{export, kunit_tests, module, vtable}; +pub use macros::{export, fmt, kunit_tests, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
@@ -31,7 +31,6 @@ // `super::std_vendor` is hidden, which makes the macro inline for some reason. #[doc(no_inline)] pub use super::dbg; -pub use super::fmt; pub use super::{dev_alert, dev_crit, dev_dbg, dev_emerg, dev_err, dev_info, dev_notice, dev_warn}; pub use super::{pr_alert, pr_crit, pr_debug, pr_emerg, pr_err, pr_info, pr_notice, pr_warn};
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index ecdcee43e5a5..d9c619edcd2f 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -149,7 +149,7 @@ macro_rules! print_macro ( // takes borrows on the arguments, but does not extend the scope of temporaries. // Therefore, a `match` expression is used to keep them around, since // the scrutinee is kept until the end of the `match`. - match format_args!($($arg)+) { + match $crate::prelude::fmt!($($arg)+) { // SAFETY: This hidden macro should only be called by the documented // printing macros which ensure the format string is one of the fixed // ones. All `__LOG_PREFIX`s are null-terminated as they are generated @@ -168,7 +168,7 @@ macro_rules! print_macro ( // The `CONT` case. ($format_string:path, true, $($arg:tt)+) => ( $crate::print::call_printk_cont( - format_args!($($arg)+), + $crate::prelude::fmt!($($arg)+), ); ); ); diff --git a/rust/kernel/seq_file.rs b/rust/kernel/seq_file.rs index 8f199b1a3bb1..05c9b7bec727 100644 --- a/rust/kernel/seq_file.rs +++ b/rust/kernel/seq_file.rs @@ -47,7 +47,7 @@ pub fn call_printf(&self, args: core::fmt::Arguments<'_>) { #[macro_export] macro_rules! seq_print { ($m:expr, $($arg:tt)+) => ( - $m.call_printf(format_args!($($arg)+)) + $m.call_printf($crate::prelude::fmt!($($arg)+)) ); } pub use seq_print; diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index cbc8b459ed41..d3fa4b703d35 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -50,11 +50,11 @@ pub fn strip_prefix(&self, pattern: impl AsRef<Self>) -> Option<&BStr> { } }
-impl fmt::Display for BStr { +impl crate::fmt::Display for BStr { /// Formats printable ASCII characters, escaping the rest. /// /// ``` - /// # use kernel::{fmt, b_str, str::{BStr, CString}}; + /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}}; /// let ascii = b_str!("Hello, BStr!"); /// let s = CString::try_from_fmt(fmt!("{}", ascii))?; /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes()); @@ -85,7 +85,7 @@ impl fmt::Debug for BStr { /// escaping the rest. /// /// ``` - /// # use kernel::{fmt, b_str, str::{BStr, CString}}; + /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}}; /// // Embedded double quotes are escaped. /// let ascii = b_str!("Hello, "BStr"!"); /// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?; @@ -424,12 +424,12 @@ pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> { } }
-impl fmt::Display for CStr { +impl crate::fmt::Display for CStr { /// Formats printable ASCII characters, escaping the rest. /// /// ``` /// # use kernel::c_str; - /// # use kernel::fmt; + /// # use kernel::prelude::fmt; /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c_str!("🐧"); @@ -459,7 +459,7 @@ impl fmt::Debug for CStr { /// /// ``` /// # use kernel::c_str; - /// # use kernel::fmt; + /// # use kernel::prelude::fmt; /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c_str!("🐧"); @@ -578,7 +578,7 @@ mod tests {
macro_rules! format { ($($f:tt)*) => ({ - CString::try_from_fmt(::kernel::fmt!($($f)*))?.to_str()? + CString::try_from_fmt(crate::prelude::fmt!($($f)*))?.to_str()? }) }
@@ -840,7 +840,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result { /// # Examples /// /// ``` -/// use kernel::{str::CString, fmt}; +/// use kernel::{str::CString, prelude::fmt}; /// /// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20))?; /// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes()); @@ -930,9 +930,3 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Debug::fmt(&**self, f) } } - -/// A convenience alias for [`core::format_args`]. -#[macro_export] -macro_rules! fmt { - ($($f:tt)*) => ( ::core::format_args!($($f)*) ) -} diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..edc37c220a89 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0 + +use proc_macro::{Ident, TokenStream, TokenTree}; +use std::collections::BTreeSet; + +/// Please see [`crate::fmt`] for documentation. +pub(crate) fn fmt(input: TokenStream) -> TokenStream { + let mut input = input.into_iter(); + + let first_opt = input.next(); + let first_owned_str; + let mut names = BTreeSet::new(); + let first_lit = { + let Some((mut first_str, first_lit)) = (match first_opt.as_ref() { + Some(TokenTree::Literal(first_lit)) => { + first_owned_str = first_lit.to_string(); + Some(first_owned_str.as_str()).and_then(|first| { + let first = first.strip_prefix('"')?; + let first = first.strip_suffix('"')?; + Some((first, first_lit)) + }) + } + _ => None, + }) else { + return first_opt.into_iter().chain(input).collect(); + }; + while let Some((_, rest)) = first_str.split_once('{') { + first_str = rest; + if let Some(rest) = first_str.strip_prefix('{') { + first_str = rest; + continue; + } + if let Some((name, rest)) = first_str.split_once('}') { + first_str = rest; + let name = name.split_once(':').map_or(name, |(name, _)| name); + if !name.is_empty() && !name.chars().all(|c| c.is_ascii_digit()) { + names.insert(name); + } + } + } + first_lit + }; + + let first_span = first_lit.span(); + let adapter = quote_spanned! { + first_span => ::kernel::fmt::Adapter + }; + + let mut args = TokenStream::from_iter(first_opt); + { + let mut flush = |args: &mut TokenStream, current: &mut TokenStream| { + let current = std::mem::take(current); + if !current.is_empty() { + let (lhs, rhs) = (|| { + let mut current = current.into_iter(); + let mut acc = TokenStream::new(); + while let Some(tt) = current.next() { + // Split on `=` only once to handle cases like `a = b = c`. + if matches!(&tt, TokenTree::Punct(p) if p.as_char() == '=') { + names.remove(acc.to_string().as_str()); + // Include the `=` itself to keep the handling below uniform. + acc.extend([tt]); + return (Some(acc), current.collect::<TokenStream>()); + } + acc.extend([tt]); + } + (None, acc) + })(); + args.extend(quote_spanned! { + first_span => #lhs #adapter(&#rhs) + }); + } + }; + + let mut current = TokenStream::new(); + for tt in input { + match &tt { + TokenTree::Punct(p) if p.as_char() == ',' => { + flush(&mut args, &mut current); + &mut args + } + _ => &mut current, + } + .extend([tt]); + } + flush(&mut args, &mut current); + } + + for name in names { + let name = Ident::new(name, first_span); + args.extend(quote_spanned! { + first_span => , #name = #adapter(&#name) + }); + } + + quote_spanned! { + first_span => ::core::format_args!(#args) + } +} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index fa847cf3a9b5..980e70d253e4 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -15,6 +15,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -201,6 +202,24 @@ pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream { export::export(attr, ts) }
+/// Like [`core::format_args!`], but automatically wraps arguments in [`kernel::fmt::Adapter`]. +/// +/// This macro allows generating `core::fmt::Arguments` while ensuring that each argument is wrapped +/// with `::kernel::fmt::Adapter`, which customizes formatting behavior for kernel logging. +/// +/// Named arguments used in the format string (e.g. `{foo}`) are detected and resolved from local +/// bindings. All positional and named arguments are automatically wrapped. +/// +/// This macro is an implementation detail of other kernel logging macros like [`pr_info!`] and +/// should not typically be used directly. +/// +/// [`kernel::fmt::Adapter`]: ../kernel/fmt/struct.Adapter.html +/// [`pr_info!`]: ../kernel/macro.pr_info.html +#[proc_macro] +pub fn fmt(input: TokenStream) -> TokenStream { + fmt::fmt(input) +} + /// Concatenate two identifiers. /// /// This is useful in macros that need to declare or reference items with names diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs index acc140c18653..ddfc21577539 100644 --- a/rust/macros/quote.rs +++ b/rust/macros/quote.rs @@ -48,6 +48,7 @@ macro_rules! quote_spanned { ($span:expr => $($tt:tt)*) => {{ let mut tokens = ::proc_macro::TokenStream::new(); { + #[allow(unused_variables)] let span = $span; quote_spanned!(@proc tokens span $($tt)*); } @@ -146,6 +147,12 @@ macro_rules! quote_spanned { )]); quote_spanned!(@proc $v $span $($tt)*); }; + (@proc $v:ident $span:ident & $($tt:tt)*) => { + $v.extend([::proc_macro::TokenTree::Punct( + ::proc_macro::Punct::new('&', ::proc_macro::Spacing::Alone), + )]); + quote_spanned!(@proc $v $span $($tt)*); + }; (@proc $v:ident $span:ident _ $($tt:tt)*) => { $v.extend([::proc_macro::TokenTree::Ident( ::proc_macro::Ident::new("_", $span), diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index 1ca253594d38..507d36875196 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -201,7 +201,7 @@ macro_rules! assert_eq {{ // This follows the syntax for declaring test metadata in the proposed KTAP v2 spec, which may // be used for the proposed KUnit test attributes API. Thus hopefully this will make migration // easier later on. - ::kernel::kunit::info(format_args!(" # {kunit_name}.location: {real_path}:{line}\n")); + ::kernel::kunit::info(fmt!(" # {kunit_name}.location: {real_path}:{line}\n"));
/// The anchor where the test code body starts. #[allow(unused)]
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.
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 {} }
+/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T);
+macro_rules! impl_fmt_adapter_forward {
- ($($trait:ident),* $(,)?) => {
$(
impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
fmt::$trait::fmt(t, f)
}
}
)*
- };
+}
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
- }
+}
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?
- ),* $(,)?) => {
$(
impl$($($generics)*)? Display for $ty $(where $($where)*)? {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(self, f)
}
}
)*
- };
+}
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- fmt::Arguments<'_>,
- i128,
- i16,
- i32,
- i64,
- i8,
- isize,
- str,
- u128,
- u16,
- u32,
- u64,
- u8,
- usize,
- {<T: ?Sized>} crate::sync::Arc<T> {where crate::sync::Arc<T>: fmt::Display},
- {<T: ?Sized>} crate::sync::UniqueArc<T> {where crate::sync::UniqueArc<T>: fmt::Display},
+);
diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..edc37c220a89 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0
+use proc_macro::{Ident, TokenStream, TokenTree}; +use std::collections::BTreeSet;
+/// Please see [`crate::fmt`] for documentation. +pub(crate) fn fmt(input: TokenStream) -> TokenStream {
- let mut input = input.into_iter();
- let first_opt = input.next();
- let first_owned_str;
- let mut names = BTreeSet::new();
- let first_lit = {
let Some((mut first_str, first_lit)) = (match first_opt.as_ref() {
Some(TokenTree::Literal(first_lit)) => {
first_owned_str = first_lit.to_string();
Some(first_owned_str.as_str()).and_then(|first| {
let first = first.strip_prefix('"')?;
let first = first.strip_suffix('"')?;
Some((first, first_lit))
You're only using first_lit to get the span later, so why not just get the span directly here?
})
}
_ => None,
}) else {
return first_opt.into_iter().chain(input).collect();
};
while let Some((_, rest)) = first_str.split_once('{') {
Let's put a comment above this loop mentioning [1] and saying that it parses the identifiers from the format arguments.
[1]: https://doc.rust-lang.org/std/fmt/index.html#syntax
first_str = rest;
if let Some(rest) = first_str.strip_prefix('{') {
first_str = rest;
continue;
}
if let Some((name, rest)) = first_str.split_once('}') {
first_str = rest;
let name = name.split_once(':').map_or(name, |(name, _)| name);
if !name.is_empty() && !name.chars().all(|c| c.is_ascii_digit()) {
names.insert(name);
}
}
}
first_lit
- };
- let first_span = first_lit.span();
- let adapter = quote_spanned! {
first_span => ::kernel::fmt::Adapter
- };
I think we should follow the formatting convention from the quote crate:
let adapter = quote_spanned!(first_span=> ::kernel::fmt::Adapter);
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut flush = |args: &mut TokenStream, current: &mut TokenStream| {
You don't need to pass `args` as a closure argument, since you always call it with `&mut args`.
let current = std::mem::take(current);
if !current.is_empty() {
let (lhs, rhs) = (|| {
let mut current = current.into_iter();
let mut acc = TokenStream::new();
while let Some(tt) = current.next() {
// Split on `=` only once to handle cases like `a = b = c`.
if matches!(&tt, TokenTree::Punct(p) if p.as_char() == '=') {
names.remove(acc.to_string().as_str());
// Include the `=` itself to keep the handling below uniform.
acc.extend([tt]);
return (Some(acc), current.collect::<TokenStream>());
}
acc.extend([tt]);
}
(None, acc)
})();
args.extend(quote_spanned! {
first_span => #lhs #adapter(&#rhs)
});
}
};
let mut current = TokenStream::new();
Define this before the closure, then you don't need to pass it as an argument.
--- Cheers, Benno
for tt in input {
match &tt {
TokenTree::Punct(p) if p.as_char() == ',' => {
flush(&mut args, &mut current);
&mut args
}
_ => &mut current,
}
.extend([tt]);
}
flush(&mut args, &mut current);
- }
- for name in names {
let name = Ident::new(name, first_span);
args.extend(quote_spanned! {
first_span => , #name = #adapter(&#name)
});
- }
- quote_spanned! {
first_span => ::core::format_args!(#args)
- }
+}
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?
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.
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.
+/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T);
+macro_rules! impl_fmt_adapter_forward {
- ($($trait:ident),* $(,)?) => {
$(
impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
fmt::$trait::fmt(t, f)
}
}
)*
- };
+}
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
- }
+}
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?
- ),* $(,)?) => {
$(
impl$($($generics)*)? Display for $ty $(where $($where)*)? {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(self, f)
}
}
)*
- };
+}
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- fmt::Arguments<'_>,
- i128,
- i16,
- i32,
- i64,
- i8,
- isize,
- str,
- u128,
- u16,
- u32,
- u64,
- u8,
- usize,
- {<T: ?Sized>} crate::sync::Arc<T> {where crate::sync::Arc<T>: fmt::Display},
- {<T: ?Sized>} crate::sync::UniqueArc<T> {where crate::sync::UniqueArc<T>: fmt::Display},
+);
diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..edc37c220a89 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0
+use proc_macro::{Ident, TokenStream, TokenTree}; +use std::collections::BTreeSet;
+/// Please see [`crate::fmt`] for documentation. +pub(crate) fn fmt(input: TokenStream) -> TokenStream {
- let mut input = input.into_iter();
- let first_opt = input.next();
- let first_owned_str;
- let mut names = BTreeSet::new();
- let first_lit = {
let Some((mut first_str, first_lit)) = (match first_opt.as_ref() {
Some(TokenTree::Literal(first_lit)) => {
first_owned_str = first_lit.to_string();
Some(first_owned_str.as_str()).and_then(|first| {
let first = first.strip_prefix('"')?;
let first = first.strip_suffix('"')?;
Some((first, first_lit))
You're only using first_lit to get the span later, so why not just get the span directly here?
Good point. I was probably using it for more stuff in an earlier iteration.
})
}
_ => None,
}) else {
return first_opt.into_iter().chain(input).collect();
};
while let Some((_, rest)) = first_str.split_once('{') {
Let's put a comment above this loop mentioning [1] and saying that it parses the identifiers from the format arguments.
👍
first_str = rest;
if let Some(rest) = first_str.strip_prefix('{') {
first_str = rest;
continue;
}
if let Some((name, rest)) = first_str.split_once('}') {
first_str = rest;
let name = name.split_once(':').map_or(name, |(name, _)| name);
if !name.is_empty() && !name.chars().all(|c| c.is_ascii_digit()) {
names.insert(name);
}
}
}
first_lit
- };
- let first_span = first_lit.span();
- let adapter = quote_spanned! {
first_span => ::kernel::fmt::Adapter
- };
I think we should follow the formatting convention from the quote crate:
let adapter = quote_spanned!(first_span=> ::kernel::fmt::Adapter);
Sure.
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut flush = |args: &mut TokenStream, current: &mut TokenStream| {
You don't need to pass `args` as a closure argument, since you always call it with `&mut args`.
This doesn't work because of the borrow checker. If I wrote what you suggest, then `args` is mutably borrowed by the closure, which prohibits the mutable borrow needed for the .extend() call here:
for tt in input { match &tt { TokenTree::Punct(p) if p.as_char() == ',' => { flush(&mut args, &mut current); &mut args } _ => &mut current, } .extend([tt]); }
let current = std::mem::take(current);
if !current.is_empty() {
let (lhs, rhs) = (|| {
let mut current = current.into_iter();
let mut acc = TokenStream::new();
while let Some(tt) = current.next() {
// Split on `=` only once to handle cases like `a = b = c`.
if matches!(&tt, TokenTree::Punct(p) if p.as_char() == '=') {
names.remove(acc.to_string().as_str());
// Include the `=` itself to keep the handling below uniform.
acc.extend([tt]);
return (Some(acc), current.collect::<TokenStream>());
}
acc.extend([tt]);
}
(None, acc)
})();
args.extend(quote_spanned! {
first_span => #lhs #adapter(&#rhs)
});
}
};
let mut current = TokenStream::new();
Define this before the closure, then you don't need to pass it as an argument.
Same reason as above. Borrow checker says no.
Cheers, Benno
for tt in input {
match &tt {
TokenTree::Punct(p) if p.as_char() == ',' => {
flush(&mut args, &mut current);
&mut args
}
_ => &mut current,
}
.extend([tt]);
}
flush(&mut args, &mut current);
- }
- for name in names {
let name = Ident::new(name, first_span);
args.extend(quote_spanned! {
first_span => , #name = #adapter(&#name)
});
- }
- quote_spanned! {
first_span => ::core::format_args!(#args)
- }
+}
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.
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.
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.
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?
+/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T);
+macro_rules! impl_fmt_adapter_forward {
- ($($trait:ident),* $(,)?) => {
$(
impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
fmt::$trait::fmt(t, f)
}
}
)*
- };
+}
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
And what is the benefit here?
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut flush = |args: &mut TokenStream, current: &mut TokenStream| {
You don't need to pass `args` as a closure argument, since you always call it with `&mut args`.
This doesn't work because of the borrow checker. If I wrote what you suggest, then `args` is mutably borrowed by the closure, which prohibits the mutable borrow needed for the .extend() call here:
Ahh right... Well then it's fine.
--- Cheers, Benno
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?
+/// Internal adapter used to route allow implementations of formatting traits for foreign types. +/// +/// It is inserted automatically by the [`fmt!`] macro and is not meant to be used directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +#[doc(hidden)] +pub struct Adapter<T>(pub T);
+macro_rules! impl_fmt_adapter_forward {
- ($($trait:ident),* $(,)?) => {
$(
impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
fmt::$trait::fmt(t, f)
}
}
)*
- };
+}
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
And what is the benefit here?
In general the benefit is that the method does not ignore some portion of `Self`. A method that uses `self.0` would not provoke a compiler error in case another field is added, while this form would.
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut flush = |args: &mut TokenStream, current: &mut TokenStream| {
You don't need to pass `args` as a closure argument, since you always call it with `&mut args`.
This doesn't work because of the borrow checker. If I wrote what you suggest, then `args` is mutably borrowed by the closure, which prohibits the mutable borrow needed for the .extend() call here:
Ahh right... Well then it's fine.
Cheers, Benno
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.
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein 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 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!`.
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.
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.
See above.
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
And what is the benefit here?
In general the benefit is that the method does not ignore some portion of `Self`. A method that uses `self.0` would not provoke a compiler error in case another field is added, while this form would.
Yeah, but why would that change happen here? And even if it got another field, why would that invalidate the impl of `fn fmt`?
--- Cheers, Benno
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin lossin@kernel.org wrote:
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein 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 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
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 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.
See above.
+impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary, Pointer, LowerExp, UpperExp);
+/// A copy of [`fmt::Display`] that allows us to implement it for foreign types. +/// +/// Types should implement this trait rather than [`fmt::Display`]. Together with the [`Adapter`] +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g. types from core) which do +/// not implement [`fmt::Display`] directly. +/// +/// [`fmt!`]: crate::prelude::fmt! +pub trait Display {
- /// Same as [`fmt::Display::fmt`].
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
+}
+impl<T: ?Sized + Display> Display for &T {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Display::fmt(*self, f)
- }
+}
+impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;
Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
And what is the benefit here?
In general the benefit is that the method does not ignore some portion of `Self`. A method that uses `self.0` would not provoke a compiler error in case another field is added, while this form would.
Yeah, but why would that change happen here? And even if it got another field, why would that invalidate the impl of `fn fmt`?
I don't know, but I would rather force a person to make that decision when they add another field rather than assume that such an addition wouldn't require changes here.
On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein tamird@gmail.com wrote:
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin lossin@kernel.org wrote:
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein 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 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
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.
On Fri Jul 4, 2025 at 1:23 AM CEST, Tamir Duberstein wrote:
On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein tamird@gmail.com wrote:
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin lossin@kernel.org wrote:
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
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.
So here is what we can do without duplicating the logic, though it requires multiple cycles:
1. We merge the two `fmt.rs` files & each subsystem merges an implementation of `kernel::fmt::Display` for their types, but keeps the `core::fmt::Display` impl around. 2. After all subsystems have merged the previous step, we change the implementations of `print_macro!` to use `fmt!` instead of `format_args!`. 3. We remove all occurrences of `core::fmt` (& replace them with `kernel::fmt`), removing the `core::fmt::Display` impls.
--- Cheers, Benno
On Fri, Jul 4, 2025 at 6:09 AM Benno Lossin lossin@kernel.org wrote:
On Fri Jul 4, 2025 at 1:23 AM CEST, Tamir Duberstein wrote:
On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein tamird@gmail.com wrote:
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin lossin@kernel.org wrote:
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
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.
So here is what we can do without duplicating the logic, though it requires multiple cycles:
- We merge the two `fmt.rs` files & each subsystem merges an implementation of `kernel::fmt::Display` for their types, but keeps the `core::fmt::Display` impl around.
- After all subsystems have merged the previous step, we change the implementations of `print_macro!` to use `fmt!` instead of `format_args!`.
- We remove all occurrences of `core::fmt` (& replace them with `kernel::fmt`), removing the `core::fmt::Display` impls.
That would probably work. We will probably see regressions because we can't just replace `core::fmt` imports with `kernel::fmt`, so new code may appear that uses the former.
I think this discussion would be productive on the next spin. The changes in other subsystems are now almost entirely changing of import paths -- perhaps that would be sufficiently uncontroversial for folks to give their Acked-bys.
On Fri, Jul 4, 2025 at 1:59 PM Tamir Duberstein tamird@gmail.com wrote:
That would probably work. We will probably see regressions because we can't just replace `core::fmt` imports with `kernel::fmt`, so new code may appear that uses the former.
That is fine -- it happens all the time with this sort of approach.
Cheers, Miguel
On Fri, Jul 4, 2025 at 8:15 AM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Fri, Jul 4, 2025 at 1:59 PM Tamir Duberstein tamird@gmail.com wrote:
That would probably work. We will probably see regressions because we can't just replace `core::fmt` imports with `kernel::fmt`, so new code may appear that uses the former.
That is fine -- it happens all the time with this sort of approach.
OK, with all the splitting requested, this comes out to ~54 patches. I'll send the first bit (which can go in cycle 0) as v14.
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).
This sounds like an abstraction problem. As a developer, i just want an API to print stuff. I don't care about what happens underneath.
Could you add an implementation of the API which uses core:fmt underneath. Get that merged. You can then convert each subsystem one by one to use the new API. Since all you are changing is the API, not the implementation, there is no compatibility issues. Then, once all users are converted to the API, you can have one patch which flips the implementation from core:fmt to kernel:fmt. It might take you three kernel cycles to get this done, but that is relatively fast for a tree wide change, which sometimes takes years.
Andrew
On Fri Jul 4, 2025 at 12:41 AM CEST, Tamir Duberstein wrote:
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin lossin@kernel.org wrote:
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein 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: > +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> { > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > + let Self(t) = self; > + Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field. With `self.0` I don't see that.
And what is the benefit here?
In general the benefit is that the method does not ignore some portion of `Self`. A method that uses `self.0` would not provoke a compiler error in case another field is added, while this form would.
Yeah, but why would that change happen here? And even if it got another field, why would that invalidate the impl of `fn fmt`?
I don't know, but I would rather force a person to make that decision when they add another field rather than assume that such an addition wouldn't require changes here.
I don't think so. If this were in another file, then destructuring might make sense if the struct could conceivably get more fields in the future **and** it if the other file relied on there only being one field (or if it *had* to be changed when there was a field added). This isn't the case here so it's just unnecessary noise.
--- Cheers, Benno
On Thu, Jul 03, 2025 at 02:55:30PM -0400, Tamir Duberstein 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.
I often tell kernel newbies:
Lots of small patches which are obviously correct.
A small patch tends to be more obviously correct than a big patch. The commit message is more focused and helpful because it refers to a small chunk of code. Because the commit message is more focused, it can answer questions reviewers might ask, before they ask them. If i can spend 60 seconds looking at a patch and decide it looks correct, i'm more likely to actually look at it and give a reviewed by. If i need to find 10 minutes, it is going to get put off for a later time. Many reviewers just have a few minutes here, a few there, slotted into time between other tasks, while drinking coffee, etc.
Andrew
On Thu, Jul 3, 2025 at 11:28 PM Andrew Lunn andrew@lunn.ch wrote:
A small patch tends to be more obviously correct than a big patch. The commit message is more focused and helpful because it refers to a small chunk of code. Because the commit message is more focused, it can answer questions reviewers might ask, before they ask them. If i
Yeah, also better for smaller reverts, as well as typically easier to backport if needed, etc.
Cheers, Miguel
On Thu, Jul 3, 2025 at 5:38 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Thu, Jul 3, 2025 at 11:28 PM Andrew Lunn andrew@lunn.ch wrote:
A small patch tends to be more obviously correct than a big patch. The commit message is more focused and helpful because it refers to a small chunk of code. Because the commit message is more focused, it can answer questions reviewers might ask, before they ask them. If i
Yeah, also better for smaller reverts, as well as typically easier to backport if needed, etc.
I appreciate that this advice is well-intentioned, thank you. I agree that all things being equal, small changes are better. In this particular case, there are specific downsides to splitting for its own sake which I tried to explain in previous replies: splitting the proc macro from the rest of the machinery risks forcing a reviewer to assess a chunk of code without seeing how it is used; in my experience this limits the scope of the review. Splitting by subsystem has other downsides, which I attempted to enumerate in my reply to Benno in the other fork of this discussion (let's discuss those there, please).
There's also a tactical question about splitting by subsystem: are there any tools that would assist in doing this, or is it a matter of manually consulting MAINTAINERS to figure out file groupings?
There's also a tactical question about splitting by subsystem: are there any tools that would assist in doing this, or is it a matter of manually consulting MAINTAINERS to figure out file groupings?
You can run ./scripts/get_maintainer -f path/to/file.c
and it will give you the Maintainers for that file. From that you can imply the subsystem.
It might be possible to do one tree wide patchset, since Rust is still small at the moment. But you will need to get Reviewed-by: or Acked-by: from each subsystem Maintainer for the patches. That is not always easy, since some subsystems have CI systems, and will want the patch to pass their CI tests before giving an Reviewed-by.
Andrew
On Fri, Jul 4, 2025 at 12:46 AM Tamir Duberstein tamird@gmail.com wrote:
There's also a tactical question about splitting by subsystem: are there any tools that would assist in doing this, or is it a matter of manually consulting MAINTAINERS to figure out file groupings?
As Andrew mentioned, you can use that script, though I recommend not fully/blindly trusting it.
Sometimes you will want to adjust things, e.g. sometimes things may be related even if in a couple different `MAINTAINERS` entries, or you may want to adjust the flags the script provides to filter, or you may want to check `git log --no-merges` to see who is recently applying patches related to that area, etc.
It is essentially the same process as when you send patches.
For instance, taking the diffstat above, and ignoring contents (i.e. assuming all lines could just be freely split and without considering other splits discussed to make the patches smaller first and reducing the flag day changes), I could have done something like this:
drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +-
drivers/gpu/nova-core/gpu.rs | 4 +-
rust/kernel/device.rs | 2 +-
rust/kernel/kunit.rs | 6 +--
rust/kernel/seq_file.rs | 2 +-
rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + rust/kernel/prelude.rs | 3 +- rust/kernel/print.rs | 4 +- 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 +-
And then those long lines may hint that it may make sense to split the smaller tweaks in the last group into their own patch, so that it mirrors what is done for the other smaller groups. Thus possibly leaving the feature being added into its own patch, which would be the biggest and the one that would take some discussion. And the others would be the small ones that are easy to Acked-by or Reviewed-by or simply take (if independently possible) by other maintainers.
And so on -- again, this is speaking generally, and it is just a dummy example, not intended to say anything about the current patch. And sometimes things may not make sense to split too far, and it can be more annoying than it is worth for everyone involved, e.g. when we are talking about trivial conversions that could take 50+ patches that could be automated instead and then applied by a single maintainer.
So it is a balance.
Cheers, Miguel
On Thu, Jul 3, 2025 at 3:56 PM Tamir Duberstein tamird@gmail.com wrote:
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?
By the way, if we are talking about splitting, it is easier to land patches that can go independently into different subsystems and avoiding flag day changes (or making those as small as possible), i.e. ideally being able to land big changes across more than one kernel cycle.
Cheers, Miguel
On Thu, Jul 3, 2025 at 12:26 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Thu, Jul 3, 2025 at 3:56 PM Tamir Duberstein tamird@gmail.com wrote:
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?
By the way, if we are talking about splitting, it is easier to land patches that can go independently into different subsystems and avoiding flag day changes (or making those as small as possible), i.e. ideally being able to land big changes across more than one kernel cycle.
Understood, though in this case I don't see how it's workable. The formatting macros can either wrap in fmt::Adapter (and thus require kernel::fmt::Display) or not (and thus require core::fmt::Display), but I don't see how they can work in a mixed world. We can't have half the subsystems implement core::fmt::Display and the other half implement kernel::fmt::Display.
`kernel::ffi::CStr` was introduced in commit d126d2380131 ("rust: str: add `CStr` type") in November 2022 as an upstreaming of earlier work that was done in May 2021[0]. That earlier work, having predated the inclusion of `CStr` in `core`, largely duplicated the implementation of `std::ffi::CStr`.
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64 in September 2022. Hence replace `kernel::str::CStr` with `core::ffi::CStr` to reduce our custom code footprint, and retain needed custom functionality through an extension trait.
C-String literals were added in Rust 1.77, while our MSRV is 1.78. Thus opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary or where existing code triggered clippy lints; the rest will be done in a later commit.
Link: https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c... [0] Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/auxiliary.rs | 4 +- rust/kernel/configfs.rs | 4 +- rust/kernel/cpufreq.rs | 2 +- rust/kernel/device.rs | 4 +- rust/kernel/drm/device.rs | 4 +- rust/kernel/error.rs | 4 +- rust/kernel/firmware.rs | 11 +- rust/kernel/kunit.rs | 6 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/net/phy.rs | 2 +- rust/kernel/of.rs | 2 +- rust/kernel/prelude.rs | 5 +- rust/kernel/seq_file.rs | 4 +- rust/kernel/str.rs | 394 +++++++++++----------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- samples/rust/rust_configfs.rs | 2 +- 19 files changed, 140 insertions(+), 318 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index 6b59d19ab631..fea062cc0383 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -948,7 +948,7 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) { // nul-terminated string. let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) }; let segments = &[ - &Segment::Binary(url_cstr.as_bytes()), + &Segment::Binary(url_cstr.to_bytes()), &Segment::Numeric(&data_slice[0..data_len]), ]; match EncodedMsg::new(segments, tmp_slice) { diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index d2cfe1eeefb6..89d961407adb 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -111,8 +111,8 @@ macro_rules! module_auxiliary_driver { impl DeviceId { /// Create a new [`DeviceId`] from name. pub const fn new(modname: &'static CStr, name: &'static CStr) -> Self { - let name = name.as_bytes_with_nul(); - let modname = modname.as_bytes_with_nul(); + let name = name.to_bytes_with_nul(); + let modname = modname.to_bytes_with_nul();
// TODO: Replace with `bindings::auxiliary_device_id::default()` once stabilized for // `const`. diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index aafef70b7177..d4797c41ba77 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -263,7 +263,7 @@ pub fn new( try_pin_init!(Self { group <- pin_init::zeroed().chain(|v: &mut Opaquebindings::config_group| { let place = v.get(); - let name = name.as_bytes_with_nul().as_ptr(); + let name = name.to_bytes_with_nul().as_ptr(); // SAFETY: It is safe to initialize a group once it has been zeroed. unsafe { bindings::config_group_init_type_name(place, name.cast(), item_type.as_ptr()) @@ -613,7 +613,7 @@ impl<const ID: u64, O, Data> Attribute<ID, O, Data> pub const fn new(name: &'static CStr) -> Self { Self { attribute: Opaque::new(bindings::configfs_attribute { - ca_name: name.as_char_ptr(), + ca_name: crate::str::as_char_ptr_in_const_context(name), ca_owner: core::ptr::null_mut(), ca_mode: 0o660, show: Some(Self::show), diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs index e8d231971276..71d601f7c261 100644 --- a/rust/kernel/cpufreq.rs +++ b/rust/kernel/cpufreq.rs @@ -1018,7 +1018,7 @@ impl<T: Driver> Registration<T> { };
const fn copy_name(name: &'static CStr) -> [c_char; CPUFREQ_NAME_LEN] { - let src = name.as_bytes_with_nul(); + let src = name.to_bytes_with_nul(); let mut dst = [0; CPUFREQ_NAME_LEN];
build_assert!(src.len() <= CPUFREQ_NAME_LEN); diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index 86b945576780..8cc2d818dca5 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -12,7 +12,7 @@ use core::{fmt, marker::PhantomData, ptr};
#[cfg(CONFIG_PRINTK)] -use crate::c_str; +use crate::str::CStrExt as _;
/// A reference-counted device. /// @@ -197,7 +197,7 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) { bindings::_dev_printk( klevel.as_ptr().cast::crate::ffi::c_char(), self.as_raw(), - c_str!("%pA").as_char_ptr(), + c"%pA".as_char_ptr(), core::ptr::from_ref(&msg).cast::crate::ffi::c_void(), ) }; diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs index b7ee3c464a12..998b942b6dd8 100644 --- a/rust/kernel/drm/device.rs +++ b/rust/kernel/drm/device.rs @@ -83,8 +83,8 @@ impl<T: drm::Driver> Device<T> { major: T::INFO.major, minor: T::INFO.minor, patchlevel: T::INFO.patchlevel, - name: T::INFO.name.as_char_ptr().cast_mut(), - desc: T::INFO.desc.as_char_ptr().cast_mut(), + name: crate::str::as_char_ptr_in_const_context(T::INFO.name).cast_mut(), + desc: crate::str::as_char_ptr_in_const_context(T::INFO.desc).cast_mut(),
driver_features: drm::driver::FEAT_GEM, ioctls: T::IOCTLS.as_ptr(), diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 6277af1c1baa..94ef4648827e 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -164,6 +164,8 @@ pub fn name(&self) -> Option<&'static CStr> { if ptr.is_null() { None } else { + use crate::str::CStrExt as _; + // SAFETY: The string returned by `errname` is static and `NUL`-terminated. Some(unsafe { CStr::from_char_ptr(ptr) }) } @@ -188,7 +190,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { Some(name) => f .debug_tuple( // SAFETY: These strings are ASCII-only. - unsafe { core::str::from_utf8_unchecked(name) }, + unsafe { core::str::from_utf8_unchecked(name.to_bytes()) }, ) .finish(), } diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index be684e860ed2..4adcf39b475e 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -4,7 +4,14 @@ //! //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
-use crate::{bindings, device::Device, error::Error, error::Result, ffi, str::CStr}; +use crate::{ + bindings, + device::Device, + error::Error, + error::Result, + ffi, + str::{CStr, CStrExt as _}, +}; use core::ptr::NonNull;
/// # Invariants @@ -291,7 +298,7 @@ const fn push_module_name(self) -> Self { let module_name = this.module_name;
if !this.module_name.is_empty() { - this = this.push_internal(module_name.as_bytes_with_nul()); + this = this.push_internal(module_name.to_bytes_with_nul());
if N != 0 { // Re-use the space taken by the NULL terminator and swap it with the '.' separator. diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 3539edbceaf5..6f80dc673974 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -102,12 +102,12 @@ unsafe impl Sync for Location {} unsafe impl Sync for UnaryAssert {}
static LOCATION: Location = Location($crate::bindings::kunit_loc { - file: FILE.as_char_ptr(), + file: $crate::str::as_char_ptr_in_const_context(FILE), line: LINE, }); static ASSERTION: UnaryAssert = UnaryAssert($crate::bindings::kunit_unary_assert { assert: $crate::bindings::kunit_assert {}, - condition: CONDITION.as_char_ptr(), + condition: $crate::str::as_char_ptr_in_const_context(CONDITION), expected_true: true, });
@@ -202,7 +202,7 @@ pub const fn kunit_case( ) -> kernel::bindings::kunit_case { kernel::bindings::kunit_case { run_case: Some(run_case), - name: name.as_char_ptr(), + name: kernel::str::as_char_ptr_in_const_context(name), attr: kernel::bindings::kunit_attributes { speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL, }, diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index 288f40e79906..b5b2e3cc158f 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -35,7 +35,7 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice { // SAFETY: All zeros is valid for this C type. let mut result: bindings::miscdevice = unsafe { MaybeUninit::zeroed().assume_init() }; result.minor = bindings::MISC_DYNAMIC_MINOR as ffi::c_int; - result.name = self.name.as_char_ptr(); + result.name = crate::str::as_char_ptr_in_const_context(self.name); result.fops = MiscdeviceVTable::<T>::build(); result } diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 65ac4d59ad77..c420e5ecab4b 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -505,7 +505,7 @@ unsafe impl Sync for DriverVTable {} pub const fn create_phy_driver<T: Driver>() -> DriverVTable { // INVARIANT: All the fields of `struct phy_driver` are initialized properly. DriverVTable(Opaque::new(bindings::phy_driver { - name: T::NAME.as_char_ptr().cast_mut(), + name: crate::str::as_char_ptr_in_const_context(T::NAME).cast_mut(), flags: T::FLAGS, phy_id: T::PHY_DEVICE_ID.id, phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index 40d1bd13682c..5cf50979c1e8 100644 --- a/rust/kernel/of.rs +++ b/rust/kernel/of.rs @@ -29,7 +29,7 @@ fn index(&self) -> usize { impl DeviceId { /// Create a new device id from an OF 'compatible' string. pub const fn new(compatible: &'static CStr) -> Self { - let src = compatible.as_bytes_with_nul(); + let src = compatible.to_bytes_with_nul(); // Replace with `bindings::of_device_id::default()` once stabilized for `const`. // SAFETY: FFI type is valid to be zero-initialized. let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() }; diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index 41cebd906c4c..244b660fa835 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -40,7 +40,10 @@
pub use super::error::{code::*, Error, Result};
-pub use super::{str::CStr, ThisModule}; +pub use super::{ + str::{CStr, CStrExt as _}, + ThisModule, +};
pub use super::init::InPlaceInit;
diff --git a/rust/kernel/seq_file.rs b/rust/kernel/seq_file.rs index 05c9b7bec727..c26e8507863a 100644 --- a/rust/kernel/seq_file.rs +++ b/rust/kernel/seq_file.rs @@ -4,7 +4,7 @@ //! //! C header: [`include/linux/seq_file.h`](srctree/include/linux/seq_file.h)
-use crate::{bindings, c_str, types::NotThreadSafe, types::Opaque}; +use crate::{bindings, str::CStrExt as _, types::NotThreadSafe, types::Opaque};
/// A utility for generating the contents of a seq file. #[repr(transparent)] @@ -36,7 +36,7 @@ pub fn call_printf(&self, args: core::fmt::Arguments<'_>) { unsafe { bindings::seq_printf( self.inner.get(), - c_str!("%pA").as_char_ptr(), + c"%pA".as_char_ptr(), core::ptr::from_ref(&args).cast::crate::ffi::c_void(), ); } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index d3fa4b703d35..5c365646c0e1 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -4,7 +4,7 @@
use crate::alloc::{flags::*, AllocError, KVec}; use core::fmt::{self, Write}; -use core::ops::{self, Deref, DerefMut, Index}; +use core::ops::{Deref, DerefMut, Index};
use crate::prelude::*;
@@ -57,11 +57,11 @@ impl crate::fmt::Display for BStr { /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}}; /// let ascii = b_str!("Hello, BStr!"); /// let s = CString::try_from_fmt(fmt!("{}", ascii))?; - /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes()); + /// assert_eq!(s.to_bytes(), "Hello, BStr!".as_bytes()); /// /// let non_ascii = b_str!("🦀"); /// let s = CString::try_from_fmt(fmt!("{}", non_ascii))?; - /// assert_eq!(s.as_bytes(), "\xf0\x9f\xa6\x80".as_bytes()); + /// assert_eq!(s.to_bytes(), "\xf0\x9f\xa6\x80".as_bytes()); /// # Ok::<(), kernel::error::Error>(()) /// ``` fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -89,11 +89,11 @@ impl fmt::Debug for BStr { /// // Embedded double quotes are escaped. /// let ascii = b_str!("Hello, "BStr"!"); /// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?; - /// assert_eq!(s.as_bytes(), ""Hello, \"BStr\"!"".as_bytes()); + /// assert_eq!(s.to_bytes(), ""Hello, \"BStr\"!"".as_bytes()); /// /// let non_ascii = b_str!("😺"); /// let s = CString::try_from_fmt(fmt!("{:?}", non_ascii))?; - /// assert_eq!(s.as_bytes(), ""\xf0\x9f\x98\xba"".as_bytes()); + /// assert_eq!(s.to_bytes(), ""\xf0\x9f\x98\xba"".as_bytes()); /// # Ok::<(), kernel::error::Error>(()) /// ``` fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -175,55 +175,19 @@ macro_rules! b_str { }}; }
-/// Possible errors when using conversion functions in [`CStr`]. -#[derive(Debug, Clone, Copy)] -pub enum CStrConvertError { - /// Supplied bytes contain an interior `NUL`. - InteriorNul, +pub use core::ffi::CStr;
- /// Supplied bytes are not terminated by `NUL`. - NotNulTerminated, +/// Returns a C pointer to the string. +// It is a free function rather than a method on an extension trait because: +// +// - error[E0379]: functions in trait impls cannot be declared const +#[inline] +pub const fn as_char_ptr_in_const_context(c_str: &CStr) -> *const crate::ffi::c_char { + c_str.as_ptr().cast() }
-impl From<CStrConvertError> for Error { - #[inline] - fn from(_: CStrConvertError) -> Error { - EINVAL - } -} - -/// A string that is guaranteed to have exactly one `NUL` byte, which is at the -/// end. -/// -/// Used for interoperability with kernel APIs that take C strings. -#[repr(transparent)] -pub struct CStr([u8]); - -impl CStr { - /// Returns the length of this string excluding `NUL`. - #[inline] - pub const fn len(&self) -> usize { - self.len_with_nul() - 1 - } - - /// Returns the length of this string with `NUL`. - #[inline] - pub const fn len_with_nul(&self) -> usize { - if self.0.is_empty() { - // SAFETY: This is one of the invariant of `CStr`. - // We add a `unreachable_unchecked` here to hint the optimizer that - // the value returned from this function is non-zero. - unsafe { core::hint::unreachable_unchecked() }; - } - self.0.len() - } - - /// Returns `true` if the string only includes `NUL`. - #[inline] - pub const fn is_empty(&self) -> bool { - self.len() == 0 - } - +/// Extensions to [`CStr`]. +pub trait CStrExt { /// Wraps a raw C string pointer. /// /// # Safety @@ -231,54 +195,9 @@ pub const fn is_empty(&self) -> bool { /// `ptr` must be a valid pointer to a `NUL`-terminated C string, and it must /// last at least `'a`. When `CStr` is alive, the memory pointed by `ptr` /// must not be mutated. - #[inline] - pub unsafe fn from_char_ptr<'a>(ptr: *const crate::ffi::c_char) -> &'a Self { - // SAFETY: The safety precondition guarantees `ptr` is a valid pointer - // to a `NUL`-terminated C string. - let len = unsafe { bindings::strlen(ptr) } + 1; - // SAFETY: Lifetime guaranteed by the safety precondition. - let bytes = unsafe { core::slice::from_raw_parts(ptr.cast(), len) }; - // SAFETY: As `len` is returned by `strlen`, `bytes` does not contain interior `NUL`. - // As we have added 1 to `len`, the last byte is known to be `NUL`. - unsafe { Self::from_bytes_with_nul_unchecked(bytes) } - } - - /// Creates a [`CStr`] from a `[u8]`. - /// - /// The provided slice must be `NUL`-terminated, does not contain any - /// interior `NUL` bytes. - pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError> { - if bytes.is_empty() { - return Err(CStrConvertError::NotNulTerminated); - } - if bytes[bytes.len() - 1] != 0 { - return Err(CStrConvertError::NotNulTerminated); - } - let mut i = 0; - // `i + 1 < bytes.len()` allows LLVM to optimize away bounds checking, - // while it couldn't optimize away bounds checks for `i < bytes.len() - 1`. - while i + 1 < bytes.len() { - if bytes[i] == 0 { - return Err(CStrConvertError::InteriorNul); - } - i += 1; - } - // SAFETY: We just checked that all properties hold. - Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) }) - } - - /// Creates a [`CStr`] from a `[u8]` without performing any additional - /// checks. - /// - /// # Safety - /// - /// `bytes` *must* end with a `NUL` byte, and should only have a single - /// `NUL` byte (or the string will be truncated). - #[inline] - pub const unsafe fn from_bytes_with_nul_unchecked(bytes: &[u8]) -> &CStr { - // SAFETY: Properties of `bytes` guaranteed by the safety precondition. - unsafe { core::mem::transmute(bytes) } - } + // This function exists to paper over the fact that `CStr::from_ptr` takes a `*const + // core::ffi::c_char` rather than a `*const crate::ffi::c_char`. + unsafe fn from_char_ptr<'a>(ptr: *const crate::ffi::c_char) -> &'a Self;
/// Creates a mutable [`CStr`] from a `[u8]` without performing any /// additional checks. @@ -287,77 +206,16 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError /// /// `bytes` *must* end with a `NUL` byte, and should only have a single /// `NUL` byte (or the string will be truncated). - #[inline] - pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { - // SAFETY: Properties of `bytes` guaranteed by the safety precondition. - unsafe { &mut *(core::ptr::from_mut(bytes) as *mut CStr) } - } + unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut Self;
/// Returns a C pointer to the string. - #[inline] - pub const fn as_char_ptr(&self) -> *const crate::ffi::c_char { - self.0.as_ptr() - } - - /// Convert the string to a byte slice without the trailing `NUL` byte. - #[inline] - pub fn as_bytes(&self) -> &[u8] { - &self.0[..self.len()] - } - - /// Convert the string to a byte slice containing the trailing `NUL` byte. - #[inline] - pub const fn as_bytes_with_nul(&self) -> &[u8] { - &self.0 - } - - /// Yields a [`&str`] slice if the [`CStr`] contains valid UTF-8. - /// - /// If the contents of the [`CStr`] are valid UTF-8 data, this - /// function will return the corresponding [`&str`] slice. Otherwise, - /// it will return an error with details of where UTF-8 validation failed. - /// - /// # Examples - /// - /// ``` - /// # use kernel::str::CStr; - /// let cstr = CStr::from_bytes_with_nul(b"foo\0")?; - /// assert_eq!(cstr.to_str(), Ok("foo")); - /// # Ok::<(), kernel::error::Error>(()) - /// ``` - #[inline] - pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> { - core::str::from_utf8(self.as_bytes()) - } - - /// Unsafely convert this [`CStr`] into a [`&str`], without checking for - /// valid UTF-8. - /// - /// # Safety - /// - /// The contents must be valid UTF-8. - /// - /// # Examples - /// - /// ``` - /// # use kernel::c_str; - /// # use kernel::str::CStr; - /// let bar = c_str!("ツ"); - /// // SAFETY: String literals are guaranteed to be valid UTF-8 - /// // by the Rust compiler. - /// assert_eq!(unsafe { bar.as_str_unchecked() }, "ツ"); - /// ``` - #[inline] - pub unsafe fn as_str_unchecked(&self) -> &str { - // SAFETY: TODO. - unsafe { core::str::from_utf8_unchecked(self.as_bytes()) } - } + // This function exists to paper over the fact that `CStr::as_ptr` returns a `*const + // core::ffi::c_char` rather than a `*const crate::ffi::c_char`. + fn as_char_ptr(&self) -> *const crate::ffi::c_char;
/// Convert this [`CStr`] into a [`CString`] by allocating memory and /// copying over the string data. - pub fn to_cstring(&self) -> Result<CString, AllocError> { - CString::try_from(self) - } + fn to_cstring(&self) -> Result<CString, AllocError>;
/// Converts this [`CStr`] to its ASCII lower case equivalent in-place. /// @@ -368,11 +226,7 @@ pub fn to_cstring(&self) -> Result<CString, AllocError> { /// [`to_ascii_lowercase()`]. /// /// [`to_ascii_lowercase()`]: #method.to_ascii_lowercase - pub fn make_ascii_lowercase(&mut self) { - // INVARIANT: This doesn't introduce or remove NUL bytes in the C - // string. - self.0.make_ascii_lowercase(); - } + fn make_ascii_lowercase(&mut self);
/// Converts this [`CStr`] to its ASCII upper case equivalent in-place. /// @@ -383,11 +237,7 @@ pub fn make_ascii_lowercase(&mut self) { /// [`to_ascii_uppercase()`]. /// /// [`to_ascii_uppercase()`]: #method.to_ascii_uppercase - pub fn make_ascii_uppercase(&mut self) { - // INVARIANT: This doesn't introduce or remove NUL bytes in the C - // string. - self.0.make_ascii_uppercase(); - } + fn make_ascii_uppercase(&mut self);
/// Returns a copy of this [`CString`] where each character is mapped to its /// ASCII lower case equivalent. @@ -398,13 +248,7 @@ pub fn make_ascii_uppercase(&mut self) { /// To lowercase the value in-place, use [`make_ascii_lowercase`]. /// /// [`make_ascii_lowercase`]: str::make_ascii_lowercase - pub fn to_ascii_lowercase(&self) -> Result<CString, AllocError> { - let mut s = self.to_cstring()?; - - s.make_ascii_lowercase(); - - Ok(s) - } + fn to_ascii_lowercase(&self) -> Result<CString, AllocError>;
/// Returns a copy of this [`CString`] where each character is mapped to its /// ASCII upper case equivalent. @@ -415,13 +259,7 @@ pub fn to_ascii_lowercase(&self) -> Result<CString, AllocError> { /// To uppercase the value in-place, use [`make_ascii_uppercase`]. /// /// [`make_ascii_uppercase`]: str::make_ascii_uppercase - pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> { - let mut s = self.to_cstring()?; - - s.make_ascii_uppercase(); - - Ok(s) - } + fn to_ascii_uppercase(&self) -> Result<CString, AllocError>; }
impl crate::fmt::Display for CStr { @@ -434,15 +272,15 @@ impl crate::fmt::Display for CStr { /// # use kernel::str::CString; /// let penguin = c_str!("🐧"); /// let s = CString::try_from_fmt(fmt!("{}", penguin))?; - /// assert_eq!(s.as_bytes_with_nul(), "\xf0\x9f\x90\xa7\0".as_bytes()); + /// assert_eq!(s.to_bytes_with_nul(), "\xf0\x9f\x90\xa7\0".as_bytes()); /// /// let ascii = c_str!("so "cool""); /// let s = CString::try_from_fmt(fmt!("{}", ascii))?; - /// assert_eq!(s.as_bytes_with_nul(), "so "cool"\0".as_bytes()); + /// assert_eq!(s.to_bytes_with_nul(), "so "cool"\0".as_bytes()); /// # Ok::<(), kernel::error::Error>(()) /// ``` fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - for &c in self.as_bytes() { + for &c in self.to_bytes() { if (0x20..0x7f).contains(&c) { // Printable character. f.write_char(c as char)?; @@ -454,98 +292,75 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { } }
-impl fmt::Debug for CStr { - /// Formats printable ASCII characters with a double quote on either end, escaping the rest. - /// - /// ``` - /// # use kernel::c_str; - /// # use kernel::prelude::fmt; - /// # use kernel::str::CStr; - /// # use kernel::str::CString; - /// let penguin = c_str!("🐧"); - /// let s = CString::try_from_fmt(fmt!("{:?}", penguin))?; - /// assert_eq!(s.as_bytes_with_nul(), ""\xf0\x9f\x90\xa7"\0".as_bytes()); - /// - /// // Embedded double quotes are escaped. - /// let ascii = c_str!("so "cool""); - /// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?; - /// assert_eq!(s.as_bytes_with_nul(), ""so \"cool\""\0".as_bytes()); - /// # Ok::<(), kernel::error::Error>(()) - /// ``` - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(""")?; - for &c in self.as_bytes() { - match c { - // Printable characters. - b'"' => f.write_str("\"")?, - 0x20..=0x7e => f.write_char(c as char)?, - _ => write!(f, "\x{c:02x}")?, - } - } - f.write_str(""") - } +/// Converts a mutable C string to a mutable byte slice. +/// +/// # Safety +/// +/// The caller must ensure that the slice ends in a NUL byte and contains no other NUL bytes before +/// the borrow ends and the underlying [`CStr`] is used. +unsafe fn to_bytes_mut(s: &mut CStr) -> &mut [u8] { + // SAFETY: the cast from `&CStr` to `&[u8]` is safe since `CStr` has the same layout as `&[u8]` + // (this is technically not guaranteed, but we rely on it here). The pointer dereference is + // safe since it comes from a mutable reference which is guaranteed to be valid for writes. + unsafe { &mut *(core::ptr::from_mut(s) as *mut [u8]) } }
-impl AsRef<BStr> for CStr { +impl CStrExt for CStr { #[inline] - fn as_ref(&self) -> &BStr { - BStr::from_bytes(self.as_bytes()) + unsafe fn from_char_ptr<'a>(ptr: *const crate::ffi::c_char) -> &'a Self { + // SAFETY: The safety preconditions are the same as for `CStr::from_ptr`. + unsafe { CStr::from_ptr(ptr.cast()) } } -}
-impl Deref for CStr { - type Target = BStr; + #[inline] + unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut Self { + // SAFETY: the cast from `&[u8]` to `&CStr` is safe since the properties of `bytes` are + // guaranteed by the safety precondition and `CStr` has the same layout as `&[u8]` (this is + // technically not guaranteed, but we rely on it here). The pointer dereference is safe + // since it comes from a mutable reference which is guaranteed to be valid for writes. + unsafe { &mut *(core::ptr::from_mut(bytes) as *mut CStr) } + }
#[inline] - fn deref(&self) -> &Self::Target { - self.as_ref() + fn as_char_ptr(&self) -> *const crate::ffi::c_char { + self.as_ptr().cast() } -}
-impl Index<ops::RangeFrom<usize>> for CStr { - type Output = CStr; + fn to_cstring(&self) -> Result<CString, AllocError> { + CString::try_from(self) + }
- #[inline] - fn index(&self, index: ops::RangeFrom<usize>) -> &Self::Output { - // Delegate bounds checking to slice. - // Assign to _ to mute clippy's unnecessary operation warning. - let _ = &self.as_bytes()[index.start..]; - // SAFETY: We just checked the bounds. - unsafe { Self::from_bytes_with_nul_unchecked(&self.0[index.start..]) } + fn make_ascii_lowercase(&mut self) { + // SAFETY: This doesn't introduce or remove NUL bytes in the C string. + unsafe { to_bytes_mut(self) }.make_ascii_lowercase(); } -}
-impl Indexops::RangeFull for CStr { - type Output = CStr; + fn make_ascii_uppercase(&mut self) { + // SAFETY: This doesn't introduce or remove NUL bytes in the C string. + unsafe { to_bytes_mut(self) }.make_ascii_uppercase(); + }
- #[inline] - fn index(&self, _index: ops::RangeFull) -> &Self::Output { - self + fn to_ascii_lowercase(&self) -> Result<CString, AllocError> { + let mut s = self.to_cstring()?; + + s.make_ascii_lowercase(); + + Ok(s) } -}
-mod private { - use core::ops; + fn to_ascii_uppercase(&self) -> Result<CString, AllocError> { + let mut s = self.to_cstring()?;
- // Marker trait for index types that can be forward to `BStr`. - pub trait CStrIndex {} + s.make_ascii_uppercase();
- impl CStrIndex for usize {} - impl CStrIndex for ops::Range<usize> {} - impl CStrIndex for ops::RangeInclusive<usize> {} - impl CStrIndex for ops::RangeToInclusive<usize> {} + Ok(s) + } }
-impl<Idx> Index<Idx> for CStr -where - Idx: private::CStrIndex, - BStr: Index<Idx>, -{ - type Output = <BStr as Index<Idx>>::Output; - +impl AsRef<BStr> for CStr { #[inline] - fn index(&self, index: Idx) -> &Self::Output { - &self.as_ref()[index] + fn as_ref(&self) -> &BStr { + BStr::from_bytes(self.to_bytes()) } }
@@ -576,6 +391,13 @@ macro_rules! c_str { mod tests { use super::*;
+ impl Fromcore::ffi::FromBytesWithNulError for Error { + #[inline] + fn from(_: core::ffi::FromBytesWithNulError) -> Error { + EINVAL + } + } + macro_rules! format { ($($f:tt)*) => ({ CString::try_from_fmt(crate::prelude::fmt!($($f)*))?.to_str()? @@ -598,40 +420,28 @@ macro_rules! format {
#[test] fn test_cstr_to_str() -> Result { - let good_bytes = b"\xf0\x9f\xa6\x80\0"; - let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; - let checked_str = checked_cstr.to_str()?; + let cstr = c"\xf0\x9f\xa6\x80"; + let checked_str = cstr.to_str()?; assert_eq!(checked_str, "🦀"); Ok(()) }
#[test] fn test_cstr_to_str_invalid_utf8() -> Result { - let bad_bytes = b"\xc3\x28\0"; - let checked_cstr = CStr::from_bytes_with_nul(bad_bytes)?; - assert!(checked_cstr.to_str().is_err()); - Ok(()) - } - - #[test] - fn test_cstr_as_str_unchecked() -> Result { - let good_bytes = b"\xf0\x9f\x90\xA7\0"; - let checked_cstr = CStr::from_bytes_with_nul(good_bytes)?; - // SAFETY: The contents come from a string literal which contains valid UTF-8. - let unchecked_str = unsafe { checked_cstr.as_str_unchecked() }; - assert_eq!(unchecked_str, "🐧"); + let cstr = c"\xc3\x28"; + assert!(cstr.to_str().is_err()); Ok(()) }
#[test] fn test_cstr_display() -> Result { - let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; + let hello_world = c"hello, world!"; assert_eq!(format!("{hello_world}"), "hello, world!"); - let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; + let non_printables = c"\x01\x09\x0a"; assert_eq!(format!("{non_printables}"), "\x01\x09\x0a"); - let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; + let non_ascii = c"d\xe9j\xe0 vu"; assert_eq!(format!("{non_ascii}"), "d\xe9j\xe0 vu"); - let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; + let good_bytes = c"\xf0\x9f\xa6\x80"; assert_eq!(format!("{good_bytes}"), "\xf0\x9f\xa6\x80"); Ok(()) } @@ -650,13 +460,13 @@ fn test_cstr_display_all_bytes() -> Result {
#[test] fn test_cstr_debug() -> Result { - let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0")?; + let hello_world = c"hello, world!"; assert_eq!(format!("{hello_world:?}"), ""hello, world!""); - let non_printables = CStr::from_bytes_with_nul(b"\x01\x09\x0a\0")?; - assert_eq!(format!("{non_printables:?}"), ""\x01\x09\x0a""); - let non_ascii = CStr::from_bytes_with_nul(b"d\xe9j\xe0 vu\0")?; + let non_printables = c"\x01\x09\x0a"; + assert_eq!(format!("{non_printables:?}"), ""\x01\t\n""); + let non_ascii = c"d\xe9j\xe0 vu"; assert_eq!(format!("{non_ascii:?}"), ""d\xe9j\xe0 vu""); - let good_bytes = CStr::from_bytes_with_nul(b"\xf0\x9f\xa6\x80\0")?; + let good_bytes = c"\xf0\x9f\xa6\x80"; assert_eq!(format!("{good_bytes:?}"), ""\xf0\x9f\xa6\x80""); Ok(()) } @@ -843,11 +653,11 @@ fn write_str(&mut self, s: &str) -> fmt::Result { /// use kernel::{str::CString, prelude::fmt}; /// /// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20))?; -/// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes()); +/// assert_eq!(s.to_bytes_with_nul(), "abc1020\0".as_bytes()); /// /// let tmp = "testing"; /// let s = CString::try_from_fmt(fmt!("{tmp}{}", 123))?; -/// assert_eq!(s.as_bytes_with_nul(), "testing123\0".as_bytes()); +/// assert_eq!(s.to_bytes_with_nul(), "testing123\0".as_bytes()); /// /// // This fails because it has an embedded `NUL` byte. /// let s = CString::try_from_fmt(fmt!("a\0b{}", 123)); @@ -917,7 +727,7 @@ impl<'a> TryFrom<&'a CStr> for CString { fn try_from(cstr: &'a CStr) -> Result<CString, AllocError> { let mut buf = KVec::new();
- buf.extend_from_slice(cstr.as_bytes_with_nul(), GFP_KERNEL)?; + buf.extend_from_slice(cstr.to_bytes_with_nul(), GFP_KERNEL)?;
// INVARIANT: The `CStr` and `CString` types have the same invariants for // the string data, and we copied it over without changes. diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index caebf03f553b..0b6bc7f2878d 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -8,7 +8,7 @@ use super::{lock::Backend, lock::Guard, LockClassKey}; use crate::{ ffi::{c_int, c_long}, - str::CStr, + str::{CStr, CStrExt as _}, task::{ MAX_SCHEDULE_TIMEOUT, TASK_FREEZABLE, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE, }, diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index e82fa5be289c..a777a22976e0 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -7,7 +7,7 @@
use super::LockClassKey; use crate::{ - str::CStr, + str::{CStr, CStrExt as _}, types::{NotThreadSafe, Opaque, ScopeGuard}, }; use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs index d65f94b5caf2..79d0ef7fda86 100644 --- a/rust/kernel/sync/lock/global.rs +++ b/rust/kernel/sync/lock/global.rs @@ -5,7 +5,7 @@ //! Support for defining statics containing locks.
use crate::{ - str::CStr, + str::{CStr, CStrExt as _}, sync::lock::{Backend, Guard, Lock}, sync::{LockClassKey, LockedBy}, types::Opaque, diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs index af04bfa35cb2..5005453f874d 100644 --- a/samples/rust/rust_configfs.rs +++ b/samples/rust/rust_configfs.rs @@ -94,7 +94,7 @@ impl configfs::AttributeOperations<0> for Configuration {
fn show(container: &Configuration, page: &mut [u8; PAGE_SIZE]) -> Result<usize> { pr_info!("Show message\n"); - let data = container.message; + let data = container.message.to_bytes(); page[0..data.len()].copy_from_slice(data); Ok(data.len()) }
On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
`kernel::ffi::CStr` was introduced in commit d126d2380131 ("rust: str: add `CStr` type") in November 2022 as an upstreaming of earlier work that was done in May 2021[0]. That earlier work, having predated the inclusion of `CStr` in `core`, largely duplicated the implementation of `std::ffi::CStr`.
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64 in September 2022. Hence replace `kernel::str::CStr` with `core::ffi::CStr` to reduce our custom code footprint, and retain needed custom functionality through an extension trait.
C-String literals were added in Rust 1.77, while our MSRV is 1.78. Thus opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary or where existing code triggered clippy lints; the rest will be done in a later commit.
Link: https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c... [0] Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/auxiliary.rs | 4 +- rust/kernel/configfs.rs | 4 +- rust/kernel/cpufreq.rs | 2 +- rust/kernel/device.rs | 4 +- rust/kernel/drm/device.rs | 4 +- rust/kernel/error.rs | 4 +- rust/kernel/firmware.rs | 11 +- rust/kernel/kunit.rs | 6 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/net/phy.rs | 2 +- rust/kernel/of.rs | 2 +- rust/kernel/prelude.rs | 5 +- rust/kernel/seq_file.rs | 4 +- rust/kernel/str.rs | 394 +++++++++++----------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- samples/rust/rust_configfs.rs | 2 +- 19 files changed, 140 insertions(+), 318 deletions(-)
Is it also possible to split this? First rename the existing functions on our CStr to match upstream & then you don't need to do the rename & removal of our CStr in the same patch?
--- Cheers, Benno
On Fri, Jul 4, 2025 at 9:00 AM Benno Lossin lossin@kernel.org wrote:
On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
`kernel::ffi::CStr` was introduced in commit d126d2380131 ("rust: str: add `CStr` type") in November 2022 as an upstreaming of earlier work that was done in May 2021[0]. That earlier work, having predated the inclusion of `CStr` in `core`, largely duplicated the implementation of `std::ffi::CStr`.
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64 in September 2022. Hence replace `kernel::str::CStr` with `core::ffi::CStr` to reduce our custom code footprint, and retain needed custom functionality through an extension trait.
C-String literals were added in Rust 1.77, while our MSRV is 1.78. Thus opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary or where existing code triggered clippy lints; the rest will be done in a later commit.
Link: https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c... [0] Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/auxiliary.rs | 4 +- rust/kernel/configfs.rs | 4 +- rust/kernel/cpufreq.rs | 2 +- rust/kernel/device.rs | 4 +- rust/kernel/drm/device.rs | 4 +- rust/kernel/error.rs | 4 +- rust/kernel/firmware.rs | 11 +- rust/kernel/kunit.rs | 6 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/net/phy.rs | 2 +- rust/kernel/of.rs | 2 +- rust/kernel/prelude.rs | 5 +- rust/kernel/seq_file.rs | 4 +- rust/kernel/str.rs | 394 +++++++++++----------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- samples/rust/rust_configfs.rs | 2 +- 19 files changed, 140 insertions(+), 318 deletions(-)
Is it also possible to split this? First rename the existing functions on our CStr to match upstream & then you don't need to do the rename & removal of our CStr in the same patch?
Yes.
C-String literals were added in Rust 1.77. Replace instances of `kernel::c_str!` with C-String literals where possible and rename `kernel::c_str!` to `str_to_cstr!` to clarify its intended use.
Closes: https://github.com/Rust-for-Linux/linux/issues/1075 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/cpufreq/rcpufreq_dt.rs | 5 ++--- drivers/gpu/drm/nova/driver.rs | 10 +++++----- drivers/gpu/nova-core/driver.rs | 6 +++--- drivers/net/phy/ax88796b_rust.rs | 7 +++---- drivers/net/phy/qt2025.rs | 5 ++--- rust/kernel/clk.rs | 6 ++---- rust/kernel/configfs.rs | 9 +++++---- rust/kernel/cpufreq.rs | 3 +-- rust/kernel/devres.rs | 2 +- rust/kernel/drm/ioctl.rs | 2 +- rust/kernel/firmware.rs | 6 +++--- rust/kernel/kunit.rs | 14 ++++++-------- rust/kernel/net/phy.rs | 6 ++---- rust/kernel/platform.rs | 4 ++-- rust/kernel/str.rs | 24 ++++++++++++++++-------- rust/kernel/sync.rs | 7 +++---- rust/kernel/sync/completion.rs | 2 +- rust/kernel/sync/lock/global.rs | 3 ++- rust/kernel/workqueue.rs | 8 ++++---- rust/macros/kunit.rs | 10 +++++----- rust/macros/module.rs | 2 +- samples/rust/rust_configfs.rs | 5 ++--- samples/rust/rust_driver_auxiliary.rs | 4 ++-- samples/rust/rust_driver_faux.rs | 4 ++-- samples/rust/rust_driver_pci.rs | 4 ++-- samples/rust/rust_driver_platform.rs | 4 ++-- samples/rust/rust_misc_device.rs | 3 +-- scripts/rustdoc_test_gen.rs | 4 ++-- 29 files changed, 84 insertions(+), 87 deletions(-)
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs index 6366da12c5a5..9aa79b862b63 100644 --- a/drivers/block/rnull.rs +++ b/drivers/block/rnull.rs @@ -55,7 +55,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> { })();
try_pin_init!(Self { - _disk <- new_mutex!(disk?, "nullb:disk"), + _disk <- new_mutex!(disk?, c"nullb:disk"), }) } } diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs index 30a170570c0e..0c88032dd919 100644 --- a/drivers/cpufreq/rcpufreq_dt.rs +++ b/drivers/cpufreq/rcpufreq_dt.rs @@ -3,7 +3,6 @@ //! Rust based implementation of the cpufreq-dt driver.
use kernel::{ - c_str, clk::Clk, cpu, cpufreq, cpumask::CpumaskVar, @@ -56,7 +55,7 @@ impl opp::ConfigOps for CPUFreqDTDriver {}
#[vtable] impl cpufreq::Driver for CPUFreqDTDriver { - const NAME: &'static CStr = c_str!("cpufreq-dt"); + const NAME: &'static CStr = c"cpufreq-dt"; const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV; const BOOST_ENABLED: bool = true;
@@ -201,7 +200,7 @@ fn register_em(policy: &mut cpufreq::Policy) { OF_TABLE, MODULE_OF_TABLE, <CPUFreqDTDriver as platform::Driver>::IdInfo, - [(of::DeviceId::new(c_str!("operating-points-v2")), ())] + [(of::DeviceId::new(c"operating-points-v2"), ())] );
impl platform::Driver for CPUFreqDTDriver { diff --git a/drivers/gpu/drm/nova/driver.rs b/drivers/gpu/drm/nova/driver.rs index b28b2e05cc15..87480ee8dbae 100644 --- a/drivers/gpu/drm/nova/driver.rs +++ b/drivers/gpu/drm/nova/driver.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0
-use kernel::{auxiliary, c_str, device::Core, drm, drm::gem, drm::ioctl, prelude::*, types::ARef}; +use kernel::{auxiliary, device::Core, drm, drm::gem, drm::ioctl, prelude::*, types::ARef};
use crate::file::File; use crate::gem::NovaObject; @@ -22,12 +22,12 @@ pub(crate) struct NovaData { major: 0, minor: 0, patchlevel: 0, - name: c_str!("nova"), - desc: c_str!("Nvidia Graphics"), + name: c"nova", + desc: c"Nvidia Graphics", };
-const NOVA_CORE_MODULE_NAME: &CStr = c_str!("NovaCore"); -const AUXILIARY_NAME: &CStr = c_str!("nova-drm"); +const NOVA_CORE_MODULE_NAME: &CStr = c"NovaCore"; +const AUXILIARY_NAME: &CStr = c"nova-drm";
kernel::auxiliary_device_table!( AUX_TABLE, diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index a0e435dc4656..16cd7e36662c 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0
-use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*}; +use kernel::{auxiliary, bindings, device::Core, pci, prelude::*};
use crate::gpu::Gpu;
@@ -34,14 +34,14 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self pdev.enable_device_mem()?; pdev.set_master();
- let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?; + let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?;
let this = KBox::pin_init( try_pin_init!(Self { gpu <- Gpu::new(pdev, bar)?, _reg: auxiliary::Registration::new( pdev.as_ref(), - c_str!("nova-drm"), + c"nova-drm", 0, // TODO: Once it lands, use XArray; for now we don't use the ID. crate::MODULE_NAME )?, diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs index bc73ebccc2aa..2d24628a4e58 100644 --- a/drivers/net/phy/ax88796b_rust.rs +++ b/drivers/net/phy/ax88796b_rust.rs @@ -5,7 +5,6 @@ //! //! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c) use kernel::{ - c_str, net::phy::{self, reg::C22, DeviceId, Driver}, prelude::*, uapi, @@ -41,7 +40,7 @@ fn asix_soft_reset(dev: &mut phy::Device) -> Result { #[vtable] impl Driver for PhyAX88772A { const FLAGS: u32 = phy::flags::IS_INTERNAL; - const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); + const NAME: &'static CStr = c"Asix Electronics AX88772A"; const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_exact_mask(0x003b1861);
// AX88772A is not working properly with some old switches (NETGEAR EN 108TP): @@ -105,7 +104,7 @@ fn link_change_notify(dev: &mut phy::Device) { #[vtable] impl Driver for PhyAX88772C { const FLAGS: u32 = phy::flags::IS_INTERNAL; - const NAME: &'static CStr = c_str!("Asix Electronics AX88772C"); + const NAME: &'static CStr = c"Asix Electronics AX88772C"; const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_exact_mask(0x003b1881);
fn suspend(dev: &mut phy::Device) -> Result { @@ -125,7 +124,7 @@ fn soft_reset(dev: &mut phy::Device) -> Result {
#[vtable] impl Driver for PhyAX88796B { - const NAME: &'static CStr = c_str!("Asix Electronics AX88796B"); + const NAME: &'static CStr = c"Asix Electronics AX88796B"; const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_model_mask(0x003b1841);
fn soft_reset(dev: &mut phy::Device) -> Result { diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs index 0b9400dcb4c1..9ccc75f70219 100644 --- a/drivers/net/phy/qt2025.rs +++ b/drivers/net/phy/qt2025.rs @@ -9,7 +9,6 @@ //! //! The QT2025 PHY integrates an Intel 8051 micro-controller.
-use kernel::c_str; use kernel::error::code; use kernel::firmware::Firmware; use kernel::net::phy::{ @@ -36,7 +35,7 @@
#[vtable] impl Driver for PhyQT2025 { - const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+"); + const NAME: &'static CStr = c"QT2025 10Gpbs SFP+"; const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043a400);
fn probe(dev: &mut phy::Device) -> Result<()> { @@ -69,7 +68,7 @@ fn probe(dev: &mut phy::Device) -> Result<()> { // The micro-controller will start running from the boot ROM. dev.write(C45::new(Mmd::PCS, 0xe854), 0x00c0)?;
- let fw = Firmware::request(c_str!("qt2025-2.0.3.3.fw"), dev.as_ref())?; + let fw = Firmware::request(c"qt2025-2.0.3.3.fw", dev.as_ref())?; if fw.data().len() > SZ_16K + SZ_8K { return Err(code::EFBIG); } diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs index 34a19bc99990..fb0f259cf231 100644 --- a/rust/kernel/clk.rs +++ b/rust/kernel/clk.rs @@ -100,13 +100,12 @@ mod common_clk { /// The following example demonstrates how to obtain and configure a clock for a device. /// /// ``` - /// use kernel::c_str; /// use kernel::clk::{Clk, Hertz}; /// use kernel::device::Device; /// use kernel::error::Result; /// /// fn configure_clk(dev: &Device) -> Result { - /// let clk = Clk::get(dev, Some(c_str!("apb_clk")))?; + /// let clk = Clk::get(dev, Some(c"apb_clk"))?; /// /// clk.prepare_enable()?; /// @@ -272,13 +271,12 @@ fn drop(&mut self) { /// device. The code functions correctly whether or not the clock is available. /// /// ``` - /// use kernel::c_str; /// use kernel::clk::{OptionalClk, Hertz}; /// use kernel::device::Device; /// use kernel::error::Result; /// /// fn configure_clk(dev: &Device) -> Result { - /// let clk = OptionalClk::get(dev, Some(c_str!("apb_clk")))?; + /// let clk = OptionalClk::get(dev, Some(c"apb_clk"))?; /// /// clk.prepare_enable()?; /// diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index d4797c41ba77..255460d571f6 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -21,7 +21,6 @@ //! //! ```ignore //! use kernel::alloc::flags; -//! use kernel::c_str; //! use kernel::configfs_attrs; //! use kernel::configfs; //! use kernel::new_mutex; @@ -50,7 +49,7 @@ //! //! try_pin_init!(Self { //! config <- configfs::Subsystem::new( -//! c_str!("rust_configfs"), item_type, Configuration::new() +//! c"rust_configfs", item_type, Configuration::new() //! ), //! }) //! } @@ -66,7 +65,7 @@ //! impl Configuration { //! fn new() -> impl PinInit<Self, Error> { //! try_pin_init!(Self { -//! message: c_str!("Hello World\n"), +//! message: c"Hello World\n", //! bar <- new_mutex!((KBox::new([0; PAGE_SIZE], flags::GFP_KERNEL)?, 0)), //! }) //! } @@ -1000,7 +999,9 @@ macro_rules! configfs_attrs { static [< $data:upper _ $name:upper _ATTR >]: $crate::configfs::Attribute<$attr, $data, $data> = unsafe { - $crate::configfs::Attribute::new(c_str!(::core::stringify!($name))) + $crate::configfs::Attribute::new( + $crate::str_to_cstr!(::core::stringify!($name)), + ) }; )*
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs index 71d601f7c261..0f316dfeb5dd 100644 --- a/rust/kernel/cpufreq.rs +++ b/rust/kernel/cpufreq.rs @@ -841,7 +841,6 @@ fn register_em(_policy: &mut Policy) { /// ``` /// use kernel::{ /// cpufreq, -/// c_str, /// device::{Core, Device}, /// macros::vtable, /// of, platform, @@ -854,7 +853,7 @@ fn register_em(_policy: &mut Policy) { /// /// #[vtable] /// impl cpufreq::Driver for SampleDriver { -/// const NAME: &'static CStr = c_str!("cpufreq-sample"); +/// const NAME: &'static CStr = c"cpufreq-sample"; /// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV; /// const BOOST_ENABLED: bool = true; /// diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index d0e6c6e162c2..dff443908278 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -47,7 +47,7 @@ struct DevresInner<T> { /// # Examples /// /// ```no_run -/// # use kernel::{bindings, c_str, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}}; +/// # use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}}; /// # use core::ops::Deref; /// /// // See also [`pci::Bar`] for a real example. diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs index 445639404fb7..e67c9fe9cb91 100644 --- a/rust/kernel/drm/ioctl.rs +++ b/rust/kernel/drm/ioctl.rs @@ -153,7 +153,7 @@ macro_rules! declare_drm_ioctls { Some($cmd) }, flags: $flags, - name: $crate::c_str!(::core::stringify!($cmd)).as_char_ptr(), + name: $crate::str_to_cstr!(::core::stringify!($cmd)).as_char_ptr(), } ),*]; ioctls diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index 4adcf39b475e..d07849333991 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -51,13 +51,13 @@ fn request_nowarn() -> Self { /// # Examples /// /// ```no_run -/// # use kernel::{c_str, device::Device, firmware::Firmware}; +/// # use kernel::{device::Device, firmware::Firmware}; /// /// # fn no_run() -> Result<(), Error> { /// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance /// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) }; /// -/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?; +/// let fw = Firmware::request(c"path/to/firmware.bin", &dev)?; /// let blob = fw.data(); /// /// # Ok(()) @@ -204,7 +204,7 @@ macro_rules! module_firmware { ($($builder:tt)*) => { const _: () = { const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) { - $crate::c_str!("") + c"" } else { <LocalModule as $crate::ModuleMetadata>::NAME }; diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 6f80dc673974..1d108e4a6a39 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -9,9 +9,6 @@ use crate::prelude::*; use core::fmt;
-#[cfg(CONFIG_PRINTK)] -use crate::c_str; - /// Prints a KUnit error-level message. /// /// Public but hidden since it should only be used from KUnit generated code. @@ -22,7 +19,7 @@ pub fn err(args: fmt::Arguments<'_>) { #[cfg(CONFIG_PRINTK)] unsafe { bindings::_printk( - c_str!("\x013%pA").as_char_ptr(), + c"\x013%pA".as_char_ptr(), core::ptr::from_ref(&args).cast::<c_void>(), ); } @@ -38,7 +35,7 @@ pub fn info(args: fmt::Arguments<'_>) { #[cfg(CONFIG_PRINTK)] unsafe { bindings::_printk( - c_str!("\x016%pA").as_char_ptr(), + c"\x016%pA".as_char_ptr(), core::ptr::from_ref(&args).cast::<c_void>(), ); } @@ -60,9 +57,10 @@ macro_rules! kunit_assert { break 'out; }
- static FILE: &'static $crate::str::CStr = $crate::c_str!($file); + static FILE: &'static $crate::str::CStr = $file; static LINE: i32 = ::core::line!() as i32 - $diff; - static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); + static CONDITION: &'static $crate::str::CStr = + $crate::str_to_cstr!(stringify!($condition));
// SAFETY: FFI call without safety requirements. let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; @@ -249,7 +247,7 @@ pub const fn kunit_case_null() -> kernel::bindings::kunit_case { /// } /// /// static mut KUNIT_TEST_CASES: [kernel::bindings::kunit_case; 2] = [ -/// kernel::kunit::kunit_case(kernel::c_str!("name"), test_fn), +/// kernel::kunit::kunit_case(c"name", test_fn), /// kernel::kunit::kunit_case_null(), /// ]; /// kernel::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES); diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index c420e5ecab4b..236ea516a134 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -781,7 +781,6 @@ const fn as_int(&self) -> u32 { /// /// ``` /// # mod module_phy_driver_sample { -/// use kernel::c_str; /// use kernel::net::phy::{self, DeviceId}; /// use kernel::prelude::*; /// @@ -800,7 +799,7 @@ const fn as_int(&self) -> u32 { /// /// #[vtable] /// impl phy::Driver for PhySample { -/// const NAME: &'static CStr = c_str!("PhySample"); +/// const NAME: &'static CStr = c"PhySample"; /// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001); /// } /// # } @@ -809,7 +808,6 @@ const fn as_int(&self) -> u32 { /// This expands to the following code: /// /// ```ignore -/// use kernel::c_str; /// use kernel::net::phy::{self, DeviceId}; /// use kernel::prelude::*; /// @@ -829,7 +827,7 @@ const fn as_int(&self) -> u32 { /// /// #[vtable] /// impl phy::Driver for PhySample { -/// const NAME: &'static CStr = c_str!("PhySample"); +/// const NAME: &'static CStr = c"PhySample"; /// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001); /// } /// diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index 0a6a6be732b2..99ad0b132ab6 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -125,7 +125,7 @@ macro_rules! module_platform_driver { /// # Examples /// ///``` -/// # use kernel::{bindings, c_str, device::Core, of, platform}; +/// # use kernel::{bindings, device::Core, of, platform}; /// /// struct MyDriver; /// @@ -134,7 +134,7 @@ macro_rules! module_platform_driver { /// MODULE_OF_TABLE, /// <MyDriver as platform::Driver>::IdInfo, /// [ -/// (of::DeviceId::new(c_str!("test,device")), ()) +/// (of::DeviceId::new(c"test,device"), ()) /// ] /// ); /// diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 5c365646c0e1..ff4aaf627179 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -266,15 +266,14 @@ impl crate::fmt::Display for CStr { /// Formats printable ASCII characters, escaping the rest. /// /// ``` - /// # use kernel::c_str; /// # use kernel::prelude::fmt; /// # use kernel::str::CStr; /// # use kernel::str::CString; - /// let penguin = c_str!("🐧"); + /// let penguin = c"🐧"; /// let s = CString::try_from_fmt(fmt!("{}", penguin))?; /// assert_eq!(s.to_bytes_with_nul(), "\xf0\x9f\x90\xa7\0".as_bytes()); /// - /// let ascii = c_str!("so "cool""); + /// let ascii = c"so "cool""; /// let s = CString::try_from_fmt(fmt!("{}", ascii))?; /// assert_eq!(s.to_bytes_with_nul(), "so "cool"\0".as_bytes()); /// # Ok::<(), kernel::error::Error>(()) @@ -364,19 +363,28 @@ fn as_ref(&self) -> &BStr { } }
-/// Creates a new [`CStr`] from a string literal. +/// Creates a new [`CStr`] at compile time. /// -/// The string literal should not contain any `NUL` bytes. +/// Rust supports C string literals since Rust 1.77, and they should be used instead of this macro +/// where possible. This macro exists to allow static *non-literal* C strings to be created at +/// compile time. This is most often used in other macros. +/// +/// # Panics +/// +/// This macro panics if the operand contains an interior `NUL` byte. /// /// # Examples /// /// ``` -/// # use kernel::c_str; +/// # use kernel::str_to_cstr; /// # use kernel::str::CStr; -/// const MY_CSTR: &CStr = c_str!("My awesome CStr!"); +/// const MY_CSTR: &CStr = str_to_cstr!(concat!(file!(), ":", line!(), ": My CStr!")); /// ``` #[macro_export] -macro_rules! c_str { +macro_rules! str_to_cstr { + // NB: we could write `($str:lit) => compile_error!("use a C string literal instead");` here but + // that would trigger when the literal is at the top of several macro expansions. That would be + // too limiting to macro authors, so we rely on the name as a hint instead. ($str:expr) => {{ const S: &str = concat!($str, "\0"); const C: &$crate::str::CStr = match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) { diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 63c99e015ad6..9e394ffa1334 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -43,7 +43,6 @@ impl LockClassKey { /// /// # Examples /// ``` - /// # use kernel::c_str; /// # use kernel::alloc::KBox; /// # use kernel::types::ForeignOwnable; /// # use kernel::sync::{LockClassKey, SpinLock}; @@ -55,7 +54,7 @@ impl LockClassKey { /// { /// stack_pin_init!(let num: SpinLock<u32> = SpinLock::new( /// 0, - /// c_str!("my_spinlock"), + /// c"my_spinlock", /// // SAFETY: `key_ptr` is returned by the above `into_foreign()`, whose /// // `from_foreign()` has not yet been called. /// unsafe { <Pin<KBox<LockClassKey>> as ForeignOwnable>::borrow(key_ptr) } @@ -108,9 +107,9 @@ macro_rules! static_lock_class { #[macro_export] macro_rules! optional_name { () => { - $crate::c_str!(::core::concat!(::core::file!(), ":", ::core::line!())) + $crate::str_to_cstr!(::core::concat!(::core::file!(), ":", ::core::line!())) }; ($name:literal) => { - $crate::c_str!($name) + $name }; } diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs index c50012a940a3..97d39c248793 100644 --- a/rust/kernel/sync/completion.rs +++ b/rust/kernel/sync/completion.rs @@ -34,7 +34,7 @@ /// impl MyTask { /// fn new() -> Result<Arc<Self>> { /// let this = Arc::pin_init(pin_init!(MyTask { -/// work <- new_work!("MyTask::work"), +/// work <- new_work!(c"MyTask::work"), /// done <- Completion::new(), /// }), GFP_KERNEL)?; /// diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs index 79d0ef7fda86..9caa9b419f09 100644 --- a/rust/kernel/sync/lock/global.rs +++ b/rust/kernel/sync/lock/global.rs @@ -267,7 +267,8 @@ macro_rules! global_lock { $pub enum $name {}
impl $crate::sync::lock::GlobalLockBackend for $name { - const NAME: &'static $crate::str::CStr = $crate::c_str!(::core::stringify!($name)); + const NAME: &'static $crate::str::CStr = + $crate::str_to_cstr!(::core::stringify!($name)); type Item = $valuety; type Backend = $crate::global_lock_inner!(backend $kind);
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index cce23684af24..432624c69c72 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -51,7 +51,7 @@ //! fn new(value: i32) -> Result<Arc<Self>> { //! Arc::pin_init(pin_init!(MyStruct { //! value, -//! work <- new_work!("MyStruct::work"), +//! work <- new_work!(c"MyStruct::work"), //! }), GFP_KERNEL) //! } //! } @@ -98,8 +98,8 @@ //! Arc::pin_init(pin_init!(MyStruct { //! value_1, //! value_2, -//! work_1 <- new_work!("MyStruct::work_1"), -//! work_2 <- new_work!("MyStruct::work_2"), +//! work_1 <- new_work!(c"MyStruct::work_1"), +//! work_2 <- new_work!(c"MyStruct::work_2"), //! }), GFP_KERNEL) //! } //! } @@ -215,7 +215,7 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>( func: T, ) -> Result<(), AllocError> { let init = pin_init!(ClosureWork { - work <- new_work!("Queue::try_spawn"), + work <- new_work!(c"Queue::try_spawn"), func: Some(func), });
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 81d18149a0cc..c64df1a01b9d 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -89,8 +89,8 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // 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(c"foo", kunit_rust_wrapper_foo), + // ::kernel::kunit::kunit_case(c"bar", kunit_rust_wrapper_bar), // ::kernel::kunit::kunit_case_null(), // ]; // @@ -109,7 +109,7 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { writeln!(kunit_macros, "{kunit_wrapper}").unwrap(); writeln!( test_cases, - " ::kernel::kunit::kunit_case(::kernel::c_str!("{test}"), {kunit_wrapper_fn_name})," + " ::kernel::kunit::kunit_case(c"{test}", {kunit_wrapper_fn_name})," ) .unwrap(); writeln!( @@ -119,7 +119,7 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { #[allow(unused)] macro_rules! assert {{ ($cond:expr $(,)?) => {{{{ - kernel::kunit_assert!("{test}", "{path}", 0, $cond); + kernel::kunit_assert!("{test}", c"{path}", 0, $cond); }}}} }}
@@ -127,7 +127,7 @@ macro_rules! assert {{ #[allow(unused)] macro_rules! assert_eq {{ ($left:expr, $right:expr $(,)?) => {{{{ - kernel::kunit_assert_eq!("{test}", "{path}", 0, $left, $right); + kernel::kunit_assert_eq!("{test}", c"{path}", 0, $left, $right); }}}} }} "# diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 5dd276a2e5cb..532342a38b6f 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -228,7 +228,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { type LocalModule = {type_};
impl ::kernel::ModuleMetadata for {type_} {{ - const NAME: &'static ::kernel::str::CStr = ::kernel::c_str!("{name}"); + const NAME: &'static ::kernel::str::CStr = c"{name}"; }}
// Double nested modules, since then nobody can access the public items inside. diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs index 5005453f874d..ea84c23b784b 100644 --- a/samples/rust/rust_configfs.rs +++ b/samples/rust/rust_configfs.rs @@ -3,7 +3,6 @@ //! Rust configfs sample.
use kernel::alloc::flags; -use kernel::c_str; use kernel::configfs; use kernel::configfs_attrs; use kernel::new_mutex; @@ -35,7 +34,7 @@ struct Configuration { impl Configuration { fn new() -> impl PinInit<Self, Error> { try_pin_init!(Self { - message: c_str!("Hello World\n"), + message: c"Hello World\n", bar <- new_mutex!((KBox::new([0; PAGE_SIZE], flags::GFP_KERNEL)?, 0)), }) } @@ -61,7 +60,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
try_pin_init!(Self { config <- configfs::Subsystem::new( - c_str!("rust_configfs"), item_type, Configuration::new() + c"rust_configfs", item_type, Configuration::new() ), }) } diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index abf3d55ed249..eaac5a94f796 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -5,14 +5,14 @@ //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
use kernel::{ - auxiliary, bindings, c_str, device::Core, driver, error::Error, pci, prelude::*, str::CStr, + auxiliary, bindings, device::Core, driver, error::Error, pci, prelude::*, str::CStr, InPlaceModule, };
use pin_init::PinInit;
const MODULE_NAME: &CStr = <LocalModule as kernel::ModuleMetadata>::NAME; -const AUXILIARY_NAME: &CStr = c_str!("auxiliary"); +const AUXILIARY_NAME: &CStr = c"auxiliary";
struct AuxiliaryDriver;
diff --git a/samples/rust/rust_driver_faux.rs b/samples/rust/rust_driver_faux.rs index ecc9fd378cbd..23add3160693 100644 --- a/samples/rust/rust_driver_faux.rs +++ b/samples/rust/rust_driver_faux.rs @@ -2,7 +2,7 @@
//! Rust faux device sample.
-use kernel::{c_str, faux, prelude::*, Module}; +use kernel::{faux, prelude::*, Module};
module! { type: SampleModule, @@ -20,7 +20,7 @@ impl Module for SampleModule { fn init(_module: &'static ThisModule) -> Result<Self> { pr_info!("Initialising Rust Faux Device Sample\n");
- let reg = faux::Registration::new(c_str!("rust-faux-sample-device"), None)?; + let reg = faux::Registration::new(c"rust-faux-sample-device", None)?;
dev_info!(reg.as_ref(), "Hello from faux device!\n");
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs index 15147e4401b2..4ba5fcd2b357 100644 --- a/samples/rust/rust_driver_pci.rs +++ b/samples/rust/rust_driver_pci.rs @@ -4,7 +4,7 @@ //! //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
-use kernel::{bindings, c_str, device::Core, devres::Devres, pci, prelude::*, types::ARef}; +use kernel::{bindings, device::Core, devres::Devres, pci, prelude::*, types::ARef};
struct Regs;
@@ -73,7 +73,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self> pdev.enable_device_mem()?; pdev.set_master();
- let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?; + let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c"rust_driver_pci")?;
let drvdata = KBox::new( Self { diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs index 8b42b3cfb363..e6487a970a59 100644 --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -2,7 +2,7 @@
//! Rust Platform driver sample.
-use kernel::{c_str, device::Core, of, platform, prelude::*, types::ARef}; +use kernel::{device::Core, of, platform, prelude::*, types::ARef};
struct SampleDriver { pdev: ARefplatform::Device, @@ -14,7 +14,7 @@ struct SampleDriver { OF_TABLE, MODULE_OF_TABLE, <SampleDriver as platform::Driver>::IdInfo, - [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))] + [(of::DeviceId::new(c"test,rust-device"), Info(42))] );
impl platform::Driver for SampleDriver { diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs index c881fd6dbd08..12b64296e912 100644 --- a/samples/rust/rust_misc_device.rs +++ b/samples/rust/rust_misc_device.rs @@ -98,7 +98,6 @@ use core::pin::Pin;
use kernel::{ - c_str, device::Device, fs::File, ioctl::{_IO, _IOC_SIZE, _IOR, _IOW}, @@ -133,7 +132,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> { pr_info!("Initialising Rust Misc Device Sample\n");
let options = MiscDeviceOptions { - name: c_str!("rust-misc-device"), + name: c"rust-misc-device", };
try_pin_init!(Self { diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index 507d36875196..0e86bdf1b5b1 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -173,7 +173,7 @@ pub extern "C" fn {kunit_name}(__kunit_test: *mut ::kernel::bindings::kunit) {{ macro_rules! assert {{ ($cond:expr $(,)?) => {{{{ ::kernel::kunit_assert!( - "{kunit_name}", "{real_path}", __DOCTEST_ANCHOR - {line}, $cond + "{kunit_name}", c"{real_path}", __DOCTEST_ANCHOR - {line}, $cond ); }}}} }} @@ -183,7 +183,7 @@ macro_rules! assert {{ macro_rules! assert_eq {{ ($left:expr, $right:expr $(,)?) => {{{{ ::kernel::kunit_assert_eq!( - "{kunit_name}", "{real_path}", __DOCTEST_ANCHOR - {line}, $left, $right + "{kunit_name}", c"{real_path}", __DOCTEST_ANCHOR - {line}, $left, $right ); }}}} }}
On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
C-String literals were added in Rust 1.77. Replace instances of `kernel::c_str!` with C-String literals where possible and rename `kernel::c_str!` to `str_to_cstr!` to clarify its intended use.
These two things can also be split? And it should also be possible to do this by-subsystem, right?
--- Cheers, Benno
Closes: https://github.com/Rust-for-Linux/linux/issues/1075 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/cpufreq/rcpufreq_dt.rs | 5 ++--- drivers/gpu/drm/nova/driver.rs | 10 +++++----- drivers/gpu/nova-core/driver.rs | 6 +++--- drivers/net/phy/ax88796b_rust.rs | 7 +++---- drivers/net/phy/qt2025.rs | 5 ++--- rust/kernel/clk.rs | 6 ++---- rust/kernel/configfs.rs | 9 +++++---- rust/kernel/cpufreq.rs | 3 +-- rust/kernel/devres.rs | 2 +- rust/kernel/drm/ioctl.rs | 2 +- rust/kernel/firmware.rs | 6 +++--- rust/kernel/kunit.rs | 14 ++++++-------- rust/kernel/net/phy.rs | 6 ++---- rust/kernel/platform.rs | 4 ++-- rust/kernel/str.rs | 24 ++++++++++++++++-------- rust/kernel/sync.rs | 7 +++---- rust/kernel/sync/completion.rs | 2 +- rust/kernel/sync/lock/global.rs | 3 ++- rust/kernel/workqueue.rs | 8 ++++---- rust/macros/kunit.rs | 10 +++++----- rust/macros/module.rs | 2 +- samples/rust/rust_configfs.rs | 5 ++--- samples/rust/rust_driver_auxiliary.rs | 4 ++-- samples/rust/rust_driver_faux.rs | 4 ++-- samples/rust/rust_driver_pci.rs | 4 ++-- samples/rust/rust_driver_platform.rs | 4 ++-- samples/rust/rust_misc_device.rs | 3 +-- scripts/rustdoc_test_gen.rs | 4 ++-- 29 files changed, 84 insertions(+), 87 deletions(-)
On Fri, Jul 4, 2025 at 9:01 AM Benno Lossin lossin@kernel.org wrote:
On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
C-String literals were added in Rust 1.77. Replace instances of `kernel::c_str!` with C-String literals where possible and rename `kernel::c_str!` to `str_to_cstr!` to clarify its intended use.
These two things can also be split? And it should also be possible to do this by-subsystem, right?
Yes.
Clean up references to `kernel::str::CStr`.
Acked-by: Stephen Boyd sboyd@kernel.org Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/gpu/nova-core/nova_core.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 1 + drivers/net/phy/qt2025.rs | 1 + rust/kernel/auxiliary.rs | 2 +- rust/kernel/clk.rs | 3 +-- rust/kernel/configfs.rs | 1 + rust/kernel/cpufreq.rs | 3 ++- rust/kernel/device.rs | 3 +-- rust/kernel/driver.rs | 4 ++-- rust/kernel/drm/driver.rs | 3 ++- rust/kernel/error.rs | 6 ++---- rust/kernel/faux.rs | 5 ++++- rust/kernel/firmware.rs | 15 ++++----------- rust/kernel/kunit.rs | 6 +++--- rust/kernel/lib.rs | 2 +- rust/kernel/miscdevice.rs | 3 +-- rust/kernel/net/phy.rs | 4 +++- rust/kernel/of.rs | 3 ++- rust/kernel/pci.rs | 2 +- rust/kernel/platform.rs | 2 +- rust/kernel/prelude.rs | 5 +---- rust/kernel/str.rs | 8 +++----- rust/kernel/sync/condvar.rs | 4 ++-- rust/kernel/sync/lock.rs | 4 ++-- rust/kernel/sync/lock/global.rs | 5 +++-- rust/kernel/sync/poll.rs | 1 + rust/kernel/workqueue.rs | 1 + rust/macros/module.rs | 2 +- samples/rust/rust_configfs.rs | 2 ++ samples/rust/rust_driver_auxiliary.rs | 5 +++-- 32 files changed, 57 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index fea062cc0383..05d4c7b87fc3 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -27,7 +27,8 @@ //! * https://github.com/erwanvivien/fast_qr //! * https://github.com/bjguillot/qr
-use kernel::{prelude::*, str::CStr}; +use core::ffi::CStr; +use kernel::prelude::*;
#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] struct Version(usize); diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs index 4b8a38358a4f..562ad048ff99 100644 --- a/drivers/gpu/nova-core/firmware.rs +++ b/drivers/gpu/nova-core/firmware.rs @@ -66,7 +66,7 @@ const fn make_entry_chipset(self, chipset: &str) -> Self { }
pub(crate) const fn create( - module_name: &'static kernel::str::CStr, + module_name: &'static core::ffi::CStr, ) -> firmware::ModInfoBuilder<N> { let mut this = Self(firmware::ModInfoBuilder::new(module_name)); let mut i = 0; diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs index f405d7a99c28..b3f9bfba64d9 100644 --- a/drivers/gpu/nova-core/nova_core.rs +++ b/drivers/gpu/nova-core/nova_core.rs @@ -8,7 +8,7 @@ mod regs; mod util;
-pub(crate) const MODULE_NAME: &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata>::NAME; +pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
kernel::module_pci_driver! { type: driver::NovaCore, diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs index 2d24628a4e58..68b8e30ae296 100644 --- a/drivers/net/phy/ax88796b_rust.rs +++ b/drivers/net/phy/ax88796b_rust.rs @@ -4,6 +4,7 @@ //! Rust Asix PHYs driver //! //! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c) +use core::ffi::CStr; use kernel::{ net::phy::{self, reg::C22, DeviceId, Driver}, prelude::*, diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs index 9ccc75f70219..78ce2866f2b6 100644 --- a/drivers/net/phy/qt2025.rs +++ b/drivers/net/phy/qt2025.rs @@ -9,6 +9,7 @@ //! //! The QT2025 PHY integrates an Intel 8051 micro-controller.
+use core::ffi::CStr; use kernel::error::code; use kernel::firmware::Firmware; use kernel::net::phy::{ diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index 89d961407adb..ff29077d9c2f 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -10,11 +10,11 @@ driver, error::{to_result, Result}, prelude::*, - str::CStr, types::{ForeignOwnable, Opaque}, ThisModule, }; use core::{ + ffi::CStr, marker::PhantomData, ptr::{addr_of_mut, NonNull}, }; diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs index fb0f259cf231..19ac47ffcbd1 100644 --- a/rust/kernel/clk.rs +++ b/rust/kernel/clk.rs @@ -78,10 +78,9 @@ mod common_clk { use crate::{ device::Device, error::{from_err_ptr, to_result, Result}, - prelude::*, };
- use core::{ops::Deref, ptr}; + use core::{ffi::CStr, ops::Deref, ptr};
/// A reference-counted clock. /// diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index 255460d571f6..f76a834d7121 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -118,6 +118,7 @@ use crate::sync::ArcBorrow; use crate::types::Opaque; use core::cell::UnsafeCell; +use core::ffi::CStr; use core::marker::PhantomData;
/// A configfs subsystem. diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs index 0f316dfeb5dd..03eb6babdee2 100644 --- a/rust/kernel/cpufreq.rs +++ b/rust/kernel/cpufreq.rs @@ -26,6 +26,7 @@
use core::{ cell::UnsafeCell, + ffi::CStr, marker::PhantomData, mem::MaybeUninit, ops::{Deref, DerefMut}, @@ -853,7 +854,7 @@ fn register_em(_policy: &mut Policy) { /// /// #[vtable] /// impl cpufreq::Driver for SampleDriver { -/// const NAME: &'static CStr = c"cpufreq-sample"; +/// const NAME: &'static core::ffi::CStr = c"cpufreq-sample"; /// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV; /// const BOOST_ENABLED: bool = true; /// diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index 8cc2d818dca5..e27b42cd3bd5 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -6,10 +6,9 @@
use crate::{ bindings, - str::CStr, types::{ARef, Opaque}, }; -use core::{fmt, marker::PhantomData, ptr}; +use core::{ffi::CStr, fmt, marker::PhantomData, ptr};
#[cfg(CONFIG_PRINTK)] use crate::str::CStrExt as _; diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs index ec9166cedfa7..9926664d9ba2 100644 --- a/rust/kernel/driver.rs +++ b/rust/kernel/driver.rs @@ -6,8 +6,8 @@ //! register using the [`Registration`] class.
use crate::error::{Error, Result}; -use crate::{device, of, str::CStr, try_pin_init, types::Opaque, ThisModule}; -use core::pin::Pin; +use crate::{device, of, try_pin_init, types::Opaque, ThisModule}; +use core::{ffi::CStr, pin::Pin}; use pin_init::{pin_data, pinned_drop, PinInit};
/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform, diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs index acb638086131..4c30933051c7 100644 --- a/rust/kernel/drm/driver.rs +++ b/rust/kernel/drm/driver.rs @@ -10,11 +10,12 @@ drm, error::{to_result, Result}, prelude::*, - str::CStr, types::ARef, }; use macros::vtable;
+use core::ffi::CStr; + /// Driver use the GEM memory manager. This should be set for all modern drivers. pub(crate) const FEAT_GEM: u32 = bindings::drm_driver_feature_DRIVER_GEM;
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 94ef4648827e..1c7784099ea7 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -4,11 +4,9 @@ //! //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h)
-use crate::{ - alloc::{layout::LayoutError, AllocError}, - str::CStr, -}; +use crate::alloc::{layout::LayoutError, AllocError};
+use core::ffi::CStr; use core::fmt; use core::num::NonZeroI32; use core::num::TryFromIntError; diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs index 8a50fcd4c9bb..d9e5cd265101 100644 --- a/rust/kernel/faux.rs +++ b/rust/kernel/faux.rs @@ -7,7 +7,10 @@ //! C header: [`include/linux/device/faux.h`]
use crate::{bindings, device, error::code::*, prelude::*}; -use core::ptr::{addr_of_mut, null, null_mut, NonNull}; +use core::{ + ffi::CStr, + ptr::{addr_of_mut, null, null_mut, NonNull}, +};
/// The registration of a faux device. /// diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index d07849333991..efc80732d5ed 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -4,15 +4,8 @@ //! //! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h)
-use crate::{ - bindings, - device::Device, - error::Error, - error::Result, - ffi, - str::{CStr, CStrExt as _}, -}; -use core::ptr::NonNull; +use crate::{bindings, device::Device, error::Error, error::Result, ffi, str::CStrExt as _}; +use core::{ffi::CStr, ptr::NonNull};
/// # Invariants /// @@ -169,7 +162,7 @@ unsafe impl Sync for Firmware {} /// const DIR: &'static str = "vendor/chip/"; /// const FILES: [&'static str; 3] = [ "foo", "bar", "baz" ]; /// -/// const fn create(module_name: &'static kernel::str::CStr) -> firmware::ModInfoBuilder<N> { +/// const fn create(module_name: &'static core::ffi::CStr) -> firmware::ModInfoBuilder<N> { /// let mut builder = firmware::ModInfoBuilder::new(module_name); /// /// let mut i = 0; @@ -203,7 +196,7 @@ macro_rules! module_firmware { // this macro. Hence, we can neither use `expr` nor `ty`. ($($builder:tt)*) => { const _: () = { - const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) { + const __MODULE_FIRMWARE_PREFIX: &'static ::core::ffi::CStr = if cfg!(MODULE) { c"" } else { <LocalModule as $crate::ModuleMetadata>::NAME diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 1d108e4a6a39..2fe9c7a3611e 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -57,9 +57,9 @@ macro_rules! kunit_assert { break 'out; }
- static FILE: &'static $crate::str::CStr = $file; + static FILE: &'static ::core::ffi::CStr = $file; static LINE: i32 = ::core::line!() as i32 - $diff; - static CONDITION: &'static $crate::str::CStr = + static CONDITION: &'static ::core::ffi::CStr = $crate::str_to_cstr!(stringify!($condition));
// SAFETY: FFI call without safety requirements. @@ -195,7 +195,7 @@ pub fn is_test_result_ok(t: impl TestResult) -> bool { /// Use [`kunit_case_null`] to generate such a delimiter. #[doc(hidden)] pub const fn kunit_case( - name: &'static kernel::str::CStr, + name: &'static core::ffi::CStr, run_case: unsafe extern "C" fn(*mut kernel::bindings::kunit), ) -> kernel::bindings::kunit_case { kernel::bindings::kunit_case { diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index aadcfaa5c759..3a7e935dd1fc 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -166,7 +166,7 @@ fn init(module: &'static ThisModule) -> impl pin_init::PinInit<Self, error::Erro /// Metadata attached to a [`Module`] or [`InPlaceModule`]. pub trait ModuleMetadata { /// The name of the module as specified in the `module!` macro. - const NAME: &'static crate::str::CStr; + const NAME: &'static core::ffi::CStr; }
/// Equivalent to `THIS_MODULE` in the C API. diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index b5b2e3cc158f..91588b0b3985 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -17,10 +17,9 @@ mm::virt::VmaNew, prelude::*, seq_file::SeqFile, - str::CStr, types::{ForeignOwnable, Opaque}, }; -use core::{marker::PhantomData, mem::MaybeUninit, pin::Pin}; +use core::{ffi::CStr, marker::PhantomData, mem::MaybeUninit, pin::Pin};
/// Options for creating a misc device. #[derive(Copy, Clone)] diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 236ea516a134..ce80117b4b48 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -7,7 +7,7 @@ //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
use crate::{error::*, prelude::*, types::Opaque}; -use core::{marker::PhantomData, ptr::addr_of_mut}; +use core::{ffi::CStr, marker::PhantomData, ptr::addr_of_mut};
pub mod reg;
@@ -781,6 +781,7 @@ const fn as_int(&self) -> u32 { /// /// ``` /// # mod module_phy_driver_sample { +/// use core::ffi::CStr; /// use kernel::net::phy::{self, DeviceId}; /// use kernel::prelude::*; /// @@ -808,6 +809,7 @@ const fn as_int(&self) -> u32 { /// This expands to the following code: /// /// ```ignore +/// use core::ffi::CStr; /// use kernel::net::phy::{self, DeviceId}; /// use kernel::prelude::*; /// diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index 5cf50979c1e8..56facfa7a0eb 100644 --- a/rust/kernel/of.rs +++ b/rust/kernel/of.rs @@ -2,7 +2,8 @@
//! Device Tree / Open Firmware abstractions.
-use crate::{bindings, device_id::RawDeviceId, prelude::*}; +use crate::{bindings, device_id::RawDeviceId}; +use core::ffi::CStr;
/// IdTable type for OF drivers. pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>; diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 6b94fd7a3ce9..fb6ee7bb6186 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -13,11 +13,11 @@ error::{to_result, Result}, io::Io, io::IoRaw, - str::CStr, types::{ARef, ForeignOwnable, Opaque}, ThisModule, }; use core::{ + ffi::CStr, marker::PhantomData, ops::Deref, ptr::{addr_of_mut, NonNull}, diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index 99ad0b132ab6..eddae9726d84 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -9,12 +9,12 @@ error::{to_result, Result}, of, prelude::*, - str::CStr, types::{ForeignOwnable, Opaque}, ThisModule, };
use core::{ + ffi::CStr, marker::PhantomData, ptr::{addr_of_mut, NonNull}, }; diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs index 244b660fa835..3f7ca5a95160 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -40,10 +40,7 @@
pub use super::error::{code::*, Error, Result};
-pub use super::{ - str::{CStr, CStrExt as _}, - ThisModule, -}; +pub use super::{str::CStrExt as _, ThisModule};
pub use super::init::InPlaceInit;
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index ff4aaf627179..41529ba41f9f 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -3,6 +3,7 @@ //! String representations.
use crate::alloc::{flags::*, AllocError, KVec}; +use core::ffi::CStr; use core::fmt::{self, Write}; use core::ops::{Deref, DerefMut, Index};
@@ -175,8 +176,6 @@ macro_rules! b_str { }}; }
-pub use core::ffi::CStr; - /// Returns a C pointer to the string. // It is a free function rather than a method on an extension trait because: // @@ -267,7 +266,6 @@ impl crate::fmt::Display for CStr { /// /// ``` /// # use kernel::prelude::fmt; - /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c"🐧"; /// let s = CString::try_from_fmt(fmt!("{}", penguin))?; @@ -376,8 +374,8 @@ fn as_ref(&self) -> &BStr { /// # Examples /// /// ``` +/// # use core::ffi::CStr; /// # use kernel::str_to_cstr; -/// # use kernel::str::CStr; /// const MY_CSTR: &CStr = str_to_cstr!(concat!(file!(), ":", line!(), ": My CStr!")); /// ``` #[macro_export] @@ -387,7 +385,7 @@ macro_rules! str_to_cstr { // too limiting to macro authors, so we rely on the name as a hint instead. ($str:expr) => {{ const S: &str = concat!($str, "\0"); - const C: &$crate::str::CStr = match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) { + const C: &core::ffi::CStr = match core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) { Ok(v) => v, Err(_) => panic!("string contains interior NUL"), }; diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index 0b6bc7f2878d..09bc35feb451 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -8,14 +8,14 @@ use super::{lock::Backend, lock::Guard, LockClassKey}; use crate::{ ffi::{c_int, c_long}, - str::{CStr, CStrExt as _}, + str::CStrExt as _, task::{ MAX_SCHEDULE_TIMEOUT, TASK_FREEZABLE, TASK_INTERRUPTIBLE, TASK_NORMAL, TASK_UNINTERRUPTIBLE, }, time::Jiffies, types::Opaque, }; -use core::{marker::PhantomPinned, pin::Pin, ptr}; +use core::{ffi::CStr, marker::PhantomPinned, pin::Pin, ptr}; use pin_init::{pin_data, pin_init, PinInit};
/// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class. diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index a777a22976e0..21deff0bb13b 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -7,10 +7,10 @@
use super::LockClassKey; use crate::{ - str::{CStr, CStrExt as _}, + str::CStrExt as _, types::{NotThreadSafe, Opaque, ScopeGuard}, }; -use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; +use core::{cell::UnsafeCell, ffi::CStr, marker::PhantomPinned, pin::Pin}; use pin_init::{pin_data, pin_init, PinInit};
pub mod mutex; diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs index 9caa9b419f09..ab5a3947fdd6 100644 --- a/rust/kernel/sync/lock/global.rs +++ b/rust/kernel/sync/lock/global.rs @@ -5,13 +5,14 @@ //! Support for defining statics containing locks.
use crate::{ - str::{CStr, CStrExt as _}, + str::CStrExt as _, sync::lock::{Backend, Guard, Lock}, sync::{LockClassKey, LockedBy}, types::Opaque, }; use core::{ cell::UnsafeCell, + ffi::CStr, marker::{PhantomData, PhantomPinned}, pin::Pin, }; @@ -267,7 +268,7 @@ macro_rules! global_lock { $pub enum $name {}
impl $crate::sync::lock::GlobalLockBackend for $name { - const NAME: &'static $crate::str::CStr = + const NAME: &'static ::core::ffi::CStr = $crate::str_to_cstr!(::core::stringify!($name)); type Item = $valuety; type Backend = $crate::global_lock_inner!(backend $kind); diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs index 339ab6097be7..b5340a904f4a 100644 --- a/rust/kernel/sync/poll.rs +++ b/rust/kernel/sync/poll.rs @@ -11,6 +11,7 @@ sync::{CondVar, LockClassKey}, types::Opaque, }; +use core::ffi::CStr; use core::ops::Deref;
/// Creates a [`PollCondVar`] initialiser with the given name and a newly-created lock class. diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index 432624c69c72..a87e2311ad7c 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -135,6 +135,7 @@
use crate::alloc::{AllocError, Flags}; use crate::{prelude::*, sync::Arc, sync::LockClassKey, types::Opaque}; +use core::ffi::CStr; use core::marker::PhantomData;
/// Creates a [`Work`] initialiser with the given name and a newly-created lock class. diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 532342a38b6f..bf0995674541 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -228,7 +228,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { type LocalModule = {type_};
impl ::kernel::ModuleMetadata for {type_} {{ - const NAME: &'static ::kernel::str::CStr = c"{name}"; + const NAME: &'static ::core::ffi::CStr = c"{name}"; }}
// Double nested modules, since then nobody can access the public items inside. diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs index ea84c23b784b..3f97ab12c39a 100644 --- a/samples/rust/rust_configfs.rs +++ b/samples/rust/rust_configfs.rs @@ -10,6 +10,8 @@ use kernel::prelude::*; use kernel::sync::Mutex;
+use core::ffi::CStr; + module! { type: RustConfigfs, name: "rust_configfs", diff --git a/samples/rust/rust_driver_auxiliary.rs b/samples/rust/rust_driver_auxiliary.rs index eaac5a94f796..4c9447d14914 100644 --- a/samples/rust/rust_driver_auxiliary.rs +++ b/samples/rust/rust_driver_auxiliary.rs @@ -5,10 +5,11 @@ //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
use kernel::{ - auxiliary, bindings, device::Core, driver, error::Error, pci, prelude::*, str::CStr, - InPlaceModule, + auxiliary, bindings, device::Core, driver, error::Error, pci, prelude::*, InPlaceModule, };
+use core::ffi::CStr; + use pin_init::PinInit;
const MODULE_NAME: &CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
On Tue, Jul 1, 2025 at 6:49 PM Tamir Duberstein tamird@gmail.com wrote:
This picks up from Michal Rostecki's work[0]. Per Michal's guidance I have omitted Co-authored tags, as the end result is quite different.
Link: https://lore.kernel.org/rust-for-linux/20240819153656.28807-2-vadorovsky@pro... [0] Closes: https://github.com/Rust-for-Linux/linux/issues/1075
Signed-off-by: Tamir Duberstein tamird@gmail.com
Thanks for keeping this up.
Let's see if we get some Acked-by's -- otherwise, we may want to split by subsystem and avoid flagday changes wherever possible.
Cheers, Miguel
linux-kselftest-mirror@lists.linaro.org