Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

[Michal Rostecki]

On 14.07.24 19:01, Björn Roy Baron wrote:
> On Sunday, July 14th, 2024 at 18:02, Michal Rostecki <vadorovsky@xxxxxxxxx> wrote:
>
> > `CStr` became a part of `core` library in Rust 1.75, therefore there is
> > no need to keep the custom implementation.
> >
> > `core::CStr` behaves generally the same as the removed implementation,
> > with the following differences:
> >
> > - It does not implement `Display` (but implements `Debug`).
> > - It does not provide `from_bytes_with_nul_unchecked_mut` method.
> >    - It was used only in `DerefMut` implementation for `CString`. This
> >      change replaces it with a manual cast to `&mut CStr`.
> >    - Otherwise, having such a method is not really desirable. `CStr` is
> >      a reference type
> >      or `str` are usually not supposed to be modified.
> > - It has `as_ptr()` method instead of `as_char_ptr()`, which also returns
> >    `*const c_char`.
> >
> > Rust also introduces C literals (`c""`), so the `c_str` macro is removed
> > here as well.
> >
> > Signed-off-by: Michal Rostecki <vadorovsky@xxxxxxxxx>
> > ---
> >   rust/kernel/error.rs        |   7 +-
> >   rust/kernel/init.rs         |   8 +-
> >   rust/kernel/kunit.rs        |  16 +-
> >   rust/kernel/net/phy.rs      |   2 +-
> >   rust/kernel/prelude.rs      |   4 +-
> >   rust/kernel/str.rs          | 490 +-----------------------------------
> >   rust/kernel/sync.rs         |  13 +-
> >   rust/kernel/sync/condvar.rs |   5 +-
> >   rust/kernel/sync/lock.rs    |   6 +-
> >   rust/kernel/workqueue.rs    |  10 +-
> >   scripts/rustdoc_test_gen.rs |   4 +-
> >   11 files changed, 57 insertions(+), 508 deletions(-)
>
> [snip]
>
> > diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> > index 68605b633e73..af0017e56c0e 100644
> > --- a/rust/kernel/init.rs
> > +++ b/rust/kernel/init.rs
> > @@ -46,7 +46,7 @@
> >   //! }
> >   //!
> >   //! let foo = pin_init!(Foo {
> > -//!     a <- new_mutex!(42, "Foo::a"),
> > +//!     a <- new_mutex!(42, c"Foo::a"),
>
> That we need a CStr here seems a bit of an internal implementation detail. Maybe
> keep accepting a regular string literal and converting it to a CStr internally?
> If others think what you have here is fine, I don't it mind all that much though.

The names passed to `new_mutex`, `new_condvar`, `new_spinlock` etc. are 
immediately passed in the FFI calls (`__mutex_init`, 
`__init_waitqueue_head`, `__spin_lock_init`) [0][1][2]. In fact, I don't 
see any internal usage, where using Rust &str would be beneficial. Am I 
missing something?

Converting a &str to &CStr inside `Mutex::new` or `CondVar::new` would 
require allocating a new buffer, larger by 1, to include the nul byte. 
Doing that for every new mutex or condvar seems a bit wasteful to me.

[0] 
https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/lock/mutex.rs#L104
[1] 
https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/condvar.rs#L111
[2] 
https://github.com/Rust-for-Linux/linux/blob/b1263411112305acf2af728728591465becb45b0/rust/kernel/sync/lock/spinlock.rs#L103

> >   //!     b: 24,
> >   //! });
> >   //! ```
>
> [snip]
>
> > @@ -840,9 +375,10 @@ fn deref(&self) -> &Self::Target {
> >
> >   impl DerefMut for CString {
> >       fn deref_mut(&mut self) -> &mut Self::Target {
> > -        // SAFETY: A `CString` is always NUL-terminated and contains no other
> > -        // NUL bytes.
> > -        unsafe { CStr::from_bytes_with_nul_unchecked_mut(self.buf.as_mut_slice()) }
> > +        debug_assert!(!self.buf.is_empty() && self.buf[self.buf.len() - 1] == 0);
> > +        // SAFETY: Casting to CStr is safe because its internal representation
> > +        // is a [u8] too.
> > +        unsafe { &mut *(self.buf.as_mut_slice() as *mut [u8] as *mut CStr) }
>
> The documentation of CStr [1] is very clear that the layout of CStr is not guaranteed.
>
> > Note that this structure does not have a guaranteed layout (the repr(transparent)
> > notwithstanding) and is not recommended to be placed in the signatures of FFI
> > functions. Instead, safe wrappers of FFI functions may leverage the unsafe
> > CStr::from_ptr constructor to provide a safe interface to other consumers.
>
> Is there any place where this DerefMut impl is actually used? If not it should probably
> be removed. The liballoc version of CString doesn't have this impl either. (Can we use
> the liballoc version of CString too just like this patch does for CStr?)
>
> [snip]
>
> Link: https://doc.rust-lang.org/stable/std/ffi/struct.CStr.html [1]

Good call. The `DerefMut` was not used anywhere, removing it works.