Re: [PATCH] rust/revocable: add try_with() convenience method

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 1:40 PM CET, Alexandre Courbot wrote:
>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> index 1e5a9d25c21b279b01f90b02997492aa4880d84f..0157b20373b5b2892cb618b46958bfe095e428b6 100644
>> --- a/rust/kernel/revocable.rs
>> +++ b/rust/kernel/revocable.rs
>> @@ -105,6 +105,28 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>>          }
>>      }
>>  
>> +    /// Tries to access the wrapped object and run the closure `f` on it with the guard held.
>> +    ///
>> +    /// This is a convenience method to run short non-sleepable code blocks while ensuring the
>> +    /// guard is dropped afterwards. [`Self::try_access`] carries the risk that the caller
>> +    /// will forget to explicitly drop that returned guard before calling sleepable code ; this
>
> Space after `;`?
>
>> +    /// method adds an extra safety to make sure it doesn't happen.
>
> To be clear, you still can call a sleeping function form within the
> closure and have the same issue, but I agree that that should not happen
> accidentally (or at least not as often).

Yes, this is by no means a complete solution to the problem, just a way
to better cope with it.

>
>> +    ///
>> +    /// Returns `Err(ENXIO)` if the wrapped object has been revoked, or the result of `f` after it
>> +    /// has been run.
>> +    pub fn try_with<R, F: Fn(&T) -> Result<R>>(&self, f: F) -> Result<R> {
>
> This (and below) can be a `FnOnce(&T) -> Result<R>`.

Indeed, thanks!

>
> Would it make sense to not use `Result` here and continue with `Option`?

We would have to return an Option<Result<R>> in this case. The current
code folds the closure's Result into the one of the guard's acquisition
for convenience.

Actually, I don't think I have ever used try_access() a single time
without converting its returned Option into a Result. Wouldn't it make
sense to do the opposite, i.e. make try_access() return Err(ENXIO) when
the guard cannot be acquired and document this behavior?