| Message ID | 20240715221126.487345-2-vadorovsky@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] rust: str: Use `core::CStr`, remove the custom `CStr` implementation | expand |
On Mon, Jul 15, 2024 at 5:14 PM Michal Rostecki <vadorovsky@gmail.com> wrote: > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs > index 0ba77276ae7e..c08f9dddaa6f 100644 > --- a/rust/kernel/kunit.rs > +++ b/rust/kernel/kunit.rs > [...] > // SAFETY: FFI call without safety requirements. > let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() }; > @@ -71,11 +71,11 @@ macro_rules! kunit_assert { > // > // This mimics KUnit's failed assertion format. > $crate::kunit::err(format_args!( > - " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", > + " # {:?}: ASSERTION FAILED at {FILE:?}:{LINE:?}\n", > $name > )); > $crate::kunit::err(format_args!( > - " Expected {CONDITION} to be true, but is false\n" > + " Expected {CONDITION:?} to be true, but is false\n" > )); These aren't exactly the same: the existing `Display` impl will print the string (hexifying invalid characters), but this will add `" ... "` around it. In Rust's libraries, string `Path` and `OsStr` both have a `.display()` method that returns a wrapper type that does implement `fmt::Display`, which can then be printed (see [1]). We could do something similar here, via a `CStrExt` trait that goes in the prelude and provides `.display(&self)`. Rust itself could actually use something here too - if you're up for it, feel free to propose an implementation via ACP (that's just an issue template at [2]). It would probably be pretty similar to the recent `OsStr` one. Of course it will be a while before we can use it in the kernel, but it wouldn't hurt to get the ball rolling. [1]: https://doc.rust-lang.org/std/path/struct.Path.html#method.display [2]: https://github.com/rust-lang/libs-team/issues > /// Creates a new [`CStr`] from a string literal. > /// > -/// The string literal should not contain any `NUL` bytes. > +/// Usually, defining C-string literals directly should be preffered, but this > +/// macro is helpful in situations when C-string literals are hard or > +/// impossible to use, for example: > +/// > +/// - 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. > +/// s/preffered/prefered "when C-string literals are hard or impossible to use" doesn't sound quite right - I think it is more common that you're just hiding an implementation detail (string type) from the user of a macro. Maybe something like: This isn't needed when C-string literals (`c"hello"` syntax) can be used directly, but can be used when a C-string version of a standard string literal is required (often when working with macros). > +/// The string should not contain any `NUL` bytes. > /// > /// # Examples > /// > /// ``` > +/// # use core::ffi::CStr; > /// # use kernel::c_str; > -/// # use kernel::str::CStr; > -/// const MY_CSTR: &CStr = c_str!("My awesome CStr!"); > +/// const MY_CSTR: &CStr = c_str!(stringify!(5)); > /// ``` > #[macro_export] > macro_rules! c_str { > ($str:expr) => {{ > const S: &str = concat!($str, "\0"); > - const C: &$crate::str::CStr = match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) { > + const C: &core::ffi::CStr = match core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) { > Ok(v) => v, > Err(_) => panic!("string contains interior NUL"), > }; Thanks for this, will be a nice bit of code cleanup. - Trevor (also, v2 and v3 are appearing in different threads on lore (as they should), but they're in the same thread as v1 in my email client - any idea if there is a reason for this?)
On 16.07.24 02:45, Trevor Gross wrote: > (also, v2 and v3 are appearing in different threads on lore (as they > should), but they're in the same thread as v1 in my email client - any > idea if there is a reason for this?) No idea, I've sent both patches with: git send-email --cc-cmd='./scripts/get_maintainer.pl --norolestats' -v${VERSION} -1
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 55280ae9fe40..18808b29604d 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; @@ -142,7 +143,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) }) } } @@ -164,7 +165,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..c08f9dddaa6f 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -56,9 +56,9 @@ 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() }; @@ -71,11 +71,11 @@ macro_rules! kunit_assert { // // This mimics KUnit's failed assertion format. $crate::kunit::err(format_args!( - " # {}: ASSERTION FAILED at {FILE}:{LINE}\n", + " # {:?}: ASSERTION FAILED at {FILE:?}:{LINE:?}\n", $name )); $crate::kunit::err(format_args!( - " Expected {CONDITION} to be true, but is false\n" + " Expected {CONDITION:?} to be true, but is false\n" )); $crate::kunit::err(format_args!( " Failure not reported to KUnit since this is a non-KUnit task\n" @@ -98,12 +98,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..e491a9803187 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!("
`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` (but implements `Debug`). Therefore, by switching to `core::CStr`, we lose the `Display` implementation. - Lack of `Display` implementation impacted only rust/kernel/kunit.rs. In this change, we use `Debug` format there. The only difference between the removed `Display` output and `Debug` output are quotation marks present in the latter (`foo` vs `"foo"`). - It does not provide `from_bytes_with_nul_unchecked_mut` method. - It was used only in `DerefMut` implementation for `CString`. This change removes that implementation. - Otherwise, having such a method is not desirable. The rule in Rust std is that `str` is used only as an immutable reference (`&str`), while mutating strings is done with the owned `String` type. Similarly, we can introduce the rule that `CStr` should be used only as an immutable reference (`&CStr`), while mutating is done only with the owned `CString` type. - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns `*const c_char`. Signed-off-by: Michal Rostecki <vadorovsky@gmail.com> --- v1 -> 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`. v2 -> v3: - Fix the commit message. - Remove redundant braces in `use`, when only one item is imported. rust/kernel/error.rs | 7 +- rust/kernel/kunit.rs | 12 +- rust/kernel/net/phy.rs | 2 +- rust/kernel/prelude.rs | 4 +- rust/kernel/str.rs | 486 ++---------------------------------- 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, 44 insertions(+), 484 deletions(-)