Re: [PULL 25/41] rust: qom: put class_init together from multiple ClassInitImpl<>

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 19/12/24 09:32, Paolo Bonzini wrote:
> Parameterize the implementation of ClassInitImpl so that it is
> possible to call up the chain of implementations, one superclass at
> a time starting at ClassInitImpl<Self::Class>.
> 
> In order to avoid having to implement (for example)
> ClassInitImpl<PL011Class>, also remove the dummy PL011Class and
> PL011LuminaryClass structs and specify the same ObjectType::Class as
> the superclass.  In the future this default behavior can be handled by
> a procedural macro, by looking at the first field in the struct.
> 
> Note that the new trait is safe: the calls are started by
> rust_class_init<>(), which is not public and can convert the class
> pointer to a Rust reference.
> 
> Since CLASS_BASE_INIT applies to the type that is being defined,
> and only to it, move it to ObjectImpl.
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs  |  19 +----
>   rust/qemu-api/src/definitions.rs  | 111 ++++++++++++++++++++++++------
>   rust/qemu-api/src/device_class.rs |  50 +++++---------
>   rust/qemu-api/src/sysbus.rs       |  18 ++++-
>   rust/qemu-api/tests/tests.rs      |   9 +--
>   5 files changed, 127 insertions(+), 80 deletions(-)


> diff --git a/rust/qemu-api/src/device_class.rs b/rust/qemu-api/src/device_class.rs
> index c98f0b2c7da..dcec5488291 100644
> --- a/rust/qemu-api/src/device_class.rs
> +++ b/rust/qemu-api/src/device_class.rs


> -/// # Safety
> -///
> -/// We expect the FFI user of this function to pass a valid pointer that
> -/// can be downcasted to type `DeviceClass`, because `T` implements
> -/// `DeviceImpl`.
> -pub unsafe extern "C" fn rust_device_class_init<T: DeviceImpl>(
> -    klass: *mut ObjectClass,
> -    _: *mut c_void,
> -) {
> -    let mut dc = ::core::ptr::NonNull::new(klass.cast::<DeviceClass>()).unwrap();
> -    unsafe {
> -        let dc = dc.as_mut();
> +impl<T> ClassInitImpl<DeviceClass> for T
> +where
> +    T: DeviceImpl,
> +{
> +    fn class_init(dc: &mut DeviceClass) {
>           if <T as DeviceImpl>::REALIZE.is_some() {
>               dc.realize = Some(rust_realize_fn::<T>);
>           }
>           if <T as DeviceImpl>::RESET.is_some() {
> -            bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));
> +            unsafe {
> +                bindings::device_class_set_legacy_reset(dc, Some(rust_reset_fn::<T>));

Pre-existing, but since it appears on this patch, Rust device models
should not implement this legacy interface. If a non-Rust parent
implements it, I think we should convert the non-Rust parent before
adding a Rust child. No clue how to check a parent don't implement
this interface in Rust.

Generally, we shouldn't access legacy API from Rust IMHO.

> +            }
>           }
>           if let Some(vmsd) = <T as DeviceImpl>::vmsd() {
>               dc.vmsd = vmsd;
>           }
>           let prop = <T as DeviceImpl>::properties();
>           if !prop.is_empty() {
> -            bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> +            unsafe {
> +                bindings::device_class_set_props_n(dc, prop.as_ptr(), prop.len());
> +            }
>           }
>       }
>   }