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 that the caller will explicitly need to 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_with() and try_with_ok() 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 is forgetting to release the guard when
needed is considerably reduced:

    let reg = bar.try_with_ok(|b| b.readl(...))?;

    something_that_might_sleep();

    let reg2 = bar.try_with_ok(|b| b.readl(...))?;

Unlike try_access() which returns an Option, try_with() and
try_with_ok() return Err(ENXIO) if the object cannot be acquired. The
Option returned by try_access() is typically converted to an error in
practice, so this saves one step for the caller.

try_with() requires the callback itself to return a Result that is
passed to the caller. try_with_ok() accepts a callback that never fails.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
This is a feature I found useful to have while writing Nova driver code
that accessed registers alongside other operations. I would find myself
quite confused about whether the guard was held or dropped at a given
point of the code, and it felt like walking through a minefield ; this
pattern makes things safer and easier to read IMHO.
---
 rust/kernel/revocable.rs | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

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
+    /// method adds an extra safety to make sure it doesn't happen.
+    ///
+    /// 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> {
+        self.try_access().ok_or(ENXIO).and_then(|t| f(&*t))
+    }
+
+    /// Tries to access the wrapped object and run the closure `f` on it with the guard held.
+    ///
+    /// This is the same as [`Self::try_with`], with the exception that `f` is expected to
+    /// always succeed and thus does not need to return a `Result`. Thus the only error case is if
+    /// the wrapped object has been revoked.
+    pub fn try_with_ok<R, F: Fn(&T) -> R>(&self, f: F) -> Result<R> {
+        self.try_with(|t| Ok(f(t)))
+    }
+
     /// Tries to access the revocable wrapped object.
     ///
     /// Returns `None` if the object has been revoked and is therefore no longer accessible.

---
base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
change-id: 20250313-try_with-cc9f91dd3b60

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

Hi Alex,

Thanks for looking into this!

On Thu, Mar 13, 2025 at 09:40:59PM +0900, 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 that the caller will explicitly need to 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(...);

Ideally, we get klint to protect us against those kind of mistakes too.

> This is arguably error-prone. try_with() and try_with_ok() 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 is forgetting to release the guard when
> needed is considerably reduced:
> 
>     let reg = bar.try_with_ok(|b| b.readl(...))?;
> 
>     something_that_might_sleep();
> 
>     let reg2 = bar.try_with_ok(|b| b.readl(...))?;

However, that's much more convenient and a great improvement.

Feel free to add

	Acked-by: Danilo Krummrich <dakr@kernel.org>

> 
> Unlike try_access() which returns an Option, try_with() and
> try_with_ok() return Err(ENXIO) if the object cannot be acquired. The
> Option returned by try_access() is typically converted to an error in
> practice, so this saves one step for the caller.
> 
> try_with() requires the callback itself to return a Result that is
> passed to the caller. try_with_ok() accepts a callback that never fails.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Since I proposed something like that in one of the nova threads (and in Zulip),
feel free to also add

	Suggested-by: Danilo Krummrich <dakr@kernel.org>

> ---
> This is a feature I found useful to have while writing Nova driver code
> that accessed registers alongside other operations. I would find myself
> quite confused about whether the guard was held or dropped at a given
> point of the code, and it felt like walking through a minefield ; this
> pattern makes things safer and easier to read IMHO.
> ---
>  rust/kernel/revocable.rs | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> 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
> +    /// method adds an extra safety to make sure it doesn't happen.
> +    ///
> +    /// 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> {
> +        self.try_access().ok_or(ENXIO).and_then(|t| f(&*t))
> +    }
> +
> +    /// Tries to access the wrapped object and run the closure `f` on it with the guard held.
> +    ///
> +    /// This is the same as [`Self::try_with`], with the exception that `f` is expected to
> +    /// always succeed and thus does not need to return a `Result`. Thus the only error case is if
> +    /// the wrapped object has been revoked.
> +    pub fn try_with_ok<R, F: Fn(&T) -> R>(&self, f: F) -> Result<R> {
> +        self.try_with(|t| Ok(f(t)))
> +    }
> +
>      /// Tries to access the revocable wrapped object.
>      ///
>      /// Returns `None` if the object has been revoked and is therefore no longer accessible.
> 
> ---
> base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
> change-id: 20250313-try_with-cc9f91dd3b60
> 
> Best regards,
> -- 
> Alexandre Courbot <acourbot@nvidia.com>
>

On Thu Mar 13, 2025 at 11:37 PM JST, Danilo Krummrich wrote:
> Hi Alex,
>
> Thanks for looking into this!
>
> On Thu, Mar 13, 2025 at 09:40:59PM +0900, 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 that the caller will explicitly need to 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(...);
>
> Ideally, we get klint to protect us against those kind of mistakes too.

Yes, but even with klint I find it easier to delimitate the critical
sections explicitly and not having to remember about dropping the guard
when needed.

>
>> This is arguably error-prone. try_with() and try_with_ok() 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 is forgetting to release the guard when
>> needed is considerably reduced:
>> 
>>     let reg = bar.try_with_ok(|b| b.readl(...))?;
>> 
>>     something_that_might_sleep();
>> 
>>     let reg2 = bar.try_with_ok(|b| b.readl(...))?;
>
> However, that's much more convenient and a great improvement.
>
> Feel free to add
>
> 	Acked-by: Danilo Krummrich <dakr@kernel.org>

Thanks!

>
>> 
>> Unlike try_access() which returns an Option, try_with() and
>> try_with_ok() return Err(ENXIO) if the object cannot be acquired. The
>> Option returned by try_access() is typically converted to an error in
>> practice, so this saves one step for the caller.
>> 
>> try_with() requires the callback itself to return a Result that is
>> passed to the caller. try_with_ok() accepts a callback that never fails.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Since I proposed something like that in one of the nova threads (and in Zulip),
> feel free to also add
>
> 	Suggested-by: Danilo Krummrich <dakr@kernel.org>

Will do. I wasn't aware of this discussion, please let me know if I have
omitted something from your suggestion (like better method names, for
instance :)).

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).

> +    ///
> +    /// 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>`.

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

---
Cheers,
Benno

> +        self.try_access().ok_or(ENXIO).and_then(|t| f(&*t))
> +    }
> +
> +    /// Tries to access the wrapped object and run the closure `f` on it with the guard held.
> +    ///
> +    /// This is the same as [`Self::try_with`], with the exception that `f` is expected to
> +    /// always succeed and thus does not need to return a `Result`. Thus the only error case is if
> +    /// the wrapped object has been revoked.
> +    pub fn try_with_ok<R, F: Fn(&T) -> R>(&self, f: F) -> Result<R> {
> +        self.try_with(|t| Ok(f(t)))
> +    }
> +
>      /// Tries to access the revocable wrapped object.
>      ///
>      /// Returns `None` if the object has been revoked and is therefore no longer accessible.
>
> ---
> base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
> change-id: 20250313-try_with-cc9f91dd3b60
>
> Best regards,

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?

On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
> On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
>> 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?

Sure, if you're always doing

    let guard = rev.try_access().ok_or(ENXIO)?;

Then it makes sense from my view, maybe Danilo has some other argument
for why `Option` is better.

---
Cheers,
Benno

On Thu, Mar 13, 2025 at 03:38:55PM +0000, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
> > On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
> >> 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?
> 
> Sure, if you're always doing
> 
>     let guard = rev.try_access().ok_or(ENXIO)?;
> 
> Then it makes sense from my view, maybe Danilo has some other argument
> for why `Option` is better.

Most of the time I think we indeed want to derive an Err() if try_access()
fails, but not with a specific error code. The error code depends on the context
of where the revocable is used (e.g. for I/O mappings), but it also depends on
the driver semantics.

On Thu Mar 13, 2025 at 4:48 PM CET, Danilo Krummrich wrote:
> On Thu, Mar 13, 2025 at 03:38:55PM +0000, Benno Lossin wrote:
>> On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
>> > On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
>> >> 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?
>> 
>> Sure, if you're always doing
>> 
>>     let guard = rev.try_access().ok_or(ENXIO)?;
>> 
>> Then it makes sense from my view, maybe Danilo has some other argument
>> for why `Option` is better.
>
> Most of the time I think we indeed want to derive an Err() if try_access()
> fails, but not with a specific error code. The error code depends on the context
> of where the revocable is used (e.g. for I/O mappings), but it also depends on
> the driver semantics.

In that case a single function with this signature would make sense:

    fn access_with<R>(&self, f: impl FnOnce(&T) -> R) -> Option<R>;

If there are common usages that always return the same error code, then
we could add them as functions with `Result`.

---
Cheers,
Benno

On Fri Mar 14, 2025 at 2:50 AM JST, Benno Lossin wrote:
> On Thu Mar 13, 2025 at 4:48 PM CET, Danilo Krummrich wrote:
>> On Thu, Mar 13, 2025 at 03:38:55PM +0000, Benno Lossin wrote:
>>> On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
>>> > On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
>>> >> 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?
>>> 
>>> Sure, if you're always doing
>>> 
>>>     let guard = rev.try_access().ok_or(ENXIO)?;
>>> 
>>> Then it makes sense from my view, maybe Danilo has some other argument
>>> for why `Option` is better.
>>
>> Most of the time I think we indeed want to derive an Err() if try_access()
>> fails, but not with a specific error code. The error code depends on the context
>> of where the revocable is used (e.g. for I/O mappings), but it also depends on
>> the driver semantics.
>
> In that case a single function with this signature would make sense:
>
>     fn access_with<R>(&self, f: impl FnOnce(&T) -> R) -> Option<R>;
>
> If there are common usages that always return the same error code, then
> we could add them as functions with `Result`.

Yeah the more I think about it the more this seems to make sense,
from a strictly logical point of view.

Where I am still on the fence is that the goal is also to reduce the
friction introduced by the Revocable business, which a large driver
might need to interact with hundreds of times. If the user wants the
callback to return a Result, then this method will return an
Option<Result>. One would then need to ok_or the Option, then flatten
the two results, which is a bit verbose.

I suppose drivers could add their own macros to do that automatically
and reduce code verbosity, at the cost of less cohesion across drivers.
Guess I'll go with that if I cannot come with anything better.

On Sat, Mar 15, 2025 at 11:07:44PM +0900, Alexandre Courbot wrote:
> On Fri Mar 14, 2025 at 2:50 AM JST, Benno Lossin wrote:
> > On Thu Mar 13, 2025 at 4:48 PM CET, Danilo Krummrich wrote:
> >> On Thu, Mar 13, 2025 at 03:38:55PM +0000, Benno Lossin wrote:
> >>> On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
> >>> > On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
> >>> >> 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?
> >>> 
> >>> Sure, if you're always doing
> >>> 
> >>>     let guard = rev.try_access().ok_or(ENXIO)?;
> >>> 
> >>> Then it makes sense from my view, maybe Danilo has some other argument
> >>> for why `Option` is better.
> >>
> >> Most of the time I think we indeed want to derive an Err() if try_access()
> >> fails, but not with a specific error code. The error code depends on the context
> >> of where the revocable is used (e.g. for I/O mappings), but it also depends on
> >> the driver semantics.
> >
> > In that case a single function with this signature would make sense:
> >
> >     fn access_with<R>(&self, f: impl FnOnce(&T) -> R) -> Option<R>;
> >
> > If there are common usages that always return the same error code, then
> > we could add them as functions with `Result`.
> 
> Yeah the more I think about it the more this seems to make sense,
> from a strictly logical point of view.
> 
> Where I am still on the fence is that the goal is also to reduce the
> friction introduced by the Revocable business, which a large driver
> might need to interact with hundreds of times. If the user wants the
> callback to return a Result, then this method will return an
> Option<Result>. One would then need to ok_or the Option, then flatten
> the two results, which is a bit verbose.

I think you see this from the perspective of one specific usecase, i.e.
Devres<T>, where T dereferences to Io, right?

> I suppose drivers could add their own macros to do that automatically
> and reduce code verbosity, at the cost of less cohesion across drivers.
> Guess I'll go with that if I cannot come with anything better.

Maybe we could do something more specific but yet generic on top (for the
use-case above), but we still can't assume the exact error code a driver wants
to derive from failing try_access(). So, maybe a driver specific wrapper is
indeed what you want on top of what this patch provides.

On Sat Mar 15, 2025 at 11:17 PM JST, Danilo Krummrich wrote:
> On Sat, Mar 15, 2025 at 11:07:44PM +0900, Alexandre Courbot wrote:
>> On Fri Mar 14, 2025 at 2:50 AM JST, Benno Lossin wrote:
>> > On Thu Mar 13, 2025 at 4:48 PM CET, Danilo Krummrich wrote:
>> >> On Thu, Mar 13, 2025 at 03:38:55PM +0000, Benno Lossin wrote:
>> >>> On Thu Mar 13, 2025 at 4:08 PM CET, Alexandre Courbot wrote:
>> >>> > On Thu Mar 13, 2025 at 11:19 PM JST, Benno Lossin wrote:
>> >>> >> 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?
>> >>> 
>> >>> Sure, if you're always doing
>> >>> 
>> >>>     let guard = rev.try_access().ok_or(ENXIO)?;
>> >>> 
>> >>> Then it makes sense from my view, maybe Danilo has some other argument
>> >>> for why `Option` is better.
>> >>
>> >> Most of the time I think we indeed want to derive an Err() if try_access()
>> >> fails, but not with a specific error code. The error code depends on the context
>> >> of where the revocable is used (e.g. for I/O mappings), but it also depends on
>> >> the driver semantics.
>> >
>> > In that case a single function with this signature would make sense:
>> >
>> >     fn access_with<R>(&self, f: impl FnOnce(&T) -> R) -> Option<R>;
>> >
>> > If there are common usages that always return the same error code, then
>> > we could add them as functions with `Result`.
>> 
>> Yeah the more I think about it the more this seems to make sense,
>> from a strictly logical point of view.
>> 
>> Where I am still on the fence is that the goal is also to reduce the
>> friction introduced by the Revocable business, which a large driver
>> might need to interact with hundreds of times. If the user wants the
>> callback to return a Result, then this method will return an
>> Option<Result>. One would then need to ok_or the Option, then flatten
>> the two results, which is a bit verbose.
>
> I think you see this from the perspective of one specific usecase, i.e.
> Devres<T>, where T dereferences to Io, right?

Indeed.

>
>> I suppose drivers could add their own macros to do that automatically
>> and reduce code verbosity, at the cost of less cohesion across drivers.
>> Guess I'll go with that if I cannot come with anything better.
>
> Maybe we could do something more specific but yet generic on top (for the
> use-case above), but we still can't assume the exact error code a driver wants
> to derive from failing try_access(). So, maybe a driver specific wrapper is
> indeed what you want on top of what this patch provides.

So be it! It's not that bad and more flexible in the end.

On Sat Mar 15, 2025 at 3:26 PM CET, Alexandre Courbot wrote:
> On Sat Mar 15, 2025 at 11:17 PM JST, Danilo Krummrich wrote:
>> On Sat, Mar 15, 2025 at 11:07:44PM +0900, Alexandre Courbot wrote:
>>> I suppose drivers could add their own macros to do that automatically
>>> and reduce code verbosity, at the cost of less cohesion across drivers.
>>> Guess I'll go with that if I cannot come with anything better.
>>
>> Maybe we could do something more specific but yet generic on top (for the
>> use-case above), but we still can't assume the exact error code a driver wants
>> to derive from failing try_access(). So, maybe a driver specific wrapper is
>> indeed what you want on top of what this patch provides.
>
> So be it! It's not that bad and more flexible in the end.

You could have the following signature:

    fn try_access_with<R>(&self, on_vacant: Error, f: impl FnOnce(&T) -> Result<R>) -> Result<R>;

That will use the `on_vacant` error instead of hard coding ENXIO. But
maybe it's better to just have such a wrapper in drivers that actually
need it (ie even with the concrete error specified and not a parameter).
You'll know better through actually trying to write a driver.

---
Cheers,
Benno

On Sun Mar 16, 2025 at 2:48 AM JST, Benno Lossin wrote:
> On Sat Mar 15, 2025 at 3:26 PM CET, Alexandre Courbot wrote:
>> On Sat Mar 15, 2025 at 11:17 PM JST, Danilo Krummrich wrote:
>>> On Sat, Mar 15, 2025 at 11:07:44PM +0900, Alexandre Courbot wrote:
>>>> I suppose drivers could add their own macros to do that automatically
>>>> and reduce code verbosity, at the cost of less cohesion across drivers.
>>>> Guess I'll go with that if I cannot come with anything better.
>>>
>>> Maybe we could do something more specific but yet generic on top (for the
>>> use-case above), but we still can't assume the exact error code a driver wants
>>> to derive from failing try_access(). So, maybe a driver specific wrapper is
>>> indeed what you want on top of what this patch provides.
>>
>> So be it! It's not that bad and more flexible in the end.
>
> You could have the following signature:
>
>     fn try_access_with<R>(&self, on_vacant: Error, f: impl FnOnce(&T) -> Result<R>) -> Result<R>;
>
> That will use the `on_vacant` error instead of hard coding ENXIO. But
> maybe it's better to just have such a wrapper in drivers that actually
> need it (ie even with the concrete error specified and not a parameter).
> You'll know better through actually trying to write a driver.

Yeah, having the extra on_vacant parameter would require callers to
specify the error they need every time, where it is supposed to be
a per-driver constant. So I guess a per-driver macro or wrapper would be
more ergonomic in the end.

On Sun, Mar 16, 2025 at 09:20:03PM +0900, Alexandre Courbot wrote:
> On Sun Mar 16, 2025 at 2:48 AM JST, Benno Lossin wrote:
> > On Sat Mar 15, 2025 at 3:26 PM CET, Alexandre Courbot wrote:
> >> On Sat Mar 15, 2025 at 11:17 PM JST, Danilo Krummrich wrote:
> >>> On Sat, Mar 15, 2025 at 11:07:44PM +0900, Alexandre Courbot wrote:
> >>>> I suppose drivers could add their own macros to do that automatically
> >>>> and reduce code verbosity, at the cost of less cohesion across drivers.
> >>>> Guess I'll go with that if I cannot come with anything better.
> >>>
> >>> Maybe we could do something more specific but yet generic on top (for the
> >>> use-case above), but we still can't assume the exact error code a driver wants
> >>> to derive from failing try_access(). So, maybe a driver specific wrapper is
> >>> indeed what you want on top of what this patch provides.
> >>
> >> So be it! It's not that bad and more flexible in the end.
> >
> > You could have the following signature:
> >
> >     fn try_access_with<R>(&self, on_vacant: Error, f: impl FnOnce(&T) -> Result<R>) -> Result<R>;
> >
> > That will use the `on_vacant` error instead of hard coding ENXIO. But
> > maybe it's better to just have such a wrapper in drivers that actually
> > need it (ie even with the concrete error specified and not a parameter).
> > You'll know better through actually trying to write a driver.
> 
> Yeah, having the extra on_vacant parameter would require callers to
> specify the error they need every time, where it is supposed to be
> a per-driver constant.

I think the signature proposed by Benno is useful, since even specific drivers
may return a different error code depending on the semantic context of where
this call fails.

> So I guess a per-driver macro or wrapper would be more ergonomic in the end.

I think this makes sense as an addition on top of the above.