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

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

+Markus, Daniel & Peter

On 3/1/25 10:02, Zhao Liu wrote:
>>> -/// # 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());
>>> +            }
>>>            }
>>>        }
>>>    }
>>