Re: [PATCH v4 6/7] rust: alloc: add Vec::remove

[Danilo Krummrich <dakr@kernel.org>]

On Thu, May 01, 2025 at 11:10:46AM +0000, Alice Ryhl wrote:
> On Wed, Apr 30, 2025 at 06:28:48PM +0200, Danilo Krummrich wrote:
> > On Tue, Apr 29, 2025 at 02:44:26PM +0000, Alice Ryhl wrote:
> > > This is needed by Rust Binder in the range allocator, and by upcoming
> > > GPU drivers during firmware initialization.
> > > 
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/alloc/kvec.rs | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index 357f5a37c7b1d15b709a10c162292841eed0e376..0682108951675cbee05faa130e5a9ce72fc343ba 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -386,6 +386,42 @@ pub fn pop(&mut self) -> Option<T> {
> > >          Some(unsafe { removed.read() })
> > >      }
> > >  
> > > +    /// Removes the element at the given index.
> > > +    ///
> > > +    /// # Panics
> > > +    ///
> > > +    /// Panics if the index is out of bounds.
> > 
> > Let's check for the index and return an error instead. I know we also can't
> > prevent OOB index access panics for e.g. slices, but here we can control it.
> 
> Okay, I will return an `Option<T>`.

Hm...to me this looks like it is a real error condition rather than something
optional.

What does it mean if remove() returns None? It really means that the given index
is out of bounds, which is never correct behavior for the caller of the API.

So, I'd argue that None is an unexpected return value for a caller and needs to
be handled in an error path, for which returning a Result is much more
ergonomic and correct, since Result can describe the reason, i.e. EINVAL,
whereas with Option a caller would need to pick an error code itself.