Re: [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray

[Philipp Stanner <pstanner@redhat.com>]

On Mon, 2024-03-18 at 13:10 +0100, Alice Ryhl wrote:
> On Sun, Mar 10, 2024 at 1:00 AM Maíra Canal <mcanal@igalia.com>
> wrote:
> > 
> > From: Asahi Lina <lina@asahilina.net>
> > 
> > The XArray is an abstract data type which behaves like a very large
> > array of pointers. Add a Rust abstraction for this data type.
> > 
> > The initial implementation uses explicit locking on get operations
> > and
> > returns a guard which blocks mutation, ensuring that the referenced
> > object remains alive. To avoid excessive serialization, users are
> > expected to use an inner type that can be efficiently cloned (such
> > as
> > Arc<T>), and eagerly clone and drop the guard to unblock other
> > users
> > after a lookup.
> > 
> > Future variants may support using RCU instead to avoid mutex
> > locking.
> > 
> > This abstraction also introduces a reservation mechanism, which can
> > be
> > used by alloc-capable XArrays to reserve a free slot without
> > immediately
> > filling it, and then do so at a later time. If the reservation is
> > dropped without being filled, the slot is freed again for other
> > users,
> > which eliminates the need for explicit cleanup code.
> > 
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > Co-developed-by: Maíra Canal <mcanal@igalia.com>
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
> 
> Overall looks good to me.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> > +        if ret < 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            guard.dismiss();
> > +            Ok(id as usize)
> > +        }
> 
> You could make this easier to read using to_result.
> 
> to_result(ret)?;
> guard.dismiss();
> Ok(id as usize)

My 2 cents, I'd go for classic kernel style:


if ret < 0 {
    return Err(...);
}

guard.dismiss();
Ok(id as usize)


P.

> 
> Alice
>