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.
