[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df092baf-03a5-4b4a-ab8b-ee7a5677c172@gmail.com>
Date: Mon, 15 Jul 2024 17:46:16 +0200
From: Michal Rostecki <vadorovsky@...il.com>
To: Björn Roy Baron <bjorn3_gh@...tonmail.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng
<boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl
<aliceryhl@...gle.com>, Brendan Higgins <brendan.higgins@...ux.dev>,
David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>,
FUJITA Tomonori <fujita.tomonori@...il.com>, Trevor Gross
<tmgross@...ch.edu>, Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling
<morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
Martin Rodriguez Reboredo <yakoyoku@...il.com>, Finn Behrens
<me@...enk.dev>, Manmohan Shukla <manmshuk@...il.com>,
Valentin Obst <kernel@...entinobst.de>,
Laine Taffin Altman <alexanderaltman@...com>,
Danilo Krummrich <dakr@...hat.com>, Yutaro Ohno <yutaro.ono.418@...il.com>,
Tiago Lam <tiagolam@...il.com>, Charalampos Mitrodimas
<charmitro@...teo.net>, Ben Gooding <ben.gooding.dev@...il.com>,
Roland Xu <mu001999@...look.com>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
kunit-dev@...glegroups.com, netdev@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr`
implementation
On 14.07.24 19:01, Björn Roy Baron wrote:
> On Sunday, July 14th, 2024 at 18:02, Michal Rostecki <vadorovsky@...il.com> 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@...il.com>
>> ---
>> 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.
Powered by blists - more mailing lists