Re: [PATCH 04/19] rust/qobject: add basic bindings

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/5/25 10:35, Markus Armbruster wrote:
>>> +/// A wrapper for a C `QObject`.
>>> +///
>>> +/// Because `QObject` is not thread-safe, the safety of these bindings
>>> +/// right now hinges on treating them as immutable.  It is part of the
>>> +/// contract with the `QObject` constructors that the Rust struct is
>>> +/// only built after the contents are stable.
>>> +///
>>> +/// Only a bare bones API is public; production and consumption of `QObject`
>>> +/// generally goes through `serde`.
>>> +pub struct QObject(&'static UnsafeCell<bindings::QObject>);
>> 
>> This defines the Rust QObject.  All it contains is a reference (wrapped
>> in UnsafeCell) self.0 to the C QObject.  Correct?
>
> Correct.
>
>>> +
>>> +// SAFETY: the QObject API are not thread-safe other than reference counting;
>>> +// but the Rust struct is only created once the contents are stable, and
>>> +// therefore it obeys the aliased XOR mutable invariant.
>> 
>> In other words, we promise never to change a QObject while Rust code
>> holds a reference, except for the reference counts.  Correct?
>> 
>> The reference count is the mutable part of an otherwise immutable
>> object.  Not mentioned here: it is atomic.  Therefore, concurrent
>> updates cannot mess it up.  Nothing depends on its value except
>> deallocation when the last reference drops.  I figure that's why the
>> exception to "aliased XOR mutable" is fine.  Correct?
>
> Yes, it's one of a few exceptions to "aliased XOR mutable" including:
>
> - Mutex (because only one guy can access it at all anyway)
>
> - RefCell (enforces aliased XOR mutable at run-time, enforces 
> single-thread usage at compile-time)
>
> - atomics (a mini mutex)
>
> - Cell (Mutex:RefCell = atomics:Cell, in other words every access is 
> independent but also single-thread usage is checked at compile time)
>
>>> +unsafe impl Send for QObject {}
>>> +unsafe impl Sync for QObject {}
>>> +
>>> +// Since a QObject can be a floating-point value, and potentially a NaN,
>>> +// do not implement Eq
>>> +impl PartialEq for QObject {
>>> +    fn eq(&self, other: &Self) -> bool {
>>> +        unsafe { bindings::qobject_is_equal(self.0.get(), other.0.get()) }
>>> +    }
>>> +}
>>> +
>>> +impl QObject {
>>> +    /// Construct a [`QObject`] from a C `QObjectBase` pointer.
>> 
>> It's spelled QObjectBase_.  More of the same below, not flagging again.
>> 
>> Comment next to its definition:
>> 
>>      /* Not for use outside include/qobject/ */
>> 
>> We're using it outside now.  Do we really need to?
>
> It's because we're defining equivalents of inline functions in 
> include/qobject.

Fair, but we should update comments when we make them wrong :)

> I can however replace uses of from_base with a macro similar to QOBJECT()

Might be cleaner.

>>> +    /// Obtain a raw C pointer from a reference. `self` is consumed
>>> +    /// and the C `QObject` pointer is leaked.
>> 
>> What exactly do you mean by "leaked"?
>
> s/and the.*/without decreasing the reference count, thus transferring 
> the reference to the `*mut bindings::QOjbect`/

Much clearer.

>>> +    pub fn into_raw(self) -> *mut bindings::QObject {
>>> +        let src = ManuallyDrop::new(self);
>>> +        src.0.get()
>>> +    }
>>> +
>>> +    /// Construct a [`QObject`] from a C `QObject` pointer.
>> 
>> Pasto?  Isn't it QObjectBase_ here?
>
> Yes.
>
>>> +impl From<()> for QObject {
>>> +    fn from(_null: ()) -> Self {
>>> +        unsafe { QObject::cloned_from_base(addr_of!(bindings::qnull_.base)) }
>> 
>> qnull_ is not meant for use outside qnull.[ch] and its unit test
>> check-qnull.c.  Could we use qnull()?
>
> Same as above---it's inline.  The above is a translation of
>
> static inline QNull *qnull(void)
> {
>      return qobject_ref(&qnull_);
> }

Could we call C qnull() instead?

>>> +macro_rules! from_double {
>>> +    ($t:ty) => {
>>> +        impl From<$t> for QObject {
>>> +            fn from(n: $t) -> Self {
>>> +                let qobj = unsafe { &*bindings::qnum_from_double(n.into()) };
>>> +                unsafe { QObject::from_base(addr_of!(qobj.base)) }
>>> +            }
>>> +        }
>>> +    };
>>> +}
>>> +
>>> +from_double!(f32);
>> 
>> Uh, isn't the double in from_double misleading?
>
> It's a reference to the function that it calls (qnum_from_double).  Can 
> rename it to impl_from_returning_qnum_double.
>
>>> +from_double!(f64);
>> 
>> Can you briefly explain why we need more than i64, u64, and double?
>
> Because Rust doesn't do automatic casts.  So it's nicer (and also less 
> error prone) if the subsequent patches do not have to always convert to 
> u64 or i64.

Okay.

>> Skipping the remainder, it's too much macro magic for poor, ignorant me
>> :)
>
> It's not really hard.  The thing to the left of => effectively defines a 
> parser. Each thing of the shape $IDENT:RULE matches a piece of Rust 
> grammar; expr is expression an tt is token tree (either a single token 
> or a parenthesized group).  To access $IDENT that appears within $(...)? 
> on the left of => you must have a similar $(...)? on the right, and the 
> whole $(...)? on the right will be skipped if the left-side wasn't there.
>
> The macro is used like this:
>
>          match_qobject! { (self) =>
>              () => Unexpected::Unit,
>              bool(b) => Unexpected::Bool(b),
>              i64(n) => Unexpected::Signed(n),
>              u64(n) => Unexpected::Unsigned(n),
>              f64(n) => Unexpected::Float(n),
>              CStr(s) => s.to_str().map_or_else(
>                  |_| Unexpected::Other("string with invalid UTF-8"),
>                  Unexpected::Str),
>              QList(_) => Unexpected::Seq,
>              QDict(_) => Unexpected::Map,
>          }
>
> And it produces a "switch" on QObject types, where each "case" extracts 
> the datum, places it in the variable to the left of "=>" (such as "b" 
> for bool), and returns the value on the right of "=>" (such as 
> "Unexpected::Bool(b)"):
>
>
>>> +    ) => {
>>> +        loop {
>>> +            let qobj_ = $qobj.0.get();
>>> +            match unsafe { &* qobj_ }.base.type_ {
>>> +                $($crate::bindings::QTYPE_QNULL => break $unit,)?
>>> +                $($crate::bindings::QTYPE_QBOOL => break {
>>> +                    let qbool__: *mut $crate::bindings::QBool = qobj_.cast();
>>> +                    let $boolvar = unsafe { (&*qbool__).value };
>>> +                    $bool
>>> +                },)?
>
> (The loop/break is just a syntactic convenience---the loop never rolls 
> more than once).
>
> Paolo

Thanks for your help!