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

[Paolo Bonzini <pbonzini@redhat.com>]

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?

Yes, that works because all callers specify all three of i64/u64/f64.

Paolo