Message-ID: <52676577-372c-4a7f-aace-4cf100f93bfb@gmail.com>
Date: Mon, 15 Jul 2024 18:12:05 +0200
From: Michal Rostecki <vadorovsky@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.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>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 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:30, Miguel Ojeda wrote:
> Hi Michal,
> 
> Thanks for the patch! Some notes below...
> 
> On Sun, Jul 14, 2024 at 6:02 PM 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.
> 
> It would depend on the differences, right? i.e. for a reader, is this
> meant to imply there is no meaningful difference in what you point out
> below?
> 

Alright, I will remove the second part of the sentence.

>> - It does not implement `Display` (but implements `Debug`).
> 
> One question that comes up when reading this is: are we losing
> `Display`'s output form?
> 

Yes, we are losing the `Display` trait implementation by switching to 
`core::ffi::CStr`.

I was thinking whether I should keep `kernel::str::CStr` as a wrapper, 
just to keep the `Display` implementation. I could still do that if you 
want. I'm also open for other solutions.

The reason why I decided to not do that and go ahead without `Display` 
is that it was used only in rust/kernel/kunit.rs inside `kunit_assert`, 
for formatting the file and path the error message. This diff:

@@ -71,11 +75,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"
                  ));

The only practical difference in switching from `Display` to `Debug` 
here is that the fallback kunit error messages are going to include 
quotation marks around conditions, files and lines.

> Also, for clarity, please mention if there is a difference in the
> output of the `Debug` ones.
> 

There isn't any difference, I will mention that.

>>    - Otherwise, having such a method is not really desirable. `CStr` is
>>      a reference type
>>      or `str` are usually not supposed to be modified.
> 
> The sentence seems to be cut, and it should probably try to explain
> better why it is undesirable, i.e. if it is needed for something like
> `DerefMut`, then it seems better to have a method.
> 

Regarding `DerefMut` implementation for `CString` - we don't need it. Or 
at least - removing it (after my CStr patch), does not break anything. 
If that's fine for you, I'm going to remove it in v2 all together.

About why having `&mut CStr` is undesirable - I will try to find better 
wording. My general point is that I've never seen `&mut str` being 
exposed in any core/std API to the external user, mutation usually 
implies usage of an owned String.

>> -            static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
>> +            static CONDITION: &'static core::ffi::CStr = unsafe {
>> +                core::ffi::CStr::from_bytes_with_nul_unchecked(
>> +                    core::concat!(stringify!($condition), "\0").as_bytes()
>> +                )
>> +            };
> 
> This looks worse after the change and requires `unsafe`. Can we do
> something to improve it?
> 

I think the best solution would be leaving `c_str` macro for that. The 
reason why I removed it is that the GitHub issue[0] mentions its 
removal. But for that case, I think it makes sense to leave it. What do 
you think?

[0] https://github.com/Rust-for-Linux/linux/issues/1075

>> +        // 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) }
> 
> I see Björn commented on this already -- `CStr`'s layout is not
> guaranteed (and is a `[c_char]` instead).
> 
> Also, the casting is not what is unsafe, so perhaps it may be clearer
> to reword the comment.
> 
> In addition, please format comments as Markdown.
> 

Good point, I will fix the comment.

>> -//!             work <- new_work!("MyStruct::work"),
>> +//!             work <- new_work!(c"MyStruct::work"),
> 
> I agree as well that it may make sense to simplify the callers as much
> as possible, unless there is a need to have that flexibility.
> 

I already replied to Björn - names passed to `new_work!`, `new_mutex!` 
are immediatelly passed to FFI calls and are not used in the Rust code 
internally, so I prefer to keep them as C strings rather than Rust strings.

> Cheers,
> Miguel


Cheers,
Michal

Powered by blists - more mailing lists