On Fri, Jul 11, 2025 at 11:29 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> "Alice Ryhl" <aliceryhl@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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.
I don't mind. I just thought it was simpler since you only need to
support a single write instead of having to support multiple writes.
Alice
