Re: [PATCH v11 1/4] rust: types: Add Ownable/Owned types

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

Hi,

I finally found time to seriously work on completing this.

There a few questions that turned up for me, though.

On 250702 1303, Benno Lossin wrote:
> 
> We shouldn't call this a reference. Also we should start the first
> paragraph with how this trait enables the usage of `Owned<Self>`.

Did you come up with any  suggesting what to call it? `Owned<T>` holds a
pointer to `T`. C++ would call it a smart pointer, but I guess that's also
not a good name in Rust.

> 
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> > +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> > +///   kernel is case it frees the object, etc.).
> 
> This invariant sounds weird to me. It's vague "a state which the kernel
> expects" and difficult to use (what needs this invariant?).

The whole matter of what exactly are the safety conditions here is a bit
confusing, I find:

- That the passed `T` is (and stays) valid is a requirement on
  `Owned::from_raw`.
- That `Ownable::release()` is called with a live and unused `T` is a
  requirement for that function call.

I understand things like this then, that implementing `Ownable` is unsafe
because `Owned<T>::drop()` calls the unsafe `T::release()`.

So the requirement is basically:

- it is safe to call `T::release()` _once_ on a `T` stored on an `Owned<T>`
  if the `Owned<T>` isn't used anymore afterwards.

Not sure how to phrase that in a non-confusing way.

I went with this now:

"Implementers must ensure that the [`release()`](Self::release) function
frees the underlying object in the correct way for a valid, owned object
of this type."

> Maybe we should give `Ownable` the task to document the exact ownership
> semantics of `T`?

> > +pub struct Owned<T: Ownable> {
> > +    ptr: NonNull<T>,
> > +    _p: PhantomData<T>,
> > +}
> > +
> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> 
> How does this amount to sending a `&mut T`?

Right, good point. I have to guess, but likely the reasoning was, that
Owned<T> is a wrapper around `*T` and has exclusive access, so somehow
equivalent to `&mut T`.

> I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
> list grows...
> 
> I'll try to come up with something to simplify this design a bit wrt the
> safety docs.

I added "`ptr` points to a valid instance of `T`" to the safety
requirements of `Owned::from_raw`. I think this should imply such things,
because a valid instance of `T` clearly has to be Send/Sync, if it is
implemented for the type `T`, no?

> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> > +
> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> 
> Same here.

Isn't it okay here? All you can do with an `&Owned<T>` is to obtain a `&T`
from it.

Best regards,

Oliver