Re: [PATCH 01/12] rust: qom: add ParentField

[Zhao Liu]

On Fri, Dec 20, 2024 at 03:29:43PM +0100, Paolo Bonzini wrote:
> Date: Fri, 20 Dec 2024 15:29:43 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 01/12] rust: qom: add ParentField
> X-Mailer: git-send-email 2.47.1
> 
> Add a type that, together with the C function object_deinit, ensures the
> correct drop order for QOM objects relative to their superclasses.
> 
> Right now it is not possible to implement the Drop trait for QOM classes
> that are defined in Rust, as the drop() function would not be called when
> the object goes away; instead what is called is ObjectImpl::INSTANCE_FINALIZE.
> It would be nice for INSTANCE_FINALIZE to just drop the object, but this has
> a problem: suppose you have
> 
>    pub struct MySuperclass {
>        parent: DeviceState,
>        field: Box<MyData>,
>        ...
>    }
> 
>    impl Drop for MySuperclass {
>        ...
>    }
> 
>    pub struct MySubclass {
>        parent: MySuperclass,
>        ...
>    }
> 
> and an instance_finalize implementation that is like
> 
>     unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
>         unsafe { std::ptr::drop_in_place(obj.cast::<T>()) }
>     }
> 
> When instance_finalize is called for MySubclass, it will walk the struct's
> list of fields and call the drop method for MySuperclass.  Then, object_deinit
> recurses to the superclass and calls the same drop method again.  This
> will cause double-freeing of the Box<Data>.
> 
> What's happening here is that QOM wants to control the drop order of
> MySuperclass and MySubclass's fields.  To do so, the parent field must
> be marked ManuallyDrop<>, which is quite ugly.  Instead, add a wrapper
> type ParentField<> that is specific to QOM.  This hides the implementation
> detail of *what* is special about the ParentField, and will also be easy
> to check in the #[derive(Object)] macro.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs |  6 ++--
>  rust/qemu-api/src/qom.rs         | 56 +++++++++++++++++++++++++++++---
>  rust/qemu-api/tests/tests.rs     |  4 +--
>  3 files changed, 57 insertions(+), 9 deletions(-)

...

>  unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
>      // SAFETY: obj is an instance of T, since rust_instance_init<T>
>      // is called from QOM core as the instance_init function
> @@ -151,8 +198,9 @@ fn as_ref(&self) -> &$parent {
>  ///
>  /// - the struct must be `#[repr(C)]`;
>  ///
> -/// - the first field of the struct must be of the instance struct 
> corresponding
> -///   to the superclass, which is `ObjectImpl::ParentType`
> +/// - the first field of the struct must be of type
> +///   [`ParentField<T>`](ParentField), where `T` is the parent type
> +///   [`ObjectImpl::ParentType`]
>  ///
>  /// - likewise, the first field of the `Class` must be of the class struct
>  ///   corresponding to the superclass, which is 
> `ObjectImpl::ParentType::Class`.

I think the "likewise" word should be deleted as well, since Class'
parent field doesn't need any wrapper because Class also doesn't have
finalize method. The remaining description is clear enough.

Others look good to me!

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>