Message-ID: <CALNs47sa3RfbeGoT+_MUcZUAv=Wn2t0o1L-9v1D7jWp6NVicGg@mail.gmail.com>
Date: Sat, 24 Aug 2024 02:22:11 -0500
From: Trevor Gross <tmgross@...ch.edu>
To: Michal Rostecki <vadorovsky@...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>, 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>, Yutaro Ohno <yutaro.ono.418@...il.com>, 
	Asahi Lina <lina@...hilina.net>, Danilo Krummrich <dakr@...hat.com>, Tiago Lam <tiagolam@...il.com>, 
	Charalampos Mitrodimas <charmitro@...teo.net>, Tejun Heo <tj@...nel.org>, 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 RESEND v5] rust: str: Use `core::CStr`, remove the custom
 `CStr` implementation

On Mon, Aug 19, 2024 at 10:39 AM Michal Rostecki <vadorovsky@...il.com> wrote:
>
> From: Michal Rostecki <vadorovsky@...il.com>

You don't need this since the email already shows it is already from
you :) Aiui this is only needed when forwarding a patch for someone
else, or if you use a different commit email for some reason.

> `CStr` became a part of `core` library in Rust 1.75. This change replaces
> the custom `CStr` implementation with the one from `core`.

The diff in `kernel/str.rs` is really difficult to read and review
since the new parts get interleaved with the removed lines. Could you
split this into a couple patches? Probably roughly the five described
below:

1. Add all the new things `CStrExt`, `CStrDisplay`, and their implementations.
2. Add `CStrExt` to the prelude (Alice's suggestion)
3. Update existing uses of our `CStr` to instead use `core::CStr`
4. Remove our current `CStr`
5. Change any docs for `CString` or `c_str!`, as relevant

Just remember that things should build after each patch.

> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 0ba77276ae7e..79a50ab59af0 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> @@ -56,13 +56,15 @@ macro_rules! kunit_assert {
>                 break 'out;
>             }
>
> -            static FILE: &'static $crate::str::CStr = $crate::c_str!($file);
> +            static FILE: &'static core::ffi::CStr = $file;
>             static LINE: i32 = core::line!() as i32 - $diff;
> -            static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
> +            static CONDITION: &'static core::ffi::CStr = $crate::c_str!(stringify!($condition));

This change and the associated invocation changes can be dropped since
we are keeping `c_str`. It's cleaner to be able to call macros with
"standard strings" rather than c"c strings" where possible.

> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index bb8d4f41475b..97a298a44b96 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs

(I removed most of the `-` lines for my review below)

> +/// Wrapper around [`CStr`] which implements [`Display`](core::fmt::Display).
> +pub struct CStrDisplay<'a>(&'a CStr);
>
> +impl fmt::Display for CStrDisplay<'_> {
> +    /// Formats printable ASCII characters, escaping the rest.
>      ///
>      /// # Examples
>      ///
>      /// ```
> +    /// # use core::ffi::CStr;
>      /// # use kernel::c_str;
>      /// # use kernel::fmt;
> +    /// # use kernel::str::{CStrExt, CString};
> +    /// let penguin = c"🐧";
> +    /// let s = CString::try_from_fmt(fmt!("{}", penguin.display())).unwrap();
> +    /// assert_eq!(s.to_bytes_with_nul(), "\\xf0\\x9f\\x90\\xa7\0".as_bytes());
> +    ///
> +    /// let ascii = c"so \"cool\"";
> +    /// let s = CString::try_from_fmt(fmt!("{}", ascii.display())).unwrap();
> +    /// assert_eq!(s.to_bytes_with_nul(), "so \"cool\"\0".as_bytes());
>      /// ```
>      fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

You don't need docs on the `Display` impl since that is more or less
innate. Docs should indeed be on `fn display()`, which you have.

> +/// Extensions to [`CStr`].
> +pub trait CStrExt {
> +    /// Returns an object that implements [`Display`](core::fmt::Display) for
> +    /// safely printing a [`CStr`] that may contain non-ASCII data, which are
> +    /// escaped.

Just split this into two sentences, e.g.

    /// Returns an object ... for safely printing a [`CStr`].
    ///
    /// If the `CStr` contains non-ASCII data, it is escaped.

> +    ///
> +    /// # Examples
>      ///
>      /// ```
> +    /// # use core::ffi::CStr;
>      /// # use kernel::c_str;
>      /// # use kernel::fmt;
> +    /// # use kernel::str::{CStrExt, CString};
> +    /// let penguin = c"🐧";
> +    /// let s = CString::try_from_fmt(fmt!("{}", penguin.display())).unwrap();
> +    /// assert_eq!(s.to_bytes_with_nul(), "\\xf0\\x9f\\x90\\xa7\0".as_bytes());
> +    ///
> +    /// let ascii = c"so \"cool\"";
> +    /// let s = CString::try_from_fmt(fmt!("{}", ascii.display())).unwrap();
> +    /// assert_eq!(s.to_bytes_with_nul(), "so \"cool\"\0".as_bytes());
>      /// ```
> +    fn display(&self) -> CStrDisplay<'_>;

Nit: Could you swap the ascii and penguin examples so the easier one
is first? Also I would remove the extra quote chars `\"` since it
makes things tougher to read without demonstrating much.

> +    /// Creates a mutable [`CStr`] from a `[u8]` without performing any
> +    /// additional checks.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `bytes` *must* end with a `NUL` byte, and should only have a single
> +    /// `NUL` byte (or the string will be truncated).
> +    unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut Self;
>  }

+1 to Alice's suggestion of removing this and leaving `DerefMut` if
that works for our usecases.

If we leave this, just copy the `# Safety` section from
`CStr::from_bytes_with_nul_unchecked` since I think this could use
some improved wording (and "or the string will be truncated" is not
accurate - any number of things could break, it doesn't just become a
shorter string).

>  /// Creates a new [`CStr`] from a string literal.
>  ///
> +/// This macro is not needed when C-string literals (`c"hello"` syntax) can be
> +/// used directly, but can be used when a C-string version of a standard string
> +/// literal is required (often when working with macros).
> +///
> +/// The string should not contain any `NUL` bytes.

For the last line, maybe

    /// # Panics
    ///
    /// This macro panics if the string contains an interior `NUL` byte.

>  /// # Examples
>  ///
>  /// ```
> +/// # use core::ffi::CStr;
>  /// # use kernel::c_str;
> +/// const MY_CSTR: &CStr = c_str!(stringify!(5));
>  /// ```
>  #[macro_export]
>  macro_rules! c_str {
>      ($str:expr) => {{
>          const S: &str = concat!($str, "\0");
> +        const C: &core::ffi::CStr = match core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) {
>              Ok(v) => v,
>              Err(_) => panic!("string contains interior NUL"),
>          };

Thanks for the updates from last time. For what it's worth, this is
the first time an email from this series has come through for me with
no problems (not getting grouped in the same thread as all other
versions in my client) so whatever it is, do the same thing next time
:)

- Trevor

[1]: https://doc.rust-lang.org/std/ffi/struct.CStr.html#method.from_bytes_with_nul_unchecked

Powered by blists - more mailing lists