Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Benno Lossin]

On Mon Mar 17, 2025 at 12:34 PM CET, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>>
>> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> > Add `Vec::dec_len` that reduces the length of the receiver. This method
>> > is intended to be used from methods that remove elements from `Vec` such
>> > as `truncate`, `pop`, `remove`, and others. This method is intentionally
>> > not `pub`.
>>
>> I think it should be `pub`. Otherwise we're loosing functionality
>> compared to now. If one decides to give the raw pointer to some C API
>> that takes ownership of the pointer, then I want them to be able to call
>> `dec_len` manually.
>
> This is premature. It is trivial to make this function pub when the need arises.

And it's trivial to do it now. If it's private now, someone will have to
change this in some random patch and it's annoying.

>> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
>> > ---
>> >  rust/kernel/alloc/kvec.rs | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> > index d43a1d609434..5d604e04b9a5 100644
>> > --- a/rust/kernel/alloc/kvec.rs
>> > +++ b/rust/kernel/alloc/kvec.rs
>> > @@ -195,6 +195,21 @@ pub unsafe fn inc_len(&mut self, additional: usize) {
>> >          self.len += additional;
>> >      }
>> >
>> > +    /// Decreases `self.len` by `count`.
>> > +    ///
>> > +    /// Returns a mutable reference to the removed elements.
>>
>> s/reference/slice/
>>
>> I would also mention here that the elements won't be dropped when the
>> user doesn't do that manually using the slice. So explain that this is a
>> low-level operation and `clear` or `truncate` should be used instead
>> where possible.
>
> Neither function exists. I've added a description of the semantics of the slice.

Fair point, would still be nice to point users to these when they exist.

>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// - `count` must be less than or equal to `self.len`.
>>
>> I also think that we should use saturating_sub instead and then not have
>> to worry about this. (It should still be documented in the function
>> though). That way this can also be a safe function.
>
> This doesn't seem better to me. I'd prefer to have more rather than
> fewer guardrails on such low-level operations.

Your second sentence seems like an argument for making it safe? I think
it's a lot better as a safe function.

---
Cheers,
Benno