Re: [PATCH v2 05/14] rust: block: use `NullBorrowFormatter`

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

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Tue, Jul 08, 2025 at 09:45:00PM +0200, Andreas Hindborg wrote:
>> Use the new `NullBorrowFormatter` to write the name of a `GenDisk` to the
>> name buffer. This new formatter automatically adds a trailing null marker
>> after the written characters, so we don't need to append that at the call
>> site any longer.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/block/mq/gen_disk.rs   | 8 ++++----
>>  rust/kernel/block/mq/raw_writer.rs | 1 +
>>  rust/kernel/str.rs                 | 7 -------
>>  3 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index 679ee1bb21950..e0e42f7028276 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -7,9 +7,10 @@
>>
>>  use crate::{
>>      bindings,
>> -    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>> +    block::mq::{Operations, TagSet},
>>      error::{self, from_err_ptr, Result},
>>      static_lock_class,
>> +    str::NullBorrowFormatter,
>>      sync::Arc,
>>  };
>>  use core::fmt::{self, Write};
>> @@ -143,14 +144,13 @@ pub fn build<T: Operations>(
>>          // SAFETY: `gendisk` is a valid pointer as we initialized it above
>>          unsafe { (*gendisk).fops = &TABLE };
>>
>> -        let mut raw_writer = RawWriter::from_array(
>> +        let mut writer = NullBorrowFormatter::from_array(
>>              // SAFETY: `gendisk` points to a valid and initialized instance. We
>>              // have exclusive access, since the disk is not added to the VFS
>>              // yet.
>>              unsafe { &mut (*gendisk).disk_name },
>>          )?;
>> -        raw_writer.write_fmt(name)?;
>> -        raw_writer.write_char('\0')?;
>> +        writer.write_fmt(name)?;
>
> Although this is nicer than the existing code, I wonder if it should
> just be a function rather than a whole NullBorrowFormatter struct? Take
> a slice and a fmt::Arguments and write it with a nul-terminator. Do you
> need anything more complex than what you have here?

I don't need anything more complex right now. But I think the
`NullTerminatedFormatter` could be useful anyway:

  +/// 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 NullTerminatedFormatter<'a> {
  +    buffer: &'a mut [u8],
  +}
  +
  +impl<'a> NullTerminatedFormatter<'a> {
  +    /// Create a new [`Self`] instance.
  +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
  +        *(buffer.first_mut()?) = 0;
  +
  +        // INVARIANT: We null terminated the buffer above.
  +        Some(Self { buffer })
  +    }
  +
  +    pub(crate) fn from_array<const N: usize>(
  +        buffer: &'a mut [crate::ffi::c_char; N],
  +    ) -> Option<NullTerminatedFormatter<'a>> {
  +        Self::new(buffer)
  +    }
  +}
  +
  +impl Write for NullTerminatedFormatter<'_> {
  +    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. Buffer length is always at least 1, so no overflow.
  +        if len > self.buffer.len() - 1 {
  +            return Err(fmt::Error);
  +        }
  +
  +        let buffer = core::mem::take(&mut self.buffer);
  +        // We break the null termination invariant for a short while.
  +        buffer[..len].copy_from_slice(bytes);
  +        self.buffer = &mut buffer[len..];
  +
  +        // INVARIANT: We null terminate the buffer.
  +        self.buffer[0] = 0;
  +
  +        Ok(())
  +    }
  +}
  +

If you insist, I can write something like

  fn format_to_buffer(buffer: &mut [u8], args: fmt::Arguments) -> fmt::Result

although I am not sure I see the point of this change.


Best regards,
Andreas Hindborg