Re: [PATCH v7 07/10] rust: id_pool: add contiguous ID reservation

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The `reserve_ids` method modifies the pool state immediately and returns a plain `Range<usize>`, breaking the typestate pattern and risking ID leaks.
- [Low] Imports from `crate::` are not formatted using the vertical import style.

Pre-existing issues:
- [High] `reserve_ids` and existing iteration methods panic under `CONFIG_RUST_BITMAP_HARDENED` if `offset >= pool.capacity()`.
--

--- Patch [7]: [PATCH v7 07/10] rust: id_pool: add contiguous ID reservation ---
commit 1c4cba33204f36d476fe12dc8cb9f2594da796ce
Author: Eliot Courtney <[email protected]>

rust: id_pool: add contiguous ID reservation

This commit adds `IdPool::reserve_ids` to allow allocating a contiguous
range of IDs from the pool with a specified offset, count, and alignment.

diff --git a/rust/kernel/id_pool.rs b/rust/kernel/id_pool.rs
index 9494fde701eea..382a5645767fb 100644
--- a/rust/kernel/id_pool.rs
+++ b/rust/kernel/id_pool.rs
@@ -4,8 +4,14 @@
 
 //! Rust API for an ID pool backed by a [`BitmapVec`].
 
+use core::{
+    num::NonZero,
+    ops::Range, //
+};
+
 use crate::alloc::{AllocError, Flags};
 use crate::bitmap::BitmapVec;
+use crate::ptr::Alignment;

[Severity: Low]
This isn't a bug, but could we format these crate:: imports vertically?

The Rust subsystem coding guidelines request that new imports follow the
kernel vertical import style, rather than adding standalone single-line
imports to existing blocks.

 
 /// Represents a dynamic ID pool backed by a [`BitmapVec`].
 ///

[ ... ]

@@ -244,6 +250,32 @@ pub fn find_unused_id(&mut self, offset: usize) -> Option<UnusedId<'_>> {
     pub fn release_id(&mut self, id: usize) {
         self.map.clear_bit(id);
     }
+
+    /// Reserves a contiguous area of `count` IDs at or after `offset`.
+    ///
+    /// The start of the returned area is a multiple of `align`.
+    ///
+    /// Returns the reserved range upon success, or [`None`] if no such area could be found.
+    #[inline]
+    #[must_use]
+    pub fn reserve_ids(
+        &mut self,
+        offset: usize,
+        count: NonZero<usize>,
+        align: Alignment,
+    ) -> Option<Range<usize>> {
+        let start = self.map.next_zero_area(offset, count, align)?;

[Severity: High]
This is a pre-existing issue, but does passing an offset equal to the pool
capacity cause a kernel panic under CONFIG_RUST_BITMAP_HARDENED?

If a caller iterates through the ID pool and the offset reaches the end
of the pool, it appears that self.map.next_zero_area() triggers the
bitmap_assert!(start < self.len()) bounds assertion inside the underlying
bitmap implementation (like next_zero_area_off) instead of gracefully
returning None.

Could this strict assertion be overly restrictive and unsafe for standard loop
patterns over the ID pool?

> +        self.map.set(start, count);
> +        Some(start..start + count.get())
> +    }

[Severity: Medium]
Does returning a plain Range<usize> here risk leaking IDs if the caller
returns early on a subsequent fallible operation?

Since this method modifies the pool state immediately by calling
self.map.set(start, count) but returns a type without a Drop implementation,
any error path (such as memory allocation failing) that propagates with `?`
after the reservation would permanently lock these IDs.

Could this be updated to use the RAII/typestate pattern (similar to UnusedId
used by find_unused_id) to either defer setting the bitmap or properly release
the IDs when dropped?

+
+    /// Releases a contiguous area of IDs.
+    #[inline]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7