Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`

[Oliver Mangold <oliver.mangold@pm.me>]

On 251128 1506, Daniel Almeida wrote:

> > /// Type allocated and destroyed on the C side, but owned by Rust.
> > ///
> > -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> > -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> > -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> > -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> > +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
> > +///  - This is useful when it is desirable to tie the lifetime of an object reference to an owned
> > +///    object, rather than pass around a bare reference.
> > +///  - [`Ownable`] types can define custom drop logic that is executed when the owned reference
> > +///    of type [`Owned<_>`] pointing to the object is dropped.
> > ///
> > /// Note: The underlying object is not required to provide internal reference counting, because it
> > /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> > -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> > +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
> > +/// between unique and shared (reference counted) ownership is needed.
> > ///
> > /// # Safety
> > ///
> > @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
> >     ///   mutable reference requirements. That is, the kernel will not mutate or free the underlying
> >     ///   object and is okay with it being modified by Rust code.
> >     pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > -        Self {
> > -            ptr,
> > -        }
> > +        Self { ptr }
> >     }
> 
> Unrelated change?

Ah, yes, rustfmt must I done that, and I missed it. Will fix.

> > +///
> > +/// impl OwnableRefCounted for Foo {
> > +///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> > +///         if this.refcount.get() == 1 {
> > +///             // SAFETY: The `Foo` is still alive and has no other Rust references as the refcount
> > +///             // is 1.
> > +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> > +///         } else {
> > +///             Err(this)
> > +///         }
> > +///     }
> > +/// }
> > +///
> 
> We wouldn’t need this implementation if we added a “refcount()”
> member to this trait. This lets you abstract away this logic for all
> implementors, which has the massive upside of making sure we hardcode (and thus
> enforce) the refcount == 1 check.

This wouldn't work for the block `Request` use case. There a reference can
be acquired "out of thin air" using a `TagSet`. Thus "check for unique
refcount" + "create an owned reference" needs to be one atomic operation.

Also I think it might be generally problematic to require a refcount()
function. The API of the underlying kernel object we want to wrap might not
offer that, so we would need to access internal data.


> > +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
> > +/// unsafe impl Ownable for Foo {
> > +///     unsafe fn release(this: NonNull<Self>) {
> > +///         // SAFETY: Using `dec_ref()` from [`RefCounted`] to release is okay, as the refcount is
> > +///         // always 1 for an [`Owned<Foo>`].
> > +///         unsafe{ Foo::dec_ref(this) };
> > +///     }
> > +/// }
> > +///
> > +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
> 
> All these “expects()” and custom error strings would go away if you
> place this behind a fictional function that returns Result.

Not sure what you mean by fictional function. Do you mean a non-existent
function? We want to compile this code as a unit test.

The rest of your suggested changes make sense, I guess. I will implement
them.

Thanks,

Oliver