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

[Andreas Hindborg <a.hindborg@kernel.org>]

"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