Re: [PATCH 02/14] rust: add basic QObject bindings

[Paolo Bonzini <pbonzini@redhat.com>]

On 12/4/25 15:18, Paolo Bonzini wrote:
> On 12/4/25 15:15, Zhao Liu wrote:
>>> diff --git a/rust/util/src/qobject/mod.rs b/rust/util/src/qobject/mod.rs
>>> new file mode 100644
>>> index 00000000000..f43d87a3b66
>>> --- /dev/null
>>> +++ b/rust/util/src/qobject/mod.rs
>>> @@ -0,0 +1,309 @@
>>> +//! `QObject` bindings
>>> +//!
>>> +//! This module implements bindings for QEMU's `QObject` data 
>>> structure.
>>> +//! The bindings integrate with `serde`, which take the role of 
>>> visitors
>>> +//! in Rust code.
>>> +
>>> +#![deny(clippy::unwrap_used)]
>>
>> Are there are any specific considerations for this lint rule?
> 
> It ensures that all conversion errors (e.g. from try_into()) are 
> propagated.
> 
>>> +pub struct QObject(&'static UnsafeCell<bindings::QObject>);
>>
>> It seems Opaque<> feels more natural than UnsafeCell<>.
>>
>> Opaque::from_raw() requires *mut T, but QObject::from_raw() and
>> QObject::clone_from_raw() mainly play with C bindings which usually use
>> *mut pointer. So it seems unnecessary to convert *mut to *const in the
>> middle.
>>
>> And furthermore, I think QObject(Opaque<bindings::QObject>) is better
>> than QObject(&'static Opaque<bindings::QObject>). From a semantic view,
>> C's QObject is a struct, while Rust's QObject is a reference, which seems
>> somewhat mismatched.
>>
>> I'm not sure yet if there may be gaps when remove &'static, but it
>> looks like using &'static Opaque<> instead of &'static UnsafeCell<> is
>> Okay in code?
> 
> I am using UnsafeCell because the QObject here is always valid, i.e. 
> MaybeUninit is explicitly not necessary.  Opaque explicitly allows it to 
> be invalid, here instead the API is "create via C code or FFI and only 
> then create the QObject".
> 
> However, while it is possible to use Opaque<> instead of UnsafeCell<>, 
> it is not possible to make this a simple wrapper because QObject is 
> unmovable and reference counted.  That is, QObject is the equivalent of 
> (for example) Owned<DeviceState>.
> 
>> These 2 methods are the clone, but it seems they're actually similar to
>> Owned<>, i.e., increase refcnt when Rust side wants to "own" or ensure
>> to use this safely.
>>
>> However, there indeed isn't a non-object version of Owned for now, so I
>> think this kind of clone should be okay. Hmm, for long-term, is it
>> valuable to consider a more generic Owned<>?
> 
> Linux has ARef<> I think.  But it seems overkill for now.
> 
>>> +impl<A> FromIterator<A> for QObject
>>> +where
>>> +    Self: From<A>,
>>> +{
>>> +    fn from_iter<I: IntoIterator<Item = A>>(it: I) -> Self {
>>
>> nit: maybe the name "iter" is better than "it"?
> 
> Sure.
> 
>>> +                    match qnum__.kind {
>>> +                        $($crate::bindings::QNUM_I64 => break {
>>> +                            let $i64var = unsafe { qnum__.u.i64_ };
>>> +                            $i64
>>> +                        },)?
>>> +                        $($crate::bindings::QNUM_U64 => break {
>>> +                            let $u64var = unsafe { qnum__.u.u64_ };
>>> +                            $u64
>>> +                        },)?
>>> +                        $($crate::bindings::QNUM_DOUBLE => break {
>>> +                            let $f64var = unsafe { qnum__.u.dbl };
>>> +                            $f64
>>> +                        },)?
>>> +                        _ => {}
>>
>> I think this doesn't handle a special case: qobj's QNUM type doesn't
>> macth the value type, for example, qobj is QNUM_U64 but value is i64.
>>
>> Though external code won't use this macro, I think panic may be better
>> than {}?> Thurther, there're 2 match blocks. Could we merge them as one?

Just one thing: it's not returning {}, it's returning the value 
expressed in $(_ => $other:expr,)?.  So I think it's better to keep it 
this way.

Paolo