Re: [PATCH 02/10] rust: qom: add reference counting functionality

[Paolo Bonzini]

On Mon, Jan 27, 2025 at 8:38 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> > +impl<T: ObjectType> Owned<T> {
> > +    /// Convert a raw C pointer into an owned reference to the QOM
> > +    /// object it points to.  The object's reference count will be
> > +    /// decreased when the `Owned` is dropped.
> > +    ///
> > +    /// # Panics
> > +    ///
> > +    /// Panics if `ptr` is NULL.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The caller must indeed own a reference to the QOM object.
> > +    /// The object must not be embedded in another unless the outer
> > +    /// object is guaranteed to have a longer lifetime.
> > +    ///
> > +    /// A raw pointer obtained via [`Owned::into_raw()`] can always be 
> > passed
> > +    /// back to `from_raw()` (assuming the original `Owned` was valid!),
> > +    /// since the owned reference remains there between the calls to
> > +    /// `into_raw()` and `from_raw()`.
> > +    #[allow(clippy::missing_const_for_fn)]
> > +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> > +        // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > +        // Deref is implemented so the pointer passed to from_raw
> > +        // remains const
> > +        Owned(NonNull::new(ptr as *mut T).unwrap())
> > +    }
>
> ...
>
> > +    /// Increase the reference count of a QOM object and return
> > +    /// a new owned reference to it.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The object must not be embedded in another, unless the outer
> > +    /// object is guaranteed to have a longer lifetime.
> > +    pub unsafe fn from(obj: &T) -> Self {
> > +        unsafe {
> > +            object_ref(obj.as_object_mut_ptr().cast::<c_void>());
> > +
> > +            // SAFETY NOTE: while NonNull requires a mutable pointer, only
> > +            // Deref is implemented so the reference passed to from_raw
> > +            // remains shared
> > +            Owned(NonNull::new_unchecked(obj.as_mut_ptr()))
> > +        }
> > +    }
> > +}
> > +
>
> About the difference between from_raw() and from(), I understand if the
> C side also holds a pointer, the Rust side must increase the reference
> count (using Owned::from), and If the C side does not have any other
> pointers, Rust can directly use Owned::from_raw. Am I right?

Pretty much - more precisely you use Object::from_raw 1) if the C side
gifts a reference 2) if you got the pointer from Owned::into_raw. The
second case is similar to Arc::from_raw, which expects that you got a
reference from Arc::into_raw. The first is the more common case.

>
> * The use of from():
>
>                 let clk = bindings::qdev_init_clock_in(...)
>                 Owned::from(&*clk)

In this case the C side wants to manage the reference that
qdev_init_clock_in() returns; it is dropped in
qdev_finalize_clocklist(). So Rust code needs to increase the
refcount.

> * The use of from_raw():
>
>     fn new() -> Owned<Self> {
>         assert!(bql_locked());
>         // SAFETY: the object created by object_new is allocated on
>         // the heap and has a reference count of 1
>         unsafe {
>             let obj = &*object_new(Self::TYPE_NAME.as_ptr());
>             Owned::from_raw(obj.unsafe_cast::<Self>())
>         }
>     }

In this case the C side lets the caller manage the (only) reference
when object_new returns, so you must not increase the refcount.

Owned::from() is slightly less efficient, though that almost never
matters. If it does you can use ManuallyDrop::new(Owned::from_raw(p)).

> Comparing with these 2 use cases, I find the difference is
> qdev_init_clock_in() creates a pointer in qdev_init_clocklist().

That is related, but more precisely the difference is that
qdev_init_clock_in() wants to unref that pointer later.

> Then the comment "the clock is heap allocated and does not have
> a reference" sounds like a conflict. I'm sure I'm missing something. :-(

Changed:

      // SAFETY: the clock is heap allocated, but qdev_init_clock_in()
      // does not gift the reference to its caller; so use Owned::from to
      // add one.  the callback is disabled automatically when the clock
      // is unparented, which happens before the device is finalized.


Thanks for the review!

Paolo