Messages in this thread |  | | Date | Sun, 4 Dec 2022 18:05:21 +0000 | From | "Dr. David Alan Gilbert" <> | Subject | Re: [PATCH v2 20/28] rust: str: add `Formatter` type |
| |
* Wedson Almeida Filho (wedsonaf@gmail.com) wrote: > On Sun, 4 Dec 2022 at 15:41, Dr. David Alan Gilbert <dave@treblig.org> wrote: > > > > * ojeda@kernel.org (ojeda@kernel.org) wrote: > > > From: Wedson Almeida Filho <wedsonaf@gmail.com> > > > > > > Add the `Formatter` type, which leverages `RawFormatter`, > > > but fails if callers attempt to write more than will fit > > > in the buffer. > > > > > > In order to so, implement the `RawFormatter::from_buffer()` > > > constructor as well. > > > > > > Co-developed-by: Adam Bratschi-Kaye <ark.email@gmail.com> > > > Signed-off-by: Adam Bratschi-Kaye <ark.email@gmail.com> > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com> > > > Reviewed-by: Gary Guo <gary@garyguo.net> > > > [Reworded, adapted for upstream and applied latest changes] > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > > > --- > > > rust/kernel/str.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 57 insertions(+) > > > > > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > > index a995db36486f..ce207d1b3d2a 100644 > > > --- a/rust/kernel/str.rs > > > +++ b/rust/kernel/str.rs > > > @@ -406,6 +406,23 @@ impl RawFormatter { > > > } > > > } > > > > > > + /// Creates a new instance of [`RawFormatter`] with the given buffer. > > > + /// > > > + /// # Safety > > > + /// > > > + /// The memory region starting at `buf` and extending for `len` bytes must be valid for writes > > > + /// for the lifetime of the returned [`RawFormatter`]. > > > + pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self { > > > + let pos = buf as usize; > > > + // INVARIANT: We ensure that `end` is never less then `buf`, and the safety requirements > > > + // guarantees that the memory region is valid for writes. > > > + Self { > > > + pos, > > > + beg: pos, > > > + end: pos.saturating_add(len), > > > + } > > > + } > > > + > > > /// Returns the current insert position. > > > /// > > > /// N.B. It may point to invalid memory. > > > @@ -439,3 +456,43 @@ impl fmt::Write for RawFormatter { > > > Ok(()) > > > } > > > } > > > + > > > +/// Allows formatting of [`fmt::Arguments`] into a raw buffer. > > > +/// > > > +/// Fails if callers attempt to write more than will fit in the buffer. > > > +pub(crate) struct Formatter(RawFormatter); > > Here we mention that `Formatter` fails if callers attempt to write > more than will fit in the buffer. > > This is in contrast with `RawFormatter`, which doesn't fail in such > cases. There's also a comment there explaining it (not visible in this > patch because it's already there), but I reproduce below: > > /// Allows formatting of [`fmt::Arguments`] into a raw buffer. > /// > /// It does not fail if callers write past the end of the buffer so > that they can calculate the > /// size required to fit everything. > /// > /// # Invariants > /// > /// The memory region between `pos` (inclusive) and `end` (exclusive) > is valid for writes if `pos` > /// is less than `end`. > pub(crate) struct RawFormatter { > > `RawFormatter` is used to implement the "%pA" printf specifier, which > requires this behaviour. > > > > + > > > +impl Formatter { > > > + /// Creates a new instance of [`Formatter`] with the given buffer. > > > + /// > > > + /// # Safety > > > + /// > > > + /// The memory region starting at `buf` and extending for `len` bytes must be valid for writes > > > + /// for the lifetime of the returned [`Formatter`]. > > > + #[allow(dead_code)] > > > + pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self { > > > + // SAFETY: The safety requirements of this function satisfy those of the callee. > > > + Self(unsafe { RawFormatter::from_buffer(buf, len) }) > > > + } > > > +} > > > + > > > +impl Deref for Formatter { > > > + type Target = RawFormatter; > > > + > > > + fn deref(&self) -> &Self::Target { > > > + &self.0 > > > + } > > > +} > > > + > > > +impl fmt::Write for Formatter { > > > + fn write_str(&mut self, s: &str) -> fmt::Result { > > > + self.0.write_str(s)?; > > > + > > > + // Fail the request if we go past the end of the buffer. > > > > Reading this for the first time, I'm surprised by this, perhaps a > > bit more comment is needed? I was expecting that nothing would > > let pos pass end. > > Given the comments I highlight above, do you think we still need more? > > (My impression is that you're reading this without the context I tried > to explain above, and this context may perhaps be sufficient.)
Thanks for the pointer; I guess I find it trickier when I can't see the type in self.0 to immediately see it's RawFormatter, and 'Raw' is abstract enough to need to go hunt to see it's behaviour.
With that context, I wouldn't object to what's there, but how about something like:
// RawFormatter (self.0) still updates pos if the buffer // is too small, but doesn't fail - we want to fail the request.
Dave
> Thanks, > -Wedson -- -----Open up your eyes, open up your mind, open up your code ------- / Dr. David Alan Gilbert | Running GNU/Linux | Happy \ \ dave @ treblig.org | | In Hex / \ _________________________|_____ http://www.treblig.org |_______/
|  |