Implement an unsafe direct accessor for the data stored within the
Revocable.
This is useful for cases where we can proof that the data stored within
the Revocable is not and cannot be revoked for the duration of the
lifetime of the returned reference.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
The explicit lifetimes in access() probably don't serve a practical
purpose, but I found them to be useful for documentation purposes.
---
rust/kernel/revocable.rs | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 971d0dc38d83..33535de141ce 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
self.try_access().map(|t| f(&*t))
}
+ /// Directly access the revocable wrapped object.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
+ /// for the duration of `'a`.
+ pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
+ // SAFETY: By the safety requirement of this function it is guaranteed that
+ // `self.data.get()` is a valid pointer to an instance of `T`.
+ unsafe { &*self.data.get() }
+ }
+
/// # Safety
///
/// Callers must ensure that there are no more concurrent users of the revocable object.
--
2.49.0
On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
>
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> ---
> rust/kernel/revocable.rs | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> self.try_access().map(|t| f(&*t))
> }
>
> + /// Directly access the revocable wrapped object.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> + /// for the duration of `'a`.
Ah I missed this in my other email, in case you want to directly refer
to the lifetime, you should keep it defined. I would still remove the
`'s` lifetime though.
> + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> + // SAFETY: By the safety requirement of this function it is guaranteed that
> + // `self.data.get()` is a valid pointer to an instance of `T`.
I don't see how the "not-being revoked" state makes the `data` ptr be
valid. Is that an invariant of `Revocable`? (it's not documented to have
any invariants)
---
Cheers,
Benno
> + unsafe { &*self.data.get() }
> + }
> +
> /// # Safety
> ///
> /// Callers must ensure that there are no more concurrent users of the revocable object.
On Sat, Apr 26, 2025 at 08:24:14PM +0000, Benno Lossin wrote:
> On Sat Apr 26, 2025 at 3:30 PM CEST, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> >
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > ---
> > rust/kernel/revocable.rs | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > self.try_access().map(|t| f(&*t))
> > }
> >
> > + /// Directly access the revocable wrapped object.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > + /// for the duration of `'a`.
>
> Ah I missed this in my other email, in case you want to directly refer
> to the lifetime, you should keep it defined. I would still remove the
> `'s` lifetime though.
> > + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > + // SAFETY: By the safety requirement of this function it is guaranteed that
> > + // `self.data.get()` is a valid pointer to an instance of `T`.
>
> I don't see how the "not-being revoked" state makes the `data` ptr be
> valid. Is that an invariant of `Revocable`? (it's not documented to have
> any invariants)
What else makes it valid?
AFAICS, try_access() and try_access_with_guard() argue the exact same way,
except that the reason for not being revoked is the atomic check and the RCU
read lock.
On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> Implement an unsafe direct accessor for the data stored within the
> Revocable.
>
> This is useful for cases where we can proof that the data stored within
> the Revocable is not and cannot be revoked for the duration of the
> lifetime of the returned reference.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes.
> ---> rust/kernel/revocable.rs | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 971d0dc38d83..33535de141ce 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> self.try_access().map(|t| f(&*t))
> }
>
> + /// Directly access the revocable wrapped object.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> + /// for the duration of `'a`.
> + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
I'm not sure if the `'s` lifetime really carries much meaning here.
I find just (explicit) `'a` on both parameter and return value is clearer to me,
but I'm not sure what others (particularly those not very familiar with rust)
think of this.
Either way:
Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> + // SAFETY: By the safety requirement of this function it is guaranteed that
> + // `self.data.get()` is a valid pointer to an instance of `T`.
> + unsafe { &*self.data.get() }
> + }
> +
> /// # Safety
> ///
> /// Callers must ensure that there are no more concurrent users of the revocable object.
On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > Implement an unsafe direct accessor for the data stored within the
> > Revocable.
> >
> > This is useful for cases where we can proof that the data stored within
> > the Revocable is not and cannot be revoked for the duration of the
> > lifetime of the returned reference.
> >
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> > The explicit lifetimes in access() probably don't serve a practical
> > purpose, but I found them to be useful for documentation purposes.
> > ---> rust/kernel/revocable.rs | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 971d0dc38d83..33535de141ce 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > self.try_access().map(|t| f(&*t))
> > }
> >
> > + /// Directly access the revocable wrapped object.
> > + ///
> > + /// # Safety
> > + ///
> > + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > + /// for the duration of `'a`.
> > + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> I'm not sure if the `'s` lifetime really carries much meaning here.
> I find just (explicit) `'a` on both parameter and return value is clearer to me,
> but I'm not sure what others (particularly those not very familiar with rust)
> think of this.
Yeah, I don't think we need two lifetimes here, the following version
should be fine (with implicit lifetime):
pub unsafe fn access(&self) -> &T { ... }
, because if you do:
let revocable: &'1 Revocable = ...;
...
let t: &'2 T = unsafe { revocable.access() };
'1 should already outlive '2 (i.e. '1: '2).
Regards,
Boqun
>
> Either way:
>
> Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
>
> > + // SAFETY: By the safety requirement of this function it is guaranteed that
> > + // `self.data.get()` is a valid pointer to an instance of `T`.
> > + unsafe { &*self.data.get() }
> > + }
> > +
> > /// # Safety
> > ///
> > /// Callers must ensure that there are no more concurrent users of the revocable object.
>
On 26.04.25 6:54 PM, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>> Implement an unsafe direct accessor for the data stored within the
>>> Revocable.
>>>
>>> This is useful for cases where we can proof that the data stored within
>>> the Revocable is not and cannot be revoked for the duration of the
>>> lifetime of the returned reference.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>> The explicit lifetimes in access() probably don't serve a practical
>>> purpose, but I found them to be useful for documentation purposes.
>>> ---> rust/kernel/revocable.rs | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>> index 971d0dc38d83..33535de141ce 100644
>>> --- a/rust/kernel/revocable.rs
>>> +++ b/rust/kernel/revocable.rs
>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>> self.try_access().map(|t| f(&*t))
>>> }
>>>
>>> + /// Directly access the revocable wrapped object.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>> + /// for the duration of `'a`.
>>> + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>> I'm not sure if the `'s` lifetime really carries much meaning here.
>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>> but I'm not sure what others (particularly those not very familiar with rust)
>> think of this.
>
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
>
> pub unsafe fn access(&self) -> &T { ... }
>
> , because if you do:
>
> let revocable: &'1 Revocable = ...;
> ...
> let t: &'2 T = unsafe { revocable.access() };
>
> '1 should already outlive '2 (i.e. '1: '2).
I understand that implicit lifetimes desugars to
effectively the same code, I just think that keeping
a explicit 'a makes it a bit more obvious that the
lifetimes need to be considered here.
But I'm also fine with just implicit lifetimes here.
Cheers
Christian
On Sat Apr 26, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
> On 26.04.25 6:54 PM, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> ---> rust/kernel/revocable.rs | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>> self.try_access().map(|t| f(&*t))
>>>> }
>>>>
>>>> + /// Directly access the revocable wrapped object.
>>>> + ///
>>>> + /// # Safety
>>>> + ///
>>>> + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> + /// for the duration of `'a`.
>>>> + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>>
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>>
>> pub unsafe fn access(&self) -> &T { ... }
>>
>> , because if you do:
>>
>> let revocable: &'1 Revocable = ...;
>> ...
>> let t: &'2 T = unsafe { revocable.access() };
>>
>> '1 should already outlive '2 (i.e. '1: '2).
>
> I understand that implicit lifetimes desugars to
> effectively the same code, I just think that keeping
> a explicit 'a makes it a bit more obvious that the
> lifetimes need to be considered here.
>
> But I'm also fine with just implicit lifetimes here.
We elide lifetimes all over the place especially for methods taking
`&self` and returning `&T`. I don't think that it serves a purpose here.
---
Cheers,
Benno
On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > Implement an unsafe direct accessor for the data stored within the
> > > Revocable.
> > >
> > > This is useful for cases where we can proof that the data stored within
> > > the Revocable is not and cannot be revoked for the duration of the
> > > lifetime of the returned reference.
> > >
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > > The explicit lifetimes in access() probably don't serve a practical
> > > purpose, but I found them to be useful for documentation purposes.
> > > ---> rust/kernel/revocable.rs | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > index 971d0dc38d83..33535de141ce 100644
> > > --- a/rust/kernel/revocable.rs
> > > +++ b/rust/kernel/revocable.rs
> > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > > self.try_access().map(|t| f(&*t))
> > > }
> > >
> > > + /// Directly access the revocable wrapped object.
> > > + ///
> > > + /// # Safety
> > > + ///
> > > + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > + /// for the duration of `'a`.
> > > + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > I'm not sure if the `'s` lifetime really carries much meaning here.
> > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > but I'm not sure what others (particularly those not very familiar with rust)
> > think of this.
>
> Yeah, I don't think we need two lifetimes here, the following version
> should be fine (with implicit lifetime):
>
> pub unsafe fn access(&self) -> &T { ... }
>
> , because if you do:
>
> let revocable: &'1 Revocable = ...;
> ...
> let t: &'2 T = unsafe { revocable.access() };
>
> '1 should already outlive '2 (i.e. '1: '2).
Yes, this is indeed sufficient, that's why I wrote
"The explicit lifetimes in access() probably don't serve a practical
purpose, but I found them to be useful for documentation purposes."
below the commit message. :)
Any opinions in terms of documentation purposes?
> >
> > Either way:
> >
> > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> >
> > > + // SAFETY: By the safety requirement of this function it is guaranteed that
> > > + // `self.data.get()` is a valid pointer to an instance of `T`.
> > > + unsafe { &*self.data.get() }
> > > + }
> > > +
> > > /// # Safety
> > > ///
> > > /// Callers must ensure that there are no more concurrent users of the revocable object.
> >
On Sat, Apr 26, 2025 at 07:01:52PM +0200, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
> > On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
> > > On 26.04.25 3:30 PM, Danilo Krummrich wrote:
> > > > Implement an unsafe direct accessor for the data stored within the
> > > > Revocable.
> > > >
> > > > This is useful for cases where we can proof that the data stored within
> > > > the Revocable is not and cannot be revoked for the duration of the
> > > > lifetime of the returned reference.
> > > >
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > ---
> > > > The explicit lifetimes in access() probably don't serve a practical
> > > > purpose, but I found them to be useful for documentation purposes.
> > > > ---> rust/kernel/revocable.rs | 12 ++++++++++++
> > > > 1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > > > index 971d0dc38d83..33535de141ce 100644
> > > > --- a/rust/kernel/revocable.rs
> > > > +++ b/rust/kernel/revocable.rs
> > > > @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
> > > > self.try_access().map(|t| f(&*t))
> > > > }
> > > >
> > > > + /// Directly access the revocable wrapped object.
> > > > + ///
> > > > + /// # Safety
> > > > + ///
> > > > + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
> > > > + /// for the duration of `'a`.
> > > > + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
> > > I'm not sure if the `'s` lifetime really carries much meaning here.
> > > I find just (explicit) `'a` on both parameter and return value is clearer to me,
> > > but I'm not sure what others (particularly those not very familiar with rust)
> > > think of this.
> >
> > Yeah, I don't think we need two lifetimes here, the following version
> > should be fine (with implicit lifetime):
> >
> > pub unsafe fn access(&self) -> &T { ... }
> >
> > , because if you do:
> >
> > let revocable: &'1 Revocable = ...;
> > ...
> > let t: &'2 T = unsafe { revocable.access() };
> >
> > '1 should already outlive '2 (i.e. '1: '2).
>
> Yes, this is indeed sufficient, that's why I wrote
>
> "The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes."
>
> below the commit message. :)
>
Sorry, I overlooked.
> Any opinions in terms of documentation purposes?
>
I think for access() the explicit lifetimes is unnecessary, because it's
a one-lifetime case, the two explicit lifetimes would make a simple case
looking complicated.
For access_with(), that's needed and a good idea.
Just my two cents.
Regards,
Boqun
> > >
> > > Either way:
> > >
> > > Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> > >
> > > > + // SAFETY: By the safety requirement of this function it is guaranteed that
> > > > + // `self.data.get()` is a valid pointer to an instance of `T`.
> > > > + unsafe { &*self.data.get() }
> > > > + }
> > > > +
> > > > /// # Safety
> > > > ///
> > > > /// Callers must ensure that there are no more concurrent users of the revocable object.
> > >
On 26.04.25 7:01 PM, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 09:54:58AM -0700, Boqun Feng wrote:
>> On Sat, Apr 26, 2025 at 06:44:03PM +0200, Christian Schrefl wrote:
>>> On 26.04.25 3:30 PM, Danilo Krummrich wrote:
>>>> Implement an unsafe direct accessor for the data stored within the
>>>> Revocable.
>>>>
>>>> This is useful for cases where we can proof that the data stored within
>>>> the Revocable is not and cannot be revoked for the duration of the
>>>> lifetime of the returned reference.
>>>>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>> The explicit lifetimes in access() probably don't serve a practical
>>>> purpose, but I found them to be useful for documentation purposes.
>>>> ---> rust/kernel/revocable.rs | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>>>> index 971d0dc38d83..33535de141ce 100644
>>>> --- a/rust/kernel/revocable.rs
>>>> +++ b/rust/kernel/revocable.rs
>>>> @@ -139,6 +139,18 @@ pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
>>>> self.try_access().map(|t| f(&*t))
>>>> }
>>>>
>>>> + /// Directly access the revocable wrapped object.
>>>> + ///
>>>> + /// # Safety
>>>> + ///
>>>> + /// The caller must ensure this [`Revocable`] instance hasn't been revoked and won't be revoked
>>>> + /// for the duration of `'a`.
>>>> + pub unsafe fn access<'a, 's: 'a>(&'s self) -> &'a T {
>>> I'm not sure if the `'s` lifetime really carries much meaning here.
>>> I find just (explicit) `'a` on both parameter and return value is clearer to me,
>>> but I'm not sure what others (particularly those not very familiar with rust)
>>> think of this.
>>
>> Yeah, I don't think we need two lifetimes here, the following version
>> should be fine (with implicit lifetime):
>>
>> pub unsafe fn access(&self) -> &T { ... }
>>
>> , because if you do:
>>
>> let revocable: &'1 Revocable = ...;
>> ...
>> let t: &'2 T = unsafe { revocable.access() };
>>
>> '1 should already outlive '2 (i.e. '1: '2).
>
> Yes, this is indeed sufficient, that's why I wrote
>
> "The explicit lifetimes in access() probably don't serve a practical
> purpose, but I found them to be useful for documentation purposes."
>
> below the commit message. :)
>
> Any opinions in terms of documentation purposes?
I would prefer just one explicit lifetime, but I'm
not sure about others.
But I think either way is fine.
Maybe I should have written it more clearly that I
only meant that the second lifetime makes it
IMO unnecessarily complicated when reading it.
Cheers
Christian
© 2016 - 2026 Red Hat, Inc.