Re: [PATCH v3 1/2] rust/revocable: add try_access_with() convenience method

[Benno Lossin]

On Sun Apr 6, 2025 at 3:58 PM CEST, Alexandre Courbot wrote:
> Revocable::try_access() returns a guard through which the wrapped object
> can be accessed. Code that can sleep is not allowed while the guard is
> held; thus, it is common for the caller to explicitly drop it before
> running sleepable code, e.g:
>
>     let b = bar.try_access()?;
>     let reg = b.readl(...);
>
>     // Don't forget this or things could go wrong!
>     drop(b);
>
>     something_that_might_sleep();
>
>     let b = bar.try_access()?;
>     let reg2 = b.readl(...);
>
> This is arguably error-prone. try_access_with() provides an arguably
> safer alternative, by taking a closure that is run while the guard is
> held, and by dropping the guard automatically after the closure
> completes. This way, code can be organized more clearly around the
> critical sections and the risk of forgetting to release the guard when
> needed is considerably reduced:
>
>     let reg = bar.try_access_with(|b| b.readl(...))?;
>
>     something_that_might_sleep();
>
>     let reg2 = bar.try_access_with(|b| b.readl(...))?;
>
> The closure can return nothing, or any value including a Result which is
> then wrapped inside the Option returned by try_access_with. Error
> management is driver-specific, so users are encouraged to create their
> own macros that map and flatten the returned values to something
> appropriate for the code they are working on.
>
> Acked-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> Suggested-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx>

> ---
>  rust/kernel/revocable.rs | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 1e5a9d25c21b279b01f90b02997492aa4880d84f..b91e40e8160be0cc0ff8e0699e48e063c9dbce22 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -123,6 +123,22 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
>          }
>      }
>  
> +    /// Tries to access the wrapped object and run a closure on it while the guard is 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 method
> +    /// adds an extra safety to make sure it doesn't happen.
> +    ///
> +    /// Returns `None` if the object has been revoked and is therefore no longer accessible, or the
> +    /// result of the closure wrapped in `Some`. If the closure returns a [`Result`] then the
> +    /// return type becomes `Option<Result<>>`, which can be inconvenient. Users are encouraged to
> +    /// define their own macro that turns the `Option` into a proper error code and flattens the
> +    /// inner result into it if it makes sense within their subsystem.

I personally wouldn't have mentioned this in the docs, since to me such
a helper would be obvious, but I don't mind it either.

---
Cheers,
Benno

> +    pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> +        self.try_access().map(|t| f(&*t))
> +    }
> +
>      /// # Safety
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.