Re: [PATCH v3] rust: add bindings and API for bitmap.h and bitops.h.

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Mar 10, 2025 at 12:44:59PM -0400, Tamir Duberstein wrote:
> Hi Burak, some comments inline. Hopefully I haven't missed important
> context from previous versions.
> 
> On Mon, Mar 10, 2025 at 12:21 PM Burak Emir <bqe@google.com> wrote:
> > +/// # Invariants
> > +///
> > +/// * `ptr` is obtained from a successful call to `bitmap_zalloc` and
> > +///   holds the address of an initialized array of unsigned long
> > +///   that is large enough to hold `nbits` bits.
> > +pub struct Bitmap {
> > +    /// Pointer to an array of unsigned long.
> > +    ptr: NonNull<usize>,
> > +    /// How many bits this bitmap stores. Must be < 2^32.
> > +    nbits: usize,
> 
> How come this isn't held as u32? There's a lot of conversion to u32
> sprinkled around.

Then we would need conversions to usize in other places. I think the
right choice of type in the public API here is usize, but the underlying
C API uses int in various places.

> > +#[cold]
> > +fn panic_not_in_bounds_lt(arg: &'static str, len: usize, val: usize) -> ! {
> > +    panic!("{arg} must be less than length {len}, was {val}");
> > +}
> 
> Have you considered using build_error or returning an error?

I think using build error for these is a bad idea. It's a hack that Rust
doesn't support well, and the optimizer will not always be able to prove
that the integer is in bounds.

> > +    /// Constructs a new [`Bitmap`].
> > +    ///
> > +    /// Fails with AllocError if `nbits` is greater than or equal to 2^32,
> > +    /// or when the bitmap could not be allocated.
> > +    ///
> > +    /// # Example
> > +    ///
> > +    /// ```
> > +    /// # use kernel::bitmap::Bitmap;
> > +    ///
> > +    /// fn new_bitmap() -> Bitmap {
> > +    ///   Bitmap::new(128, GFP_KERNEL).unwrap()
> > +    /// }
> > +    /// ```
> > +    #[inline]
> > +    pub fn new(nbits: usize, flags: Flags) -> Result<Self, AllocError> {
> > +        if let Ok(nbits_u32) = u32::try_from(nbits) {
> > +            // SAFETY: nbits == 0 is permitted and nbits fits in u32.
> > +            let ptr = unsafe { bindings::bitmap_zalloc(nbits_u32, flags.as_raw()) };
> > +            // Zero-size allocation is ok and yields a dangling pointer.
> > +            let ptr = NonNull::new(ptr).ok_or(AllocError)?;
> > +            Ok(Bitmap { ptr, nbits })
> > +        } else {
> > +            Err(AllocError)
> > +        }
> > +    }
> 
> Similar question to above: why return an error here but panic in the setters?

Out of memory is something the caller must handle. There's no way for
the caller to avoid it.

Out of bounds is a bug in the caller. The caller can avoid it by not
passing too large indices.

Alice