[PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`

Andreas Hindborg posted 14 patches 3 months ago
There is a newer version of this series
[PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`
Posted by Andreas Hindborg 3 months ago
Add `NullBorrowFormatter`, a formatter that writes a null terminated string
to an array or slice buffer. Because this type needs to manage the trailing
null marker, the existing formatters cannot be used to implement this type.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 78b2f95eb3171..05d79cf40c201 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -860,6 +860,65 @@ fn deref_mut(&mut self) -> &mut Self::Target {
     }
 }
 
+/// A mutable reference to a byte buffer where a string can be written into.
+///
+/// The buffer will be automatically null terminated after the last written character.
+///
+/// # Invariants
+///
+/// `buffer` is always null terminated.
+pub(crate) struct NullBorrowFormatter<'a> {
+    buffer: &'a mut [u8],
+    pos: usize,
+}
+
+impl<'a> NullBorrowFormatter<'a> {
+    /// Create a new [`Self`] instance.
+    pub(crate) fn new(buffer: &'a mut [u8]) -> Result<NullBorrowFormatter<'a>> {
+        *(buffer.first_mut().ok_or(EINVAL)?) = 0;
+
+        // INVARIANT: We null terminated the buffer above.
+        Ok(Self { buffer, pos: 0 })
+    }
+
+    #[expect(dead_code)]
+    pub(crate) fn from_array<const N: usize>(
+        a: &'a mut [crate::ffi::c_char; N],
+    ) -> Result<NullBorrowFormatter<'a>> {
+        Self::new(
+            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
+            // at least `N` bytes.
+            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
+        )
+    }
+
+    /// Return the position of the write pointer in the underlying buffer.
+    #[expect(dead_code)]
+    pub(crate) fn pos(&self) -> usize {
+        self.pos
+    }
+}
+
+impl Write for NullBorrowFormatter<'_> {
+    fn write_str(&mut self, s: &str) -> fmt::Result {
+        let bytes = s.as_bytes();
+        let len = bytes.len();
+
+        // We want space for a null terminator
+        if self.pos + len > self.buffer.len() - 1 {
+            return Err(fmt::Error);
+        }
+
+        self.buffer[self.pos..self.pos + len].copy_from_slice(bytes);
+        self.pos += len;
+
+        // INVARIANT: The buffer is null terminated.
+        self.buffer[self.pos] = 0;
+
+        Ok(())
+    }
+}
+
 /// An owned string that is guaranteed to have exactly one `NUL` byte, which is at the end.
 ///
 /// Used for interoperability with kernel APIs that take C strings.

-- 
2.47.2
Re: [PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`
Posted by Alice Ryhl 3 months ago
On Tue, Jul 08, 2025 at 09:44:58PM +0200, Andreas Hindborg wrote:
> Add `NullBorrowFormatter`, a formatter that writes a null terminated string
> to an array or slice buffer. Because this type needs to manage the trailing
> null marker, the existing formatters cannot be used to implement this type.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/str.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 78b2f95eb3171..05d79cf40c201 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -860,6 +860,65 @@ fn deref_mut(&mut self) -> &mut Self::Target {
>      }
>  }
>  
> +/// A mutable reference to a byte buffer where a string can be written into.
> +///
> +/// The buffer will be automatically null terminated after the last written character.
> +///
> +/// # Invariants
> +///
> +/// `buffer` is always null terminated.
> +pub(crate) struct NullBorrowFormatter<'a> {
> +    buffer: &'a mut [u8],
> +    pos: usize,
> +}

Do you need `pos`? Often I see this kind of code subslice `buffer`
instead.

> +impl<'a> NullBorrowFormatter<'a> {
> +    /// Create a new [`Self`] instance.
> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Result<NullBorrowFormatter<'a>> {
> +        *(buffer.first_mut().ok_or(EINVAL)?) = 0;
> +
> +        // INVARIANT: We null terminated the buffer above.
> +        Ok(Self { buffer, pos: 0 })
> +    }

I would probably just use an Option for this constructor.

> +    #[expect(dead_code)]
> +    pub(crate) fn from_array<const N: usize>(
> +        a: &'a mut [crate::ffi::c_char; N],
> +    ) -> Result<NullBorrowFormatter<'a>> {
> +        Self::new(
> +            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
> +            // at least `N` bytes.
> +            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
> +        )
> +    }

Arrays automatically coerce to slices, so I don't think this is
necessary. You can just call `new`.

> +    /// Return the position of the write pointer in the underlying buffer.
> +    #[expect(dead_code)]
> +    pub(crate) fn pos(&self) -> usize {
> +        self.pos
> +    }

You delete this function in one of the later patches, so it makes more
sense not to add it.

> +}
> +
> +impl Write for NullBorrowFormatter<'_> {
> +    fn write_str(&mut self, s: &str) -> fmt::Result {
> +        let bytes = s.as_bytes();
> +        let len = bytes.len();
> +
> +        // We want space for a null terminator
> +        if self.pos + len > self.buffer.len() - 1 {

Integer overflow?

> +            return Err(fmt::Error);
> +        }
> +
> +        self.buffer[self.pos..self.pos + len].copy_from_slice(bytes);
> +        self.pos += len;
> +
> +        // INVARIANT: The buffer is null terminated.
> +        self.buffer[self.pos] = 0;
> +
> +        Ok(())
> +    }
> +}
> +
>  /// An owned string that is guaranteed to have exactly one `NUL` byte, which is at the end.
>  ///
>  /// Used for interoperability with kernel APIs that take C strings.
> 
> -- 
> 2.47.2
> 
>
Re: [PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`
Posted by Andreas Hindborg 2 months, 4 weeks ago
"Alice Ryhl" <aliceryhl@google.com> writes:

> On Tue, Jul 08, 2025 at 09:44:58PM +0200, Andreas Hindborg wrote:
>> Add `NullBorrowFormatter`, a formatter that writes a null terminated string
>> to an array or slice buffer. Because this type needs to manage the trailing
>> null marker, the existing formatters cannot be used to implement this type.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/str.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index 78b2f95eb3171..05d79cf40c201 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -860,6 +860,65 @@ fn deref_mut(&mut self) -> &mut Self::Target {
>>      }
>>  }
>>
>> +/// A mutable reference to a byte buffer where a string can be written into.
>> +///
>> +/// The buffer will be automatically null terminated after the last written character.
>> +///
>> +/// # Invariants
>> +///
>> +/// `buffer` is always null terminated.
>> +pub(crate) struct NullBorrowFormatter<'a> {
>> +    buffer: &'a mut [u8],
>> +    pos: usize,
>> +}
>
> Do you need `pos`? Often I see this kind of code subslice `buffer`
> instead.

How would that work? Can I move the start index of `buffer` in some way
without an unsafe block?

>
>> +impl<'a> NullBorrowFormatter<'a> {
>> +    /// Create a new [`Self`] instance.
>> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Result<NullBorrowFormatter<'a>> {
>> +        *(buffer.first_mut().ok_or(EINVAL)?) = 0;
>> +
>> +        // INVARIANT: We null terminated the buffer above.
>> +        Ok(Self { buffer, pos: 0 })
>> +    }
>
> I would probably just use an Option for this constructor.

OK.

>
>> +    #[expect(dead_code)]
>> +    pub(crate) fn from_array<const N: usize>(
>> +        a: &'a mut [crate::ffi::c_char; N],
>> +    ) -> Result<NullBorrowFormatter<'a>> {
>> +        Self::new(
>> +            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
>> +            // at least `N` bytes.
>> +            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
>> +        )
>> +    }
>
> Arrays automatically coerce to slices, so I don't think this is
> necessary. You can just call `new`.

Nice!

>
>> +    /// Return the position of the write pointer in the underlying buffer.
>> +    #[expect(dead_code)]
>> +    pub(crate) fn pos(&self) -> usize {
>> +        self.pos
>> +    }
>
> You delete this function in one of the later patches, so it makes more
> sense not to add it.

Oops.

>
>> +}
>> +
>> +impl Write for NullBorrowFormatter<'_> {
>> +    fn write_str(&mut self, s: &str) -> fmt::Result {
>> +        let bytes = s.as_bytes();
>> +        let len = bytes.len();
>> +
>> +        // We want space for a null terminator
>> +        if self.pos + len > self.buffer.len() - 1 {
>
> Integer overflow?

In the subtraction? `buffer.len()` is at least 1, because of the type invariant.

Or do you mean I should do a checked add for `self.pos + len`?


Best regards,
Andreas Hindborg
Re: [PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`
Posted by Alice Ryhl 2 months, 4 weeks ago
On Wed, Jul 09, 2025 at 05:49:57PM +0200, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@google.com> writes:
> 
> > On Tue, Jul 08, 2025 at 09:44:58PM +0200, Andreas Hindborg wrote:
> >> Add `NullBorrowFormatter`, a formatter that writes a null terminated string
> >> to an array or slice buffer. Because this type needs to manage the trailing
> >> null marker, the existing formatters cannot be used to implement this type.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >> ---
> >>  rust/kernel/str.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 59 insertions(+)
> >>
> >> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> >> index 78b2f95eb3171..05d79cf40c201 100644
> >> --- a/rust/kernel/str.rs
> >> +++ b/rust/kernel/str.rs
> >> @@ -860,6 +860,65 @@ fn deref_mut(&mut self) -> &mut Self::Target {
> >>      }
> >>  }
> >>
> >> +/// A mutable reference to a byte buffer where a string can be written into.
> >> +///
> >> +/// The buffer will be automatically null terminated after the last written character.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// `buffer` is always null terminated.
> >> +pub(crate) struct NullBorrowFormatter<'a> {
> >> +    buffer: &'a mut [u8],
> >> +    pos: usize,
> >> +}
> >
> > Do you need `pos`? Often I see this kind of code subslice `buffer`
> > instead.
> 
> How would that work? Can I move the start index of `buffer` in some way
> without an unsafe block?

Yes. I think this will work:

let buffer = mem::take(&mut self.buffer);
self.buffer = &mut buffer[pos..];

Temporarily storing an empty slice avoids lifetime issues.

> >> +    #[expect(dead_code)]
> >> +    pub(crate) fn from_array<const N: usize>(
> >> +        a: &'a mut [crate::ffi::c_char; N],
> >> +    ) -> Result<NullBorrowFormatter<'a>> {
> >> +        Self::new(
> >> +            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
> >> +            // at least `N` bytes.
> >> +            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
> >> +        )
> >> +    }
> >
> > Arrays automatically coerce to slices, so I don't think this is
> > necessary. You can just call `new`.
> 
> Nice!

I'm guessing it used to be necessary back when we used core::ffi::c_char
since &[i8;N] doesn't coerce to &[u8]. But now that we use the right
c_char definition, that isn't the case anymore.

> >> +impl Write for NullBorrowFormatter<'_> {
> >> +    fn write_str(&mut self, s: &str) -> fmt::Result {
> >> +        let bytes = s.as_bytes();
> >> +        let len = bytes.len();
> >> +
> >> +        // We want space for a null terminator
> >> +        if self.pos + len > self.buffer.len() - 1 {
> >
> > Integer overflow?
> 
> In the subtraction? `buffer.len()` is at least 1, because of the type invariant.
> 
> Or do you mean I should do a checked add for `self.pos + len`?

Ah, I guess self.pos and len are both <= the length of a slice, which is
at most isize::MAX, so the addition can't overflow an usize. Would be
good to comment this, though.

Alice
Re: [PATCH v2 03/14] rust: str: introduce `NullBorrowFormatter`
Posted by Andreas Hindborg 2 months, 4 weeks ago
"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Jul 09, 2025 at 05:49:57PM +0200, Andreas Hindborg wrote:
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Tue, Jul 08, 2025 at 09:44:58PM +0200, Andreas Hindborg wrote:
>> >> Add `NullBorrowFormatter`, a formatter that writes a null terminated string
>> >> to an array or slice buffer. Because this type needs to manage the trailing
>> >> null marker, the existing formatters cannot be used to implement this type.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >> ---
>> >>  rust/kernel/str.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 59 insertions(+)
>> >>
>> >> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> >> index 78b2f95eb3171..05d79cf40c201 100644
>> >> --- a/rust/kernel/str.rs
>> >> +++ b/rust/kernel/str.rs
>> >> @@ -860,6 +860,65 @@ fn deref_mut(&mut self) -> &mut Self::Target {
>> >>      }
>> >>  }
>> >>
>> >> +/// A mutable reference to a byte buffer where a string can be written into.
>> >> +///
>> >> +/// The buffer will be automatically null terminated after the last written character.
>> >> +///
>> >> +/// # Invariants
>> >> +///
>> >> +/// `buffer` is always null terminated.
>> >> +pub(crate) struct NullBorrowFormatter<'a> {
>> >> +    buffer: &'a mut [u8],
>> >> +    pos: usize,
>> >> +}
>> >
>> > Do you need `pos`? Often I see this kind of code subslice `buffer`
>> > instead.
>>
>> How would that work? Can I move the start index of `buffer` in some way
>> without an unsafe block?
>
> Yes. I think this will work:
>
> let buffer = mem::take(&mut self.buffer);
> self.buffer = &mut buffer[pos..];
>
> Temporarily storing an empty slice avoids lifetime issues.

Ah, that is neat.


Best regards,
Andreas Hindborg