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

[Michal Rostecki <vadorovsky@gmail.com>]

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@gmail.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