| Message ID | 20240819153656.28807-2-vadorovsky@protonmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND,v5] rust: str: Use `core::CStr`, remove the custom `CStr` implementation | expand |
On 8/19/24 17:36, Michal Rostecki wrote: > From: Michal Rostecki <vadorovsky@gmail.com> > > `CStr` became a part of `core` library in Rust 1.75. This change replaces > the custom `CStr` implementation with the one from `core`. > > `core::CStr` behaves generally the same as the removed implementation, > with the following differences: > > - It does not implement `Display`. > - It does not provide `from_bytes_with_nul_unchecked_mut` method. > - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns > `*const c_char`. > > The first two differences are handled by providing the `CStrExt` trait, > with `display()` and `from_bytes_with_nul_unchecked_mut()` methods. > `display()` returns a `CStrDisplay` wrapper, with a custom `Display` > implementation. > > `DerefMut` implementation for `CString` is removed here, as it's not > being used anywhere. > > Signed-off-by: Michal Rostecki <vadorovsky@gmail.com> > --- > rust/kernel/error.rs | 7 +- > rust/kernel/kunit.rs | 18 +- > rust/kernel/net/phy.rs | 2 +- > rust/kernel/prelude.rs | 4 +- > rust/kernel/str.rs | 465 ++++++------------------------------ > rust/kernel/sync/condvar.rs | 5 +- > rust/kernel/sync/lock.rs | 6 +- > rust/kernel/workqueue.rs | 2 +- > scripts/rustdoc_test_gen.rs | 4 +- > 9 files changed, 93 insertions(+), 420 deletions(-) > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs > index 6f1587a2524e..6a2bb11ac09c 100644 > --- a/rust/kernel/error.rs > +++ b/rust/kernel/error.rs > @@ -4,10 +4,11 @@ > //! > //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h) > > -use crate::{alloc::AllocError, str::CStr}; > +use crate::alloc::AllocError; > > use alloc::alloc::LayoutError; > > +use core::ffi::CStr; > use core::fmt; > use core::num::TryFromIntError; > use core::str::Utf8Error; > @@ -151,7 +152,7 @@ pub fn name(&self) -> Option<&'static CStr> { > None > } else { > // SAFETY: The string returned by `errname` is static and `NUL`-terminated. > - Some(unsafe { CStr::from_char_ptr(ptr) }) > + Some(unsafe { CStr::from_ptr(ptr) }) > } > } > > @@ -173,7 +174,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > None => f.debug_tuple("Error").field(&-self.0).finish(), > // SAFETY: These strings are ASCII-only. > Some(name) => f > - .debug_tuple(unsafe { core::str::from_utf8_unchecked(name) }) > + .debug_tuple(unsafe { core::str::from_utf8_unchecked(name.to_bytes()) }) > .finish(), > } > } > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs > index 0ba77276ae7e..79a50ab59af0 100644 > --- a/rust/kernel/kunit.rs > +++ b/rust/kernel/kunit.rs > @@ -56,13 +56,15 @@ macro_rules! kunit_assert { > break 'out; > } > > - static FILE: &'static $crate::str::CStr = $crate::c_str!($file); > + static FILE: &'static core::ffi::CStr = $file; > static LINE: i32 = core::line!() as i32 - $diff; > - static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); > + static CONDITION: &'static core::ffi::CStr = $crate::c_str!(stringify!($condition)); > > // SAFETY: FFI call without safety requirements. > let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; > if kunit_test.is_null() { > + use kernel::str::CStrExt; > + > // The assertion failed but this task is not running a KUnit test, so we cannot call > // KUnit, but at least print an error to the kernel log. This may happen if this > // macro is called from an spawned thread in a test (see > @@ -71,11 +73,13 @@ macro_rules! kunit_assert { > // > // This mimics KUnit's failed assertion format. > $crate::kunit::err(format_args!( > - " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", > - $name > + " # {}: ASSERTION FAILED at {}:{LINE}\n", > + $name.display(), > + FILE.display(), > )); > $crate::kunit::err(format_args!( > - " Expected {CONDITION} to be true, but is false\n" > + " Expected {} to be true, but is false\n", > + CONDITION.display(), > )); > $crate::kunit::err(format_args!( > " Failure not reported to KUnit since this is a non-KUnit task\n" > @@ -98,12 +102,12 @@ unsafe impl Sync for Location {} > unsafe impl Sync for UnaryAssert {} > > static LOCATION: Location = Location($crate::bindings::kunit_loc { > - file: FILE.as_char_ptr(), > + file: FILE.as_ptr(), > line: LINE, > }); > static ASSERTION: UnaryAssert = UnaryAssert($crate::bindings::kunit_unary_assert { > assert: $crate::bindings::kunit_assert {}, > - condition: CONDITION.as_char_ptr(), > + condition: CONDITION.as_ptr(), > expected_true: true, > }); > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > index fd40b703d224..19f45922ec42 100644 > --- a/rust/kernel/net/phy.rs > +++ b/rust/kernel/net/phy.rs > @@ -502,7 +502,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: T::NAME.as_ptr().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/prelude.rs b/rust/kernel/prelude.rs > index b37a0b3180fb..b0969ca78f10 100644 > --- a/rust/kernel/prelude.rs > +++ b/rust/kernel/prelude.rs > @@ -12,7 +12,7 @@ > //! ``` > > #[doc(no_inline)] > -pub use core::pin::Pin; > +pub use core::{ffi::CStr, pin::Pin}; > > pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt}; > > @@ -35,7 +35,7 @@ > > pub use super::error::{code::*, Error, Result}; > > -pub use super::{str::CStr, ThisModule}; > +pub use super::ThisModule; > > pub use super::init::{InPlaceInit, Init, PinInit}; > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > index bb8d4f41475b..97a298a44b96 100644 > --- a/rust/kernel/str.rs > +++ b/rust/kernel/str.rs > @@ -4,8 +4,9 @@ > > use crate::alloc::{flags::*, vec_ext::VecExt, AllocError}; > use alloc::vec::Vec; > +use core::ffi::CStr; > use core::fmt::{self, Write}; > -use core::ops::{self, Deref, DerefMut, Index}; > +use core::ops::Deref; > > use crate::error::{code::*, Error}; > > @@ -41,11 +42,11 @@ impl fmt::Display for BStr { > /// # use kernel::{fmt, b_str, str::{BStr, CString}}; > /// let ascii = b_str!("Hello, BStr!"); > /// let s = CString::try_from_fmt(fmt!("{}", ascii)).unwrap(); > - /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes()); > + /// assert_eq!(s.to_bytes(), "Hello, BStr!".as_bytes()); > /// > /// let non_ascii = b_str!("
On Mon, Aug 19, 2024 at 5:39 PM Michal Rostecki <vadorovsky@gmail.com> wrote: > > From: Michal Rostecki <vadorovsky@gmail.com> > > `CStr` became a part of `core` library in Rust 1.75. This change replaces > the custom `CStr` implementation with the one from `core`. > > `core::CStr` behaves generally the same as the removed implementation, > with the following differences: > > - It does not implement `Display`. > - It does not provide `from_bytes_with_nul_unchecked_mut` method. > - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns > `*const c_char`. > > The first two differences are handled by providing the `CStrExt` trait, > with `display()` and `from_bytes_with_nul_unchecked_mut()` methods. > `display()` returns a `CStrDisplay` wrapper, with a custom `Display` > implementation. > > `DerefMut` implementation for `CString` is removed here, as it's not > being used anywhere. > > Signed-off-by: Michal Rostecki <vadorovsky@gmail.com> A few comments: * I would probably add CStrExt to the kernel prelude. * I would probably remove `from_bytes_with_nul_unchecked_mut` and keep `DerefMut for CString` instead of the other way around. * Perhaps we should remove the `c_str!` macro and use c"" instead? Alice
On Wed, Aug 21, 2024 at 2:08 PM Alice Ryhl <aliceryhl@google.com> wrote: > > On Mon, Aug 19, 2024 at 5:39 PM Michal Rostecki <vadorovsky@gmail.com> wrote: > > > > From: Michal Rostecki <vadorovsky@gmail.com> > > > > `CStr` became a part of `core` library in Rust 1.75. This change replaces > > the custom `CStr` implementation with the one from `core`. > > > > `core::CStr` behaves generally the same as the removed implementation, > > with the following differences: > > > > - It does not implement `Display`. > > - It does not provide `from_bytes_with_nul_unchecked_mut` method. > > - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns > > `*const c_char`. > > > > The first two differences are handled by providing the `CStrExt` trait, > > with `display()` and `from_bytes_with_nul_unchecked_mut()` methods. > > `display()` returns a `CStrDisplay` wrapper, with a custom `Display` > > implementation. > > > > `DerefMut` implementation for `CString` is removed here, as it's not > > being used anywhere. > > > > Signed-off-by: Michal Rostecki <vadorovsky@gmail.com> > > A few comments: > > * I would probably add CStrExt to the kernel prelude. > * I would probably remove `from_bytes_with_nul_unchecked_mut` and keep > `DerefMut for CString` instead of the other way around. > * Perhaps we should remove the `c_str!` macro and use c"" instead? Ah, also, please add this tag: Closes: https://github.com/Rust-for-Linux/linux/issues/1075 Alice
On 21.08.24 14:08, Alice Ryhl wrote: > On Mon, Aug 19, 2024 at 5:39 PM Michal Rostecki <vadorovsky@gmail.com> wrote: >> >> From: Michal Rostecki <vadorovsky@gmail.com> >> >> `CStr` became a part of `core` library in Rust 1.75. This change replaces >> the custom `CStr` implementation with the one from `core`. >> >> `core::CStr` behaves generally the same as the removed implementation, >> with the following differences: >> >> - It does not implement `Display`. >> - It does not provide `from_bytes_with_nul_unchecked_mut` method. >> - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns >> `*const c_char`. >> >> The first two differences are handled by providing the `CStrExt` trait, >> with `display()` and `from_bytes_with_nul_unchecked_mut()` methods. >> `display()` returns a `CStrDisplay` wrapper, with a custom `Display` >> implementation. >> >> `DerefMut` implementation for `CString` is removed here, as it's not >> being used anywhere. >> >> Signed-off-by: Michal Rostecki <vadorovsky@gmail.com> > > A few comments: > > * I would probably add CStrExt to the kernel prelude. > * I would probably remove `from_bytes_with_nul_unchecked_mut` and keep > `DerefMut for CString` instead of the other way around. > * Perhaps we should remove the `c_str!` macro and use c"" instead? Read [1], it is needed, because there is no `c_stringify!` macro. [1]: https://lore.kernel.org/rust-for-linux/52676577-372c-4a7f-aace-4cf100f93bfb@gmail.com/ --- Cheers, Benno
On Mon, Aug 19, 2024 at 10:39 AM Michal Rostecki <vadorovsky@gmail.com> wrote: > > From: Michal Rostecki <vadorovsky@gmail.com> You don't need this since the email already shows it is already from you :) Aiui this is only needed when forwarding a patch for someone else, or if you use a different commit email for some reason. > `CStr` became a part of `core` library in Rust 1.75. This change replaces > the custom `CStr` implementation with the one from `core`. The diff in `kernel/str.rs` is really difficult to read and review since the new parts get interleaved with the removed lines. Could you split this into a couple patches? Probably roughly the five described below: 1. Add all the new things `CStrExt`, `CStrDisplay`, and their implementations. 2. Add `CStrExt` to the prelude (Alice's suggestion) 3. Update existing uses of our `CStr` to instead use `core::CStr` 4. Remove our current `CStr` 5. Change any docs for `CString` or `c_str!`, as relevant Just remember that things should build after each patch. > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs > index 0ba77276ae7e..79a50ab59af0 100644 > --- a/rust/kernel/kunit.rs > +++ b/rust/kernel/kunit.rs > @@ -56,13 +56,15 @@ macro_rules! kunit_assert { > break 'out; > } > > - static FILE: &'static $crate::str::CStr = $crate::c_str!($file); > + static FILE: &'static core::ffi::CStr = $file; > static LINE: i32 = core::line!() as i32 - $diff; > - static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); > + static CONDITION: &'static core::ffi::CStr = $crate::c_str!(stringify!($condition)); This change and the associated invocation changes can be dropped since we are keeping `c_str`. It's cleaner to be able to call macros with "standard strings" rather than c"c strings" where possible. > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > index bb8d4f41475b..97a298a44b96 100644 > --- a/rust/kernel/str.rs > +++ b/rust/kernel/str.rs (I removed most of the `-` lines for my review below) > +/// Wrapper around [`CStr`] which implements [`Display`](core::fmt::Display). > +pub struct CStrDisplay<'a>(&'a CStr); > > +impl fmt::Display for CStrDisplay<'_> { > + /// Formats printable ASCII characters, escaping the rest. > /// > /// # Examples > /// > /// ``` > + /// # use core::ffi::CStr; > /// # use kernel::c_str; > /// # use kernel::fmt; > + /// # use kernel::str::{CStrExt, CString}; > + /// let penguin = c"
On Mon, Aug 19, 2024 at 10:43 AM Michal Rostecki <vadorovsky@gmail.com> wrote: > > On 8/19/24 17:36, Michal Rostecki wrote: > > From: Michal Rostecki <vadorovsky@gmail.com> > > > > `CStr` became a part of `core` library in Rust 1.75. This change replaces > > the custom `CStr` implementation with the one from `core`. [ very long snip ] > > @@ -180,7 +180,7 @@ macro_rules! assert {{ > > #[allow(unused)] > > macro_rules! assert_eq {{ > > ($left:expr, $right:expr $(,)?) => {{{{ > > - kernel::kunit_assert_eq!("{kunit_name}", "{real_path}", __DOCTEST_ANCHOR - {line}, $left, $right); > > + kernel::kunit_assert_eq!(c"{kunit_name}", c"{real_path}", __DOCTEST_ANCHOR - {line}, $left, $right); > > }}}} > > }} > > > > I forgot to include the version history before sending the patch, sorry. For future reference - when replying, you should delete irrelevant bits of the message you are replying to, otherwise things get kind of messy. - Trevor
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 6f1587a2524e..6a2bb11ac09c 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -4,10 +4,11 @@ //! //! C header: [`include/uapi/asm-generic/errno-base.h`](srctree/include/uapi/asm-generic/errno-base.h) -use crate::{alloc::AllocError, str::CStr}; +use crate::alloc::AllocError; use alloc::alloc::LayoutError; +use core::ffi::CStr; use core::fmt; use core::num::TryFromIntError; use core::str::Utf8Error; @@ -151,7 +152,7 @@ pub fn name(&self) -> Option<&'static CStr> { None } else { // SAFETY: The string returned by `errname` is static and `NUL`-terminated. - Some(unsafe { CStr::from_char_ptr(ptr) }) + Some(unsafe { CStr::from_ptr(ptr) }) } } @@ -173,7 +174,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { None => f.debug_tuple("Error").field(&-self.0).finish(), // SAFETY: These strings are ASCII-only. Some(name) => f - .debug_tuple(unsafe { core::str::from_utf8_unchecked(name) }) + .debug_tuple(unsafe { core::str::from_utf8_unchecked(name.to_bytes()) }) .finish(), } } diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 0ba77276ae7e..79a50ab59af0 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -56,13 +56,15 @@ macro_rules! kunit_assert { break 'out; } - static FILE: &'static $crate::str::CStr = $crate::c_str!($file); + static FILE: &'static core::ffi::CStr = $file; static LINE: i32 = core::line!() as i32 - $diff; - static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition)); + static CONDITION: &'static core::ffi::CStr = $crate::c_str!(stringify!($condition)); // SAFETY: FFI call without safety requirements. let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; if kunit_test.is_null() { + use kernel::str::CStrExt; + // The assertion failed but this task is not running a KUnit test, so we cannot call // KUnit, but at least print an error to the kernel log. This may happen if this // macro is called from an spawned thread in a test (see @@ -71,11 +73,13 @@ macro_rules! kunit_assert { // // This mimics KUnit's failed assertion format. $crate::kunit::err(format_args!( - " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", - $name + " # {}: ASSERTION FAILED at {}:{LINE}\n", + $name.display(), + FILE.display(), )); $crate::kunit::err(format_args!( - " Expected {CONDITION} to be true, but is false\n" + " Expected {} to be true, but is false\n", + CONDITION.display(), )); $crate::kunit::err(format_args!( " Failure not reported to KUnit since this is a non-KUnit task\n" @@ -98,12 +102,12 @@ unsafe impl Sync for Location {} unsafe impl Sync for UnaryAssert {} static LOCATION: Location = Location($crate::bindings::kunit_loc { - file: FILE.as_char_ptr(), + file: FILE.as_ptr(), line: LINE, }); static ASSERTION: UnaryAssert = UnaryAssert($crate::bindings::kunit_unary_assert { assert: $crate::bindings::kunit_assert {}, - condition: CONDITION.as_char_ptr(), + condition: CONDITION.as_ptr(), expected_true: true, }); diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index fd40b703d224..19f45922ec42 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -502,7 +502,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: T::NAME.as_ptr().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/prelude.rs b/rust/kernel/prelude.rs index b37a0b3180fb..b0969ca78f10 100644 --- a/rust/kernel/prelude.rs +++ b/rust/kernel/prelude.rs @@ -12,7 +12,7 @@ //! ``` #[doc(no_inline)] -pub use core::pin::Pin; +pub use core::{ffi::CStr, pin::Pin}; pub use crate::alloc::{box_ext::BoxExt, flags::*, vec_ext::VecExt}; @@ -35,7 +35,7 @@ pub use super::error::{code::*, Error, Result}; -pub use super::{str::CStr, ThisModule}; +pub use super::ThisModule; pub use super::init::{InPlaceInit, Init, PinInit}; diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index bb8d4f41475b..97a298a44b96 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -4,8 +4,9 @@ use crate::alloc::{flags::*, vec_ext::VecExt, AllocError}; use alloc::vec::Vec; +use core::ffi::CStr; use core::fmt::{self, Write}; -use core::ops::{self, Deref, DerefMut, Index}; +use core::ops::Deref; use crate::error::{code::*, Error}; @@ -41,11 +42,11 @@ impl fmt::Display for BStr { /// # use kernel::{fmt, b_str, str::{BStr, CString}}; /// let ascii = b_str!("Hello, BStr!"); /// let s = CString::try_from_fmt(fmt!("{}", ascii)).unwrap(); - /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes()); + /// assert_eq!(s.to_bytes(), "Hello, BStr!".as_bytes()); /// /// let non_ascii = b_str!("