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 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: retitle "Example" section as "Examples" 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 | 2 +- drivers/gpu/drm/drm_panic_qr.rs | 5 +- drivers/gpu/nova-core/driver.rs | 2 +- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 8 +- drivers/net/phy/qt2025.rs | 6 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 9 +- rust/kernel/devres.rs | 2 +- rust/kernel/driver.rs | 4 +- rust/kernel/error.rs | 10 +- rust/kernel/faux.rs | 5 +- rust/kernel/firmware.rs | 16 +- rust/kernel/fmt.rs | 77 +++++++ rust/kernel/kunit.rs | 21 +- 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 | 415 ++++++++++------------------------- rust/kernel/sync.rs | 7 +- 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 | 1 + rust/macros/fmt.rs | 118 ++++++++++ rust/macros/kunit.rs | 6 +- rust/macros/lib.rs | 21 +- rust/macros/module.rs | 2 +- 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 | 2 +- 40 files changed, 426 insertions(+), 395 deletions(-) --- base-commit: cbeaa41dfe26b72639141e87183cb23e00d4b0dd change-id: 20250201-cstr-core-d4b9b69120cf
Best regards,
This title is consistent with all other macros' documentation, regardless of the number of examples contained in their "Examples" sections.
Signed-off-by: Tamir Duberstein tamird@gmail.com --- rust/macros/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 9acaa68c974e..d31e50c446b0 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -263,7 +263,7 @@ pub fn concat_idents(ts: TokenStream) -> TokenStream { /// literals (lifetimes and documentation strings are not supported). There is a difference in /// supported modifiers as well. /// -/// # Example +/// # Examples /// /// ``` /// # const binder_driver_return_protocol_BR_OK: u32 = 0;
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
This title is consistent with all other macros' documentation, regardless of the number of examples contained in their "Examples" sections.
Signed-off-by: Tamir Duberstein tamird@gmail.com
Reviewed-by: Benno Lossin lossin@kernel.org
--- Cheers, Benno
On Sat, May 24, 2025 at 10:33 PM Tamir Duberstein tamird@gmail.com wrote:
This title is consistent with all other macros' documentation, regardless of the number of examples contained in their "Examples" sections.
Signed-off-by: Tamir Duberstein tamird@gmail.com
I was going to say that I could take this one independently, but we already had #1 of:
https://lore.kernel.org/rust-for-linux/20240906164448.2268368-1-paddymills@p...
I will take that one (which given the `checkpatch.pl` one got stalled, I should have taken it separately as I mentioned at some point).
Patrick/Hridesh: there are new cases arriving (i.e. singular section names), so it would be great if the `checkpatch.pl` patch discussion could be restarted to see if we can land it, i.e. there is now even more justification behind it just after some months. Thanks!
Cheers, Miguel
On Mon, May 26, 2025 at 12:15 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Sat, May 24, 2025 at 10:33 PM Tamir Duberstein tamird@gmail.com wrote:
This title is consistent with all other macros' documentation, regardless of the number of examples contained in their "Examples" sections.
Signed-off-by: Tamir Duberstein tamird@gmail.com
I was going to say that I could take this one independently, but we already had #1 of:
https://lore.kernel.org/rust-for-linux/20240906164448.2268368-1-paddymills@proton.me/I will take that one (which given the `checkpatch.pl` one got stalled, I should have taken it separately as I mentioned at some point).
Sounds good. I'll drop this one.
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ 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 | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 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/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index fb0f393c1cea..842be88aa1cf 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 5c372cf27ed0..99d99a76934c 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -240,7 +240,7 @@ impl DeviceContext for Normal {} 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..12b08debc3b3 --- /dev/null +++ b/rust/kernel/fmt.rs @@ -0,0 +1,77 @@ +// 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); + +macro_rules! impl_display_forward { + ($( + $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )? + ),* $(,)?) => { + $( + impl$($($generics)*)? fmt::Display for Adapter<&$ty> + $(where $($where)*)? { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(t) = self; + fmt::Display::fmt(t, f) + } + } + )* + }; +} + +impl<T: ?Sized> fmt::Display for Adapter<&&T> +where + for<'a> Adapter<&'a T>: fmt::Display, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(t) = self; + Adapter::<&T>(**t).fmt(f) + } +} + +impl_display_forward!( + bool, + char, + core::panic::PanicInfo<'_>, + crate::str::BStr, + 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 1604fb6a5b1b..c29e34192553 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -72,14 +72,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 6e9287136cac..ec48c818d512 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -66,6 +66,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 baa774a351ce..ef1efcb9d945 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -17,7 +17,7 @@ pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)] -pub use macros::{export, module, vtable}; +pub use macros::{export, fmt, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
@@ -26,7 +26,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 9783d960a97a..0f5e15128005 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 7a9403eb6e5b..627bc2f7b3d2 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 98d5c74ec4f7..302423ca5eb0 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -54,7 +54,7 @@ impl 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 fmt::Display for crate::fmt::Adapter<&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!("🐧"); @@ -442,7 +442,8 @@ impl fmt::Display for CStr { /// # Ok::<(), kernel::error::Error>(()) /// ``` fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - for &c in self.as_bytes() { + let Self(cstr) = self; + for &c in cstr.as_bytes() { if (0x20..0x7f).contains(&c) { // Printable character. f.write_char(c as char)?; @@ -459,7 +460,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!("🐧"); @@ -595,7 +596,7 @@ fn deref(&self) -> &str {
macro_rules! format { ($($f:tt)*) => ({ - &*String::from_fmt(kernel::fmt!($($f)*)) + &*String::from_fmt(crate::prelude::fmt!($($f)*)) }) }
@@ -850,7 +851,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()); @@ -940,9 +941,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..6b6bd9295d18 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 + +use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, 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; + } + while let Some((name, rest)) = first_str.split_once('}') { + first_str = rest; + if let Some(rest) = first_str.strip_prefix('}') { + first_str = rest; + continue; + } + 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); + } + break; + } + } + first_lit + }; + + let first_span = first_lit.span(); + let adapt = |expr| { + let mut borrow = + TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]); + borrow.extend(expr); + make_ident(first_span, ["kernel", "fmt", "Adapter"]) + .chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))]) + }; + + let flush = |args: &mut TokenStream, current: &mut TokenStream| { + let current = std::mem::take(current); + if !current.is_empty() { + args.extend(adapt(current)); + } + }; + + let mut args = TokenStream::from_iter(first_opt); + { + let mut current = TokenStream::new(); + for tt in input { + match &tt { + TokenTree::Punct(p) => match p.as_char() { + ',' => { + flush(&mut args, &mut current); + &mut args + } + '=' => { + names.remove(current.to_string().as_str()); + args.extend(std::mem::take(&mut current)); + &mut args + } + _ => &mut current, + }, + _ => &mut current, + } + .extend([tt]); + } + flush(&mut args, &mut current); + } + + for name in names { + args.extend( + [ + TokenTree::Punct(Punct::new(',', Spacing::Alone)), + TokenTree::Ident(Ident::new(name, first_span)), + TokenTree::Punct(Punct::new('=', Spacing::Alone)), + ] + .into_iter() + .chain(adapt(TokenTree::Ident(Ident::new(name, first_span)).into())), + ); + } + + TokenStream::from_iter(make_ident(first_span, ["core", "format_args"]).chain([ + TokenTree::Punct(Punct::new('!', Spacing::Alone)), + TokenTree::Group(Group::new(Delimiter::Parenthesis, args)), + ])) +} + +fn make_ident<'a, T: IntoIterator<Item = &'a str>>( + span: Span, + names: T, +) -> impl Iterator<Item = TokenTree> + use<'a, T> { + names.into_iter().flat_map(move |name| { + [ + TokenTree::Punct(Punct::new(':', Spacing::Joint)), + TokenTree::Punct(Punct::new(':', Spacing::Alone)), + TokenTree::Ident(Ident::new(name, span)), + ] + }) +} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,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/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index ec8d70ac888b..22ed9ee14053 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -197,7 +197,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 Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ 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 | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 deletions(-)
Can you split this into creating the proc-macro, forwarding the display impls and replacing all the uses with the proc macro?
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/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index fb0f393c1cea..842be88aa1cf 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 5c372cf27ed0..99d99a76934c 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -240,7 +240,7 @@ impl DeviceContext for Normal {} 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..12b08debc3b3 --- /dev/null +++ b/rust/kernel/fmt.rs @@ -0,0 +1,77 @@ +// 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);
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?
You don't need `{}` around the `where` clause, as a `where` keyword can follow a `ty` fragment.
- ),* $(,)?) => {
$(impl$($($generics)*)? fmt::Display for Adapter<&$ty>$(where $($where)*)? {fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {let Self(t) = self;fmt::Display::fmt(t, f)}})*- };
+}
+impl<T: ?Sized> fmt::Display for Adapter<&&T> +where
- for<'a> Adapter<&'a T>: fmt::Display,
+{
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;Adapter::<&T>(**t).fmt(f)- }
+}
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }
diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..6b6bd9295d18 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0
+use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, 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();};
This usage of let-else + match is pretty confusing and could just be a single match statement.
while let Some((_, rest)) = first_str.split_once('{') {first_str = rest;if let Some(rest) = first_str.strip_prefix('{') {first_str = rest;continue;}while let Some((name, rest)) = first_str.split_once('}') {first_str = rest;if let Some(rest) = first_str.strip_prefix('}') {
This doesn't make sense, we've matched a `{`, some text and a `}`. You can't escape a `}` that is associated to a `{`.
first_str = rest;continue;}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);}break;}}first_lit
`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])
This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))
- };
- let flush = |args: &mut TokenStream, current: &mut TokenStream| {
let current = std::mem::take(current);if !current.is_empty() {args.extend(adapt(current));}- };
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut current = TokenStream::new();for tt in input {match &tt {TokenTree::Punct(p) => match p.as_char() {',' => {flush(&mut args, &mut current);&mut args}'=' => {names.remove(current.to_string().as_str());args.extend(std::mem::take(&mut current));&mut args}_ => &mut current,},_ => &mut current,}.extend([tt]);}
This doesn't handle the following code correctly ):
let mut a = 0; pr_info!("{a:?}", a = a = a);
Looks like we'll have to remember what "kind" of an equals we parsed...
flush(&mut args, &mut current);- }
- for name in names {
args.extend([TokenTree::Punct(Punct::new(',', Spacing::Alone)),TokenTree::Ident(Ident::new(name, first_span)),TokenTree::Punct(Punct::new('=', Spacing::Alone)),].into_iter().chain(adapt(TokenTree::Ident(Ident::new(name, first_span)).into())),);
This can probably be:
let name = Ident::new(name, first_span); let value = adapt(name.clone()); args.extend(quote!(, #name = #value));
- }
- TokenStream::from_iter(make_ident(first_span, ["core", "format_args"]).chain([
TokenTree::Punct(Punct::new('!', Spacing::Alone)),TokenTree::Group(Group::new(Delimiter::Parenthesis, args)),- ]))
This can be:
quote!(::core::format_args!(#args))
(not sure if you need `#(#args)*`)
+}
+fn make_ident<'a, T: IntoIterator<Item = &'a str>>(
- span: Span,
- names: T,
+) -> impl Iterator<Item = TokenTree> + use<'a, T> {
- names.into_iter().flat_map(move |name| {
[TokenTree::Punct(Punct::new(':', Spacing::Joint)),TokenTree::Punct(Punct::new(':', Spacing::Alone)),TokenTree::Ident(Ident::new(name, span)),]- })
+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
--- Cheers, Benno
- fmt::fmt(input)
+}
/// Concatenate two identifiers. /// /// This is useful in macros that need to declare or reference items with names diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index ec8d70ac888b..22ed9ee14053 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -197,7 +197,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 Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ 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 | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 deletions(-)
Can you split this into creating the proc-macro, forwarding the display impls and replacing all the uses with the proc macro?
Can you help me understand why that's better?
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/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs index fb0f393c1cea..842be88aa1cf 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 5c372cf27ed0..99d99a76934c 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -240,7 +240,7 @@ impl DeviceContext for Normal {} 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..12b08debc3b3 --- /dev/null +++ b/rust/kernel/fmt.rs @@ -0,0 +1,77 @@ +// 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);
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?You don't need `{}` around the `where` clause, as a `where` keyword can follow a `ty` fragment.
This doesn't work: ``` error: local ambiguity when calling macro `impl_display_forward`: multiple parsing options: built-in NTs tt ('r#where') or 2 other options. --> rust/kernel/fmt.rs:75:78 | 75 | {<T: ?Sized>} crate::sync::Arc<T> where crate::sync::Arc<T>: fmt::Display, | ^ ```
- ),* $(,)?) => {
$(impl$($($generics)*)? fmt::Display for Adapter<&$ty>$(where $($where)*)? {fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {let Self(t) = self;fmt::Display::fmt(t, f)}})*- };
+}
+impl<T: ?Sized> fmt::Display for Adapter<&&T> +where
- for<'a> Adapter<&'a T>: fmt::Display,
+{
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let Self(t) = self;Adapter::<&T>(**t).fmt(f)- }
+}
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }
Is that formatting better? rustfmt refuses to touch it either way.
diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs new file mode 100644 index 000000000000..6b6bd9295d18 --- /dev/null +++ b/rust/macros/fmt.rs @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0
+use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, 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();};This usage of let-else + match is pretty confusing and could just be a single match statement.
I don't think so. Can you try rewriting it into the form you like?
while let Some((_, rest)) = first_str.split_once('{') {first_str = rest;if let Some(rest) = first_str.strip_prefix('{') {first_str = rest;continue;}while let Some((name, rest)) = first_str.split_once('}') {first_str = rest;if let Some(rest) = first_str.strip_prefix('}') {This doesn't make sense, we've matched a `{`, some text and a `}`. You can't escape a `}` that is associated to a `{`.
Sure, but such input would be malformed, so I don't think it's necessary to handle it "perfectly". We'll get a nice error from format_args anyhow.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&a...
first_str = rest;continue;}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);}break;}}first_lit`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))
Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.
- };
- let flush = |args: &mut TokenStream, current: &mut TokenStream| {
let current = std::mem::take(current);if !current.is_empty() {args.extend(adapt(current));}- };
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut current = TokenStream::new();for tt in input {match &tt {TokenTree::Punct(p) => match p.as_char() {',' => {flush(&mut args, &mut current);&mut args}'=' => {names.remove(current.to_string().as_str());args.extend(std::mem::take(&mut current));&mut args}_ => &mut current,},_ => &mut current,}.extend([tt]);}This doesn't handle the following code correctly ):
let mut a = 0; pr_info!("{a:?}", a = a = a);Looks like we'll have to remember what "kind" of an equals we parsed...
Hmm, good point. Maybe we can just avoid dealing with `=` at all until we hit the `,` and just split on the leftmost `=`. WDYT? I'll have that in v11.
flush(&mut args, &mut current);- }
- for name in names {
args.extend([TokenTree::Punct(Punct::new(',', Spacing::Alone)),TokenTree::Ident(Ident::new(name, first_span)),TokenTree::Punct(Punct::new('=', Spacing::Alone)),].into_iter().chain(adapt(TokenTree::Ident(Ident::new(name, first_span)).into())),);This can probably be:
let name = Ident::new(name, first_span); let value = adapt(name.clone()); args.extend(quote!(, #name = #value));
Indeed, see above - manual construction will be gone in v11.
- }
- TokenStream::from_iter(make_ident(first_span, ["core", "format_args"]).chain([
TokenTree::Punct(Punct::new('!', Spacing::Alone)),TokenTree::Group(Group::new(Delimiter::Parenthesis, args)),- ]))
This can be:
quote!(::core::format_args!(#args))(not sure if you need `#(#args)*`)
+}
+fn make_ident<'a, T: IntoIterator<Item = &'a str>>(
- span: Span,
- names: T,
+) -> impl Iterator<Item = TokenTree> + use<'a, T> {
- names.into_iter().flat_map(move |name| {
[TokenTree::Punct(Punct::new(':', Spacing::Joint)),TokenTree::Punct(Punct::new(':', Spacing::Alone)),TokenTree::Ident(Ident::new(name, span)),]- })
+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.
Cheers, Benno
- fmt::fmt(input)
+}
/// Concatenate two identifiers. /// /// This is useful in macros that need to declare or reference items with names diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs index ec8d70ac888b..22ed9ee14053 100644 --- a/scripts/rustdoc_test_gen.rs +++ b/scripts/rustdoc_test_gen.rs @@ -197,7 +197,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 May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ 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 | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 deletions(-)
Can you split this into creating the proc-macro, forwarding the display impls and replacing all the uses with the proc macro?
Can you help me understand why that's better?
It makes reviewing significantly easier.
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?You don't need `{}` around the `where` clause, as a `where` keyword can follow a `ty` fragment.
This doesn't work:
error: local ambiguity when calling macro `impl_display_forward`: multiple parsing options: built-in NTs tt ('r#where') or 2 other options. --> rust/kernel/fmt.rs:75:78 | 75 | {<T: ?Sized>} crate::sync::Arc<T> where crate::sync::Arc<T>: fmt::Display, | ^
Ah right that's a shame, forgot about the `tt`s at the end...
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }Is that formatting better? rustfmt refuses to touch it either way.
Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think it's better.
+/// 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();};This usage of let-else + match is pretty confusing and could just be a single match statement.
I don't think so. Can you try rewriting it into the form you like?
let (mut first_str, first_lit) match first_opt.as_ref() { Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => { let contents = lit.to_string(); let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap(); ((contents, lit)) } _ => 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;}while let Some((name, rest)) = first_str.split_once('}') {first_str = rest;if let Some(rest) = first_str.strip_prefix('}') {This doesn't make sense, we've matched a `{`, some text and a `}`. You can't escape a `}` that is associated to a `{`.
Sure, but such input would be malformed, so I don't think it's necessary to handle it "perfectly". We'll get a nice error from format_args anyhow.
My suggestion in this case would be to just remove this if-let. The search for `{` above would skip the `}` if it's correct.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&a...
Yes it will error like that, but if we do the replacement only when the syntax is correct, there also will be compile errors because of a missing `Display` impl, or is that not the case?
I'm a bit concerned about the ergonomics that this change will introduce, but I guess there really isn't anything that we can do about except not do it.
first_str = rest;continue;}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);}break;}}first_lit`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.
I don't see how this is hard, just do:
let (first_str, first_lit) = ...;
while ...
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.
I don't think that you need `quote_spanned` here at all. If you do, then let me know, something weird with spans is going on then.
- };
- let flush = |args: &mut TokenStream, current: &mut TokenStream| {
let current = std::mem::take(current);if !current.is_empty() {args.extend(adapt(current));}- };
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut current = TokenStream::new();for tt in input {match &tt {TokenTree::Punct(p) => match p.as_char() {',' => {flush(&mut args, &mut current);&mut args}'=' => {names.remove(current.to_string().as_str());args.extend(std::mem::take(&mut current));&mut args}_ => &mut current,},_ => &mut current,}.extend([tt]);}This doesn't handle the following code correctly ):
let mut a = 0; pr_info!("{a:?}", a = a = a);Looks like we'll have to remember what "kind" of an equals we parsed...
Hmm, good point. Maybe we can just avoid dealing with `=` at all until we hit the `,` and just split on the leftmost `=`. WDYT? I'll have that in v11.
Sounds good, if there is no `=`, then ignore it.
+/// 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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.
Ahh that's unfortunate.
--- Cheers, Benno
On Mon, May 26, 2025 at 7:01 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Introduce a `fmt!` macro which wraps all arguments in `kernel::fmt::Adapter` This enables formatting of foreign types (like `core::ffi::CStr`) that do not implement `fmt::Display` due to concerns around lossy conversions which do not apply in the kernel.
Replace all direct calls to `format_args!` with `fmt!`.
In preparation for replacing our `CStr` with `core::ffi::CStr`, move its `fmt::Display` implementation to `kernel::fmt::Adapter<&CStr>`.
Suggested-by: Alice Ryhl aliceryhl@google.com Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Cu... Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/block/rnull.rs | 2 +- rust/kernel/block/mq.rs | 2 +- rust/kernel/device.rs | 2 +- rust/kernel/fmt.rs | 77 +++++++++++++++++++++++++++++ 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 | 23 ++++----- rust/macros/fmt.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 19 +++++++ scripts/rustdoc_test_gen.rs | 2 +- 13 files changed, 235 insertions(+), 26 deletions(-)
Can you split this into creating the proc-macro, forwarding the display impls and replacing all the uses with the proc macro?
Can you help me understand why that's better?
It makes reviewing significantly easier.
+macro_rules! impl_display_forward {
- ($(
$( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?You don't need `{}` around the `where` clause, as a `where` keyword can follow a `ty` fragment.
This doesn't work:
error: local ambiguity when calling macro `impl_display_forward`: multiple parsing options: built-in NTs tt ('r#where') or 2 other options. --> rust/kernel/fmt.rs:75:78 | 75 | {<T: ?Sized>} crate::sync::Arc<T> where crate::sync::Arc<T>: fmt::Display, | ^Ah right that's a shame, forgot about the `tt`s at the end...
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }Is that formatting better? rustfmt refuses to touch it either way.
Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think it's better.
OK, but why? This seems entirely subjective.
+/// 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();};This usage of let-else + match is pretty confusing and could just be a single match statement.
I don't think so. Can you try rewriting it into the form you like?
let (mut first_str, first_lit) match first_opt.as_ref() { Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => { let contents = lit.to_string(); let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap(); ((contents, lit)) } _ => return first_opt.into_iter().chain(input).collect(), };
What happens if the invocation is utterly malformed, e.g. `fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
while let Some((_, rest)) = first_str.split_once('{') {first_str = rest;if let Some(rest) = first_str.strip_prefix('{') {first_str = rest;continue;}while let Some((name, rest)) = first_str.split_once('}') {first_str = rest;if let Some(rest) = first_str.strip_prefix('}') {This doesn't make sense, we've matched a `{`, some text and a `}`. You can't escape a `}` that is associated to a `{`.
Sure, but such input would be malformed, so I don't think it's necessary to handle it "perfectly". We'll get a nice error from format_args anyhow.
My suggestion in this case would be to just remove this if-let. The search for `{` above would skip the `}` if it's correct.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&a...
Makes sense to me.
Yes it will error like that, but if we do the replacement only when the syntax is correct, there also will be compile errors because of a missing `Display` impl, or is that not the case?
I'm not sure - I would guess syntax errors "mask" typeck errors.
I'm a bit concerned about the ergonomics that this change will introduce, but I guess there really isn't anything that we can do about except not do it.
first_str = rest;continue;}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);}break;}}first_lit`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.
I don't see how this is hard, just do:
let (first_str, first_lit) = ...;
It requires you to unwrap, like you did above, which is what I'm trying to avoid.
while ...
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.
I don't think that you need `quote_spanned` here at all. If you do, then let me know, something weird with spans is going on then.
You need to give idents a span, so each of `kernel`, `fmt`, and `adapter` need a span. I *could* use `quote!` and get whatever span it uses (mixed_site) but I'd rather retain control.
- };
- let flush = |args: &mut TokenStream, current: &mut TokenStream| {
let current = std::mem::take(current);if !current.is_empty() {args.extend(adapt(current));}- };
- let mut args = TokenStream::from_iter(first_opt);
- {
let mut current = TokenStream::new();for tt in input {match &tt {TokenTree::Punct(p) => match p.as_char() {',' => {flush(&mut args, &mut current);&mut args}'=' => {names.remove(current.to_string().as_str());args.extend(std::mem::take(&mut current));&mut args}_ => &mut current,},_ => &mut current,}.extend([tt]);}This doesn't handle the following code correctly ):
let mut a = 0; pr_info!("{a:?}", a = a = a);Looks like we'll have to remember what "kind" of an equals we parsed...
Hmm, good point. Maybe we can just avoid dealing with `=` at all until we hit the `,` and just split on the leftmost `=`. WDYT? I'll have that in v11.
Sounds good, if there is no `=`, then ignore it.
+/// 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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.
Ahh that's unfortunate.
Cheers, Benno
On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 7:01 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }Is that formatting better? rustfmt refuses to touch it either way.
Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think it's better.
OK, but why? This seems entirely subjective.
If more types are added to the list, it will grow over one screen size. With my formatting, leaving related types on a single line, that will only happen much later.
+/// 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();};This usage of let-else + match is pretty confusing and could just be a single match statement.
I don't think so. Can you try rewriting it into the form you like?
let (mut first_str, first_lit) match first_opt.as_ref() { Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => { let contents = lit.to_string(); let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap(); ((contents, lit)) } _ => return first_opt.into_iter().chain(input).collect(), };What happens if the invocation is utterly malformed, e.g. `fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
That example won't even survive lexing (macros always will get valid rust tokens as input). If a literal begins with a `"`, it also will end with one AFAIK.
Yes it will error like that, but if we do the replacement only when the syntax is correct, there also will be compile errors because of a missing `Display` impl, or is that not the case?
I'm not sure - I would guess syntax errors "mask" typeck errors.
I checked and it seems to be so, that's good.
first_str = rest;continue;}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);}break;}}first_lit`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.
I don't see how this is hard, just do:
let (first_str, first_lit) = ...;It requires you to unwrap, like you did above, which is what I'm trying to avoid.
How so? What do you need to unwrap?
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.
I don't think that you need `quote_spanned` here at all. If you do, then let me know, something weird with spans is going on then.
You need to give idents a span, so each of `kernel`, `fmt`, and `adapter` need a span. I *could* use `quote!` and get whatever span it uses (mixed_site) but I'd rather retain control.
Please use `quote!` if it works. No need to make this more complex than it already is. If it doesn't work then that's another story.
--- Cheers, Benno
On Tue, May 27, 2025 at 4:49 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 5:02 PM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 7:01 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:17 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:48 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+impl_display_forward!(
- bool,
- char,
- core::panic::PanicInfo<'_>,
- crate::str::BStr,
- 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},
+);
If we use `{}` instead of `()`, then we can format the contents differently:
impl_display_forward! { i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, bool, char, str, crate::str::BStr, fmt::Arguments<'_>, core::panic::PanicInfo<'_>, {<T: ?Sized>} crate::sync::Arc<T> {where Self: fmt::Display}, {<T: ?Sized>} crate::sync::UniqueArc<T> {where Self: fmt::Display}, }Is that formatting better? rustfmt refuses to touch it either way.
Yeah rustfmt doesn't touch macro parameters enclosed in `{}`. I think it's better.
OK, but why? This seems entirely subjective.
If more types are added to the list, it will grow over one screen size. With my formatting, leaving related types on a single line, that will only happen much later.
+/// 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();};This usage of let-else + match is pretty confusing and could just be a single match statement.
I don't think so. Can you try rewriting it into the form you like?
let (mut first_str, first_lit) match first_opt.as_ref() { Some(TokenTree::Literal(lit)) if lit.to_string().starts_with('"') => { let contents = lit.to_string(); let contents = contents.strip_prefix('"').unwrap().strip_suffix('"').unwrap(); ((contents, lit)) } _ => return first_opt.into_iter().chain(input).collect(), };What happens if the invocation is utterly malformed, e.g. `fmt!("hello)`? You're unwrapping here, which I intentionally avoid.
That example won't even survive lexing (macros always will get valid rust tokens as input). If a literal begins with a `"`, it also will end with one AFAIK.
Yes it will error like that, but if we do the replacement only when the syntax is correct, there also will be compile errors because of a missing `Display` impl, or is that not the case?
I'm not sure - I would guess syntax errors "mask" typeck errors.
I checked and it seems to be so, that's good.
👍
first_str = rest;continue;}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);}break;}}first_lit`first_lit` is not modified, so could we just the code above it into a block instead of keeping it in the expr for `first_lit`?
As above, can you suggest the alternate form you like better? The gymnastics here are all in service of being able to let malformed input fall through to core::format_args which will do the hard work of producing good diagnostics.
I don't see how this is hard, just do:
let (first_str, first_lit) = ...;It requires you to unwrap, like you did above, which is what I'm trying to avoid.
How so? What do you need to unwrap?
I was referring to your unwraps above.
- };
- let first_span = first_lit.span();
- let adapt = |expr| {
let mut borrow =TokenStream::from_iter([TokenTree::Punct(Punct::new('&', Spacing::Alone))]);borrow.extend(expr);make_ident(first_span, ["kernel", "fmt", "Adapter"]).chain([TokenTree::Group(Group::new(Delimiter::Parenthesis, borrow))])This should be fine with using `quote!`:
quote!(::kernel::fmt::Adapter(&#expr))Yeah, I have a local commit that uses quote_spanned to remove all the manual constructions.
I don't think that you need `quote_spanned` here at all. If you do, then let me know, something weird with spans is going on then.
You need to give idents a span, so each of `kernel`, `fmt`, and `adapter` need a span. I *could* use `quote!` and get whatever span it uses (mixed_site) but I'd rather retain control.
Please use `quote!` if it works. No need to make this more complex than it already is. If it doesn't work then that's another story.
Let's adjudicate that on v11, where you can see the code.
On Tue, May 27, 2025 at 12:18 AM Tamir Duberstein tamird@gmail.com wrote:
+}
+fn make_ident<'a, T: IntoIterator<Item = &'a str>>(
- span: Span,
- names: T,
+) -> impl Iterator<Item = TokenTree> + use<'a, T> {
- names.into_iter().flat_map(move |name| {
[TokenTree::Punct(Punct::new(':', Spacing::Joint)),TokenTree::Punct(Punct::new(':', Spacing::Alone)),TokenTree::Ident(Ident::new(name, span)),]- })
+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.
I'm pretty sure that glob imports are higher priority than the core prelude? Or is this because there are macros that now incorrectly use kernel::prelude::format_args when they should use the one from core?
Alice
On Tue, May 27, 2025 at 8:44 AM Alice Ryhl aliceryhl@google.com wrote:
On Tue, May 27, 2025 at 12:18 AM Tamir Duberstein tamird@gmail.com wrote:
+}
+fn make_ident<'a, T: IntoIterator<Item = &'a str>>(
- span: Span,
- names: T,
+) -> impl Iterator<Item = TokenTree> + use<'a, T> {
- names.into_iter().flat_map(move |name| {
[TokenTree::Punct(Punct::new(':', Spacing::Joint)),TokenTree::Punct(Punct::new(':', Spacing::Alone)),TokenTree::Ident(Ident::new(name, span)),]- })
+} diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index d31e50c446b0..fa956eaa3ba7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -10,6 +10,7 @@ mod quote; mod concat_idents; mod export; +mod fmt; mod helpers; mod kunit; mod module; @@ -196,6 +197,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 {
I'm wondering if we should name this `format_args` instead in order to better communicate that it's a replacement for `core::format_args!`.
Unfortunately that introduces ambiguity in cases where kernel::prelude::* is imported because core::format_args is in core's prelude.
I'm pretty sure that glob imports are higher priority than the core prelude? Or is this because there are macros that now incorrectly use kernel::prelude::format_args when they should use the one from core?
compiler says no, e.g.:
error[E0659]: `format_args` is ambiguous --> rust/doctests_kernel_generated.rs:8783:25 | 8783 | kernel::kunit::info(format_args!(" # rust_doctest_kernel_workqueue_rs_3.location: rust/kernel/workqueue.rs:77\n")); | ^^^^^^^^^^^ ambiguous name | = note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution = note: `format_args` could refer to a macro from prelude note: `format_args` could also refer to the macro imported here --> rust/doctests_kernel_generated.rs:8772:9 | 8772 | use kernel::prelude::*; | ^^^^^^^^^^^^^^^^^^ = help: consider adding an explicit import of `format_args` to disambiguate
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64. Replace `kernel::str::CStr` with `core::ffi::CStr` now that we can.
C-String literals were added in Rust 1.77. Opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary; the rest will be done in a later commit.
Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/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 | 358 +++++++++------------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- 14 files changed, 112 insertions(+), 294 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index f2a99681b998..d8192a9bef63 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -922,7 +922,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/device.rs b/rust/kernel/device.rs index 99d99a76934c..9074322c79e8 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -12,7 +12,7 @@ use core::{fmt, ptr};
#[cfg(CONFIG_PRINTK)] -use crate::c_str; +use crate::str::CStrExt as _;
/// A reference-counted device. /// @@ -176,7 +176,7 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) { bindings::_dev_printk( klevel as *const _ as *const crate::ffi::c_char, self.as_raw(), - c_str!("%pA").as_char_ptr(), + c"%pA".as_char_ptr(), &msg as *const _ as *const crate::ffi::c_void, ) }; diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 3dee3139fcd4..933c048c04f1 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 2494c96e105f..582ab648b14c 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 @@ -290,7 +297,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 c29e34192553..e5621d596ed3 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -100,12 +100,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, });
@@ -175,7 +175,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 b4c5f74de23d..d684ec4ef4d0 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -34,7 +34,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 _; - 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 a59469c785e3..652e060e47bd 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -504,7 +504,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 04f2d8ef29cb..12ea65df46de 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 ef1efcb9d945..97e8bcf73669 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -35,7 +35,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 627bc2f7b3d2..1011743dd0ce 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(), &args as *const _ as *const crate::ffi::c_void, ); } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 302423ca5eb0..586644912414 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::error::{code::*, Error};
@@ -57,11 +57,11 @@ impl 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 as _, 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 *(bytes as *mut [u8] 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 fmt::Display for crate::fmt::Adapter<&CStr> { @@ -434,16 +272,16 @@ impl fmt::Display for crate::fmt::Adapter<&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 { let Self(cstr) = self; - for &c in cstr.as_bytes() { + for &c in cstr.to_bytes() { if (0x20..0x7f).contains(&c) { // Printable character. f.write_char(c as char)?; @@ -455,98 +293,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 *(s as *mut CStr 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 *(bytes as *mut [u8] 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()) } }
@@ -630,15 +445,6 @@ fn test_cstr_to_str_panic() { checked_cstr.to_str().unwrap(); }
- #[test] - fn test_cstr_as_str_unchecked() { - let good_bytes = b"\xf0\x9f\x90\xA7\0"; - let checked_cstr = CStr::from_bytes_with_nul(good_bytes).unwrap(); - // 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, "🐧"); - } - #[test] fn test_cstr_display() { let hello_world = CStr::from_bytes_with_nul(b"hello, world!\0").unwrap(); @@ -854,11 +660,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)); @@ -928,7 +734,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,
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64. Replace `kernel::str::CStr` with `core::ffi::CStr` now that we can.
What's this supposed to mean?
C-String literals were added in Rust 1.77. Opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary; the rest will be done in a later commit.
Similarly this, the message should explain the motivation for the change, the change itself and can include additional information.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/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 | 358 +++++++++------------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- 14 files changed, 112 insertions(+), 294 deletions(-)
I'm a bit confused by some of the diffs here, they seem pretty messy, any chance that they can be improved?
--- Cheers, Benno
On Mon, May 26, 2025 at 10:56 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64. Replace `kernel::str::CStr` with `core::ffi::CStr` now that we can.
What's this supposed to mean?
It means that kernel::str::CStr was introduced before core::ffi:CStr was available. I didn't check this before, but it is indeed true - see https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c....
C-String literals were added in Rust 1.77. Opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary; the rest will be done in a later commit.
Similarly this, the message should explain the motivation for the change, the change itself and can include additional information.
The motivation is implied (that using standard types is preferable to having custom ones; this is also implicit rather than explicit in https://github.com/Rust-for-Linux/linux/issues/1075), but I can sharpen it.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/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 | 358 +++++++++------------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- 14 files changed, 112 insertions(+), 294 deletions(-)
I'm a bit confused by some of the diffs here, they seem pretty messy, any chance that they can be improved?
I'm open to suggestions. I think the confusion arises from git trying to keep code from moving; fundamentally much of the change is moving methods to an extension trait, which means git has to choose between keeping the documentation where it is, or keeping the implementation where it is. If I use `--patience` then everything moves together, but then the diffstat swells. Thoughts?
On Tue May 27, 2025 at 12:24 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:56 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64. Replace `kernel::str::CStr` with `core::ffi::CStr` now that we can.
What's this supposed to mean?
It means that kernel::str::CStr was introduced before core::ffi:CStr was available. I didn't check this before, but it is indeed true - see https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c....
I see, then just write that and mention the commit.
C-String literals were added in Rust 1.77. Opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary; the rest will be done in a later commit.
Similarly this, the message should explain the motivation for the change, the change itself and can include additional information.
The motivation is implied (that using standard types is preferable to having custom ones; this is also implicit rather than explicit in https://github.com/Rust-for-Linux/linux/issues/1075), but I can sharpen it.
Please add this information to the commit message.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 2 +- rust/kernel/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 | 358 +++++++++------------------------------- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 2 +- rust/kernel/sync/lock/global.rs | 2 +- 14 files changed, 112 insertions(+), 294 deletions(-)
I'm a bit confused by some of the diffs here, they seem pretty messy, any chance that they can be improved?
I'm open to suggestions. I think the confusion arises from git trying to keep code from moving; fundamentally much of the change is moving methods to an extension trait, which means git has to choose between keeping the documentation where it is, or keeping the implementation where it is. If I use `--patience` then everything moves together, but then the diffstat swells. Thoughts?
I haven't viewed this patch with color-moved (since I haven't applied it locally), I can try that first and see if it helps.
--- Cheers, Benno
On Mon, May 26, 2025 at 7:03 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:24 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 10:56 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
`std::ffi::CStr` was moved to `core::ffi::CStr` in Rust 1.64. Replace `kernel::str::CStr` with `core::ffi::CStr` now that we can.
What's this supposed to mean?
It means that kernel::str::CStr was introduced before core::ffi:CStr was available. I didn't check this before, but it is indeed true - see https://github.com/Rust-for-Linux/linux/commit/faa3cbcca03d0dec8f8e43f1d8d5c....
I see, then just write that and mention the commit.
👍
C-String literals were added in Rust 1.77. Opportunistically replace instances of `kernel::c_str!` with C-String literals where other code changes were already necessary; the rest will be done in a later commit.
Similarly this, the message should explain the motivation for the change, the change itself and can include additional information.
The motivation is implied (that using standard types is preferable to having custom ones; this is also implicit rather than explicit in https://github.com/Rust-for-Linux/linux/issues/1075), but I can sharpen it.
Please add this information to the commit message.
👍
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 `c_str_avoid_literals` to clarify its intended use.
Closes: https://github.com/Rust-for-Linux/linux/issues/1075 Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/gpu/nova-core/driver.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 7 +++---- drivers/net/phy/qt2025.rs | 5 ++--- rust/kernel/devres.rs | 2 +- rust/kernel/firmware.rs | 6 +++--- rust/kernel/kunit.rs | 7 ++++--- rust/kernel/net/phy.rs | 6 ++---- rust/kernel/platform.rs | 4 ++-- rust/kernel/str.rs | 38 ++++++++++++++++++++++++------------ rust/kernel/sync.rs | 7 +++---- rust/kernel/sync/lock/global.rs | 3 ++- rust/macros/kunit.rs | 6 +++--- rust/macros/module.rs | 2 +- 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 +-- 17 files changed, 59 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index a08fb6599267..776970049974 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -33,7 +33,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::<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 { 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/devres.rs b/rust/kernel/devres.rs index ddb1ce4a78d9..8735b227f0d3 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -45,7 +45,7 @@ struct DevresInner<T> { /// # Example /// /// ```no_run -/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::{Io, IoRaw}}; +/// # use kernel::{bindings, device::Device, devres::Devres, io::{Io, IoRaw}}; /// # use core::ops::Deref; /// /// // See also [`pci::Bar`] for a real example. diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index 582ab648b14c..09fd3a27bcf0 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(()) @@ -203,7 +203,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 e5621d596ed3..09148e982f48 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -58,9 +58,10 @@ macro_rules! kunit_assert { break 'out; }
- static FILE: &'static $crate::str::CStr = $crate::c_str!($file); + static FILE: &'static $crate::str::CStr = $crate::c_str_avoid_literals!($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::c_str_avoid_literals!(stringify!($condition));
// SAFETY: FFI call without safety requirements. let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; @@ -222,7 +223,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 652e060e47bd..8129419a3931 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -780,7 +780,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::*; /// @@ -799,7 +798,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); /// } /// # } @@ -808,7 +807,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::*; /// @@ -828,7 +826,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 fd4a494f30e8..7163fc468b32 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -124,7 +124,7 @@ macro_rules! module_platform_driver { /// # Example /// ///``` -/// # use kernel::{bindings, c_str, device::Core, of, platform}; +/// # use kernel::{bindings, device::Core, of, platform}; /// /// struct MyDriver; /// @@ -133,7 +133,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 586644912414..0385d927fcd5 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -266,15 +266,14 @@ impl fmt::Display for crate::fmt::Adapter<&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>(()) @@ -365,25 +364,38 @@ fn as_ref(&self) -> &BStr { } }
-/// Creates a new [`CStr`] from a string literal. +/// Creates a static C string wrapper 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::c_str_avoid_literals; /// # use kernel::str::CStr; -/// const MY_CSTR: &CStr = c_str!("My awesome CStr!"); +/// const MY_CSTR: &CStr = c_str_avoid_literals!(concat!(file!(), ":", line!(), ": My CStr!")); /// ``` #[macro_export] -macro_rules! c_str { +macro_rules! c_str_avoid_literals { + // 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()) { - Ok(v) => v, - Err(_) => panic!("string contains interior NUL"), - }; + const S: &'static str = concat!($str, "\0"); + const C: &'static $crate::str::CStr = + match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) { + Ok(v) => v, + Err(err) => { + let _: core::ffi::FromBytesWithNulError = err; + panic!("string contains interior NUL") + } + }; C }}; } diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 36a719015583..424864fb448f 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -41,7 +41,6 @@ impl LockClassKey { /// /// # Example /// ``` - /// # use kernel::c_str; /// # use kernel::alloc::KBox; /// # use kernel::types::ForeignOwnable; /// # use kernel::sync::{LockClassKey, SpinLock}; @@ -53,7 +52,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) } @@ -106,9 +105,9 @@ macro_rules! static_lock_class { #[macro_export] macro_rules! optional_name { () => { - $crate::c_str!(::core::concat!(::core::file!(), ":", ::core::line!())) + $crate::c_str_avoid_literals!(::core::concat!(::core::file!(), ":", ::core::line!())) }; ($name:literal) => { - $crate::c_str!($name) + $crate::c_str_avoid_literals!($name) }; } diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs index 79d0ef7fda86..0ca23b12427c 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::c_str_avoid_literals!(::core::stringify!($name)); type Item = $valuety; type Backend = $crate::global_lock_inner!(backend $kind);
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 99ccac82edde..56469fdcee3f 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(), // ]; // @@ -106,7 +106,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(); } diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 44e5cb108cea..7b71d6d99d5c 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -232,7 +232,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_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 2bb260aebc9e..8da48c1c3c2d 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 {
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
-/// Creates a new [`CStr`] from a string literal. +/// Creates a static C string wrapper 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::c_str_avoid_literals; /// # use kernel::str::CStr; -/// const MY_CSTR: &CStr = c_str!("My awesome CStr!"); +/// const MY_CSTR: &CStr = c_str_avoid_literals!(concat!(file!(), ":", line!(), ": My CStr!")); /// ``` #[macro_export] -macro_rules! c_str { +macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
- // 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()) {Ok(v) => v,Err(_) => panic!("string contains interior NUL"),};
const S: &'static str = concat!($str, "\0");const C: &'static $crate::str::CStr =match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) {
Why is this still our CStr?
Ok(v) => v,Err(err) => {let _: core::ffi::FromBytesWithNulError = err;
Is this really necessary?
--- Cheers, Benno
panic!("string contains interior NUL")}}; C}
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
- // 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()) {Ok(v) => v,Err(_) => panic!("string contains interior NUL"),};
const S: &'static str = concat!($str, "\0");const C: &'static $crate::str::CStr =match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) {Why is this still our CStr?
Good question. I'll just revert all the changes here, I don't need to touch this.
Ok(v) => v,Err(err) => {let _: core::ffi::FromBytesWithNulError = err;Is this really necessary?
No. Reverted in v11.
On Tue May 27, 2025 at 12:29 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
There is one in this patch (:
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index e5621d596ed3..09148e982f48 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -58,9 +58,10 @@ macro_rules! kunit_assert { break 'out; }
- static FILE: &'static $crate::str::CStr = $crate::c_str!($file); + static FILE: &'static $crate::str::CStr = $crate::c_str_avoid_literals!($file);
--- Cheers, Benno
On Mon, May 26, 2025 at 7:07 PM Benno Lossin lossin@kernel.org wrote:
On Tue May 27, 2025 at 12:29 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
There is one in this patch (:
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index e5621d596ed3..09148e982f48 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -58,9 +58,10 @@ macro_rules! kunit_assert { break 'out; } - static FILE: &'static $crate::str::CStr = $crate::c_str!($file); + static FILE: &'static $crate::str::CStr = $crate::c_str_avoid_literals!($file);
Great point, and an easy one to replace with a c-string literal. Done in v11.
On Mon, May 26, 2025 at 06:29:46PM -0400, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
I also think it should be renamed. Right now it sounds like it creates a c string while avoiding literals in the input ... whatever that means. I like Benno's suggestions, but str_to_cstr! could also work?
Alice
On Wed May 28, 2025 at 12:36 PM CEST, Alice Ryhl wrote:
On Mon, May 26, 2025 at 06:29:46PM -0400, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
I also think it should be renamed. Right now it sounds like it creates a c string while avoiding literals in the input ... whatever that means.
Yeah that's a good way to put why the name is weird.
I like Benno's suggestions, but str_to_cstr! could also work?
Hmm, I think then people won't know that it can also concat? I don't think it matters too much, the macro probably won't be used that often and if someone needs to use it, they probably wouldn't fine it by name alone.
--- Cheers, Benno
On Wed, May 28, 2025 at 11:35 AM Benno Lossin lossin@kernel.org wrote:
On Wed May 28, 2025 at 12:36 PM CEST, Alice Ryhl wrote:
On Mon, May 26, 2025 at 06:29:46PM -0400, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
I also think it should be renamed. Right now it sounds like it creates a c string while avoiding literals in the input ... whatever that means.
Yeah that's a good way to put why the name is weird.
I like Benno's suggestions, but str_to_cstr! could also work?
Hmm, I think then people won't know that it can also concat? I don't think it matters too much, the macro probably won't be used that often and if someone needs to use it, they probably wouldn't fine it by name alone.
What do you mean by "it can also concat"? This macro by itself doesn't concat, it takes only a single expr. The example in the docs illustrates:
const MY_CSTR: &CStr = c_str_avoid_literals!(concat!(...));
I think str_to_cstr is ok - I'll do that in v11.
On Fri May 30, 2025 at 12:21 AM CEST, Tamir Duberstein wrote:
On Wed, May 28, 2025 at 11:35 AM Benno Lossin lossin@kernel.org wrote:
On Wed May 28, 2025 at 12:36 PM CEST, Alice Ryhl wrote:
On Mon, May 26, 2025 at 06:29:46PM -0400, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:04 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
+macro_rules! c_str_avoid_literals {
I don't like this name, how about `concat_to_c_str` or `concat_with_nul`?
This macro also is useful from macros that have a normal string literal, but can't turn it into a `c""` one.
Uh, can you give an example? I'm not attached to the name.
I also think it should be renamed. Right now it sounds like it creates a c string while avoiding literals in the input ... whatever that means.
Yeah that's a good way to put why the name is weird.
I like Benno's suggestions, but str_to_cstr! could also work?
Hmm, I think then people won't know that it can also concat? I don't think it matters too much, the macro probably won't be used that often and if someone needs to use it, they probably wouldn't fine it by name alone.
What do you mean by "it can also concat"? This macro by itself doesn't concat, it takes only a single expr.
Oh right, seems like I thought it took `$($t:tt)*`...
The example in the docs illustrates:
const MY_CSTR: &CStr = c_str_avoid_literals!(concat!(...));I think str_to_cstr is ok - I'll do that in v11.
Sounds good!
--- Cheers, Benno
Clean up references to `kernel::str::CStr`.
Signed-off-by: Tamir Duberstein tamird@gmail.com --- drivers/gpu/drm/drm_panic_qr.rs | 3 ++- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 1 + drivers/net/phy/qt2025.rs | 1 + rust/kernel/device.rs | 3 +-- rust/kernel/driver.rs | 4 ++-- 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 | 22 ++++++++++------------ 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 +- 24 files changed, 51 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index d8192a9bef63..ba63238d352f 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 6e6361c59ca1..0af1f0df2fa5 100644 --- a/drivers/gpu/nova-core/firmware.rs +++ b/drivers/gpu/nova-core/firmware.rs @@ -30,7 +30,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/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/device.rs b/rust/kernel/device.rs index 9074322c79e8..2cf5903f7dde 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, ptr}; +use core::{ffi::CStr, fmt, 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/error.rs b/rust/kernel/error.rs index 933c048c04f1..b2b46d26f7b7 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 09fd3a27bcf0..4ba5e5589d7b 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 /// @@ -168,7 +161,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; @@ -202,7 +195,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 09148e982f48..83b48e2f8379 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -58,9 +58,9 @@ macro_rules! kunit_assert { break 'out; }
- static FILE: &'static $crate::str::CStr = $crate::c_str_avoid_literals!($file); + static FILE: &'static core::ffi::CStr = $crate::c_str_avoid_literals!($file); static LINE: i32 = core::line!() as i32 - $diff; - static CONDITION: &'static $crate::str::CStr = + static CONDITION: &'static core::ffi::CStr = $crate::c_str_avoid_literals!(stringify!($condition));
// SAFETY: FFI call without safety requirements. @@ -171,7 +171,7 @@ macro_rules! kunit_assert_eq { /// 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 ec48c818d512..d36b123c518b 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -152,7 +152,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 d684ec4ef4d0..47f718a9ceb5 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -16,10 +16,9 @@ fs::File, 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 8129419a3931..72635af8f20e 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;
@@ -780,6 +780,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::*; /// @@ -807,6 +808,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 12ea65df46de..087ac8e05551 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 3aeb1250c27f..b639e6a1f590 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 7163fc468b32..ba580a4e3416 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -9,12 +9,12 @@ error::{to_result, Result}, of, prelude::*, - 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/prelude.rs b/rust/kernel/prelude.rs index 97e8bcf73669..5d55e274b41e 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -35,10 +35,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 0385d927fcd5..cf0402d1daac 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 fmt::Display for crate::fmt::Adapter<&CStr> { /// /// ``` /// # use kernel::prelude::fmt; - /// # use kernel::str::CStr; /// # use kernel::str::CString; /// let penguin = c"🐧"; /// let s = CString::try_from_fmt(fmt!("{}", penguin))?; @@ -377,8 +375,8 @@ fn as_ref(&self) -> &BStr { /// # Examples /// /// ``` +/// # use core::ffi::CStr; /// # use kernel::c_str_avoid_literals; -/// # use kernel::str::CStr; /// const MY_CSTR: &CStr = c_str_avoid_literals!(concat!(file!(), ":", line!(), ": My CStr!")); /// ``` #[macro_export] @@ -388,14 +386,14 @@ macro_rules! c_str_avoid_literals { // too limiting to macro authors, so we rely on the name as a hint instead. ($str:expr) => {{ const S: &'static str = concat!($str, "\0"); - const C: &'static $crate::str::CStr = - match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) { - Ok(v) => v, - Err(err) => { - let _: core::ffi::FromBytesWithNulError = err; - panic!("string contains interior NUL") - } - }; + const C: &'static core::ffi::CStr = match core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) + { + Ok(v) => v, + Err(err) => { + let _: core::ffi::FromBytesWithNulError = err; + panic!("string contains interior NUL") + } + }; C }}; } 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 0ca23b12427c..bfeaa21ab101 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::c_str_avoid_literals!(::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 d7e6e59e124b..bf2fb24d04ea 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 f98bd02b838f..c64769ef5b90 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 7b71d6d99d5c..5246fed82e63 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -232,7 +232,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.
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Clean up references to `kernel::str::CStr`.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 3 ++- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 1 + drivers/net/phy/qt2025.rs | 1 + rust/kernel/device.rs | 3 +-- rust/kernel/driver.rs | 4 ++-- 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 | 22 ++++++++++------------ 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 +- 24 files changed, 51 insertions(+), 55 deletions(-)
Haven't compile tested this series yet, but this commit seems to suggest to me that some of the previous commits introduced code that doesn't compile or emits warnings? If so that needs to be fixed.
--- Cheers, Benno
On Mon, May 26, 2025 at 11:05 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Clean up references to `kernel::str::CStr`.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 3 ++- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 1 + drivers/net/phy/qt2025.rs | 1 + rust/kernel/device.rs | 3 +-- rust/kernel/driver.rs | 4 ++-- 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 | 22 ++++++++++------------ 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 +- 24 files changed, 51 insertions(+), 55 deletions(-)
Haven't compile tested this series yet, but this commit seems to suggest to me that some of the previous commits introduced code that doesn't compile or emits warnings? If so that needs to be fixed.
That's not the case. There are no warnings and no compilation failures in prior commits.
On Tue May 27, 2025 at 12:30 AM CEST, Tamir Duberstein wrote:
On Mon, May 26, 2025 at 11:05 AM Benno Lossin lossin@kernel.org wrote:
On Sat May 24, 2025 at 10:33 PM CEST, Tamir Duberstein wrote:
Clean up references to `kernel::str::CStr`.
Signed-off-by: Tamir Duberstein tamird@gmail.com
drivers/gpu/drm/drm_panic_qr.rs | 3 ++- drivers/gpu/nova-core/firmware.rs | 2 +- drivers/net/phy/ax88796b_rust.rs | 1 + drivers/net/phy/qt2025.rs | 1 + rust/kernel/device.rs | 3 +-- rust/kernel/driver.rs | 4 ++-- 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 | 22 ++++++++++------------ 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 +- 24 files changed, 51 insertions(+), 55 deletions(-)
Haven't compile tested this series yet, but this commit seems to suggest to me that some of the previous commits introduced code that doesn't compile or emits warnings? If so that needs to be fixed.
That's not the case. There are no warnings and no compilation failures in prior commits.
Ah it's because of the `pub use`... I tested it both with 1.86 and 1.78 and aside from the `use<>` error reported by the bot everything worked.
--- Cheers, Benno
On Sat, May 24, 2025 at 04:33:00PM -0400, Tamir Duberstein 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
Overall LGTM, thanks! Left a few comments on individual patches, but I can probably give a RB when those a fixed. :)
Alice
On Wed, May 28, 2025 at 6:38 AM Alice Ryhl aliceryhl@google.com wrote:
On Sat, May 24, 2025 at 04:33:00PM -0400, Tamir Duberstein 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
Overall LGTM, thanks! Left a few comments on individual patches, but I can probably give a RB when those a fixed. :)
Thanks for looking! You say a few comments, but I only saw one. Did some get lost?
linux-kselftest-mirror@lists.linaro.org
-
Alice Ryhl -
Benno Lossin -
Miguel Ojeda -
Tamir Duberstein