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
