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

[Zhao Liu <zhao1.liu@intel.com>]

> > -/// # 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. 

Rust pl011 is just faithful to the C functionality, and it's really time
to make the rust version drop the previous technical debt too! 

But I think this patch is OK for now since using legacy interface is the
most simplified approach, and next step we can consider how to convert
it to the modern interface with proper rust binding (my initial thought
tells me that there is at least some effort needed here to explore
whether a simple resettable trait would be enough...).

> 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.

For C side, I also didn't find some case to check parent's
legacy_reset before device_class_set_legacy_reset().

Maybe we should add `assert(!dc->legacy_reset)` in 
device_class_set_legacy_reset()?

Similarly, device_class_set_parent_[realize|unrealize] are worth
adding assert().

If you agree, I'd be happy to add assert() to these three functions as
well. :-)

> 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());
> > +            }
> >           }
> >       }
> >   }
>