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

[Markus Armbruster <armbru@redhat.com>]

Learning opportunity for me, I ask for your patience...

Paolo Bonzini <pbonzini@redhat.com> writes:

> This is only a basic API, intended to be used by the serde traits.
>
> Co-authored-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/util/wrapper.h          |   7 +
>  rust/util/meson.build        |   6 +-
>  rust/util/src/lib.rs         |   2 +
>  rust/util/src/qobject/mod.rs | 317 +++++++++++++++++++++++++++++++++++
>  4 files changed, 330 insertions(+), 2 deletions(-)
>  create mode 100644 rust/util/src/qobject/mod.rs
>
> diff --git a/rust/util/wrapper.h b/rust/util/wrapper.h
> index b9ed68a01d8..0907dd59142 100644
> --- a/rust/util/wrapper.h
> +++ b/rust/util/wrapper.h
> @@ -30,3 +30,10 @@ typedef enum memory_order {
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qemu/timer.h"
> +#include "qobject/qnull.h"
> +#include "qobject/qbool.h"
> +#include "qobject/qnum.h"
> +#include "qobject/qstring.h"
> +#include "qobject/qobject.h"
> +#include "qobject/qlist.h"
> +#include "qobject/qdict.h"
> diff --git a/rust/util/meson.build b/rust/util/meson.build
> index 8ad344dccbd..ce468ea5227 100644
> --- a/rust/util/meson.build
> +++ b/rust/util/meson.build
> @@ -36,8 +36,10 @@ _util_rs = static_library(
>        'src/module.rs',
>        'src/timer.rs',
>      ],
> -    {'.': _util_bindings_inc_rs}
> -  ),
> +    {'.': _util_bindings_inc_rs,
> +    'qobject': [
> +      'src/qobject/mod.rs',
> +    ]}),
>    override_options: ['rust_std=2021', 'build.rust_std=2021'],
>    rust_abi: 'rust',
>    dependencies: [anyhow_rs, libc_rs, foreign_rs, glib_sys_rs, common_rs, qom, qemuutil],
> diff --git a/rust/util/src/lib.rs b/rust/util/src/lib.rs
> index 16c89b95174..fe0128103c8 100644
> --- a/rust/util/src/lib.rs
> +++ b/rust/util/src/lib.rs
> @@ -4,6 +4,8 @@
>  pub mod error;
>  pub mod log;
>  pub mod module;
> +#[macro_use]
> +pub mod qobject;
>  pub mod timer;
>  
>  pub use error::{Error, Result};
> diff --git a/rust/util/src/qobject/mod.rs b/rust/util/src/qobject/mod.rs
> new file mode 100644
> index 00000000000..9c6f168d6e1
> --- /dev/null
> +++ b/rust/util/src/qobject/mod.rs
> @@ -0,0 +1,317 @@
> +//! `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)]
> +
> +use std::{
> +    cell::UnsafeCell,
> +    ffi::{c_char, CString},
> +    mem::ManuallyDrop,
> +    ptr::{addr_of, addr_of_mut},
> +    sync::atomic::{AtomicUsize, Ordering},
> +};
> +
> +use common::assert_field_type;
> +
> +use crate::bindings;
> +
> +/// 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?

> +
> +// 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?

> +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?

> +    /// The caller cedes its reference to the returned struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The `QObjectBase` must not be changed from C code while
> +    /// the Rust `QObject` lives
> +    const unsafe fn from_base(p: *const bindings::QObjectBase_) -> Self {
> +        QObject(unsafe { &*p.cast() })
> +    }
> +
> +    /// Construct a [`QObject`] from a C `QObject` pointer.
> +    /// The caller cedes its reference to the returned struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The `QObject` must not be changed from C code while
> +    /// the Rust `QObject` lives
> +    pub const unsafe fn from_raw(p: *const bindings::QObject) -> Self {
> +        QObject(unsafe { &*p.cast() })
> +    }
> +
> +    /// 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"?

> +    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?

> +    /// The caller *does not* cede its reference to the returned struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The `QObjectBase` must not be changed from C code while
> +    /// the Rust `QObject` lives
> +    unsafe fn cloned_from_base(p: *const bindings::QObjectBase_) -> Self {
> +        let orig = unsafe { ManuallyDrop::new(QObject::from_base(p)) };
> +        (*orig).clone()
> +    }
> +
> +    /// Construct a [`QObject`] from a C `QObject` pointer.
> +    /// The caller *does not* cede its reference to the returned struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The `QObject` must not be changed from C code while
> +    /// the Rust `QObject` lives
> +    pub unsafe fn cloned_from_raw(p: *const bindings::QObject) -> Self {
> +        let orig = unsafe { ManuallyDrop::new(QObject::from_raw(p)) };
> +        (*orig).clone()
> +    }
> +
> +    fn refcnt(&self) -> &AtomicUsize {
> +        assert_field_type!(bindings::QObjectBase_, refcnt, usize);
> +        let qobj = self.0.get();
> +        unsafe { AtomicUsize::from_ptr(addr_of_mut!((*qobj).base.refcnt)) }
> +    }
> +}
> +
> +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()?

> +    }
> +}
> +
> +impl<T> From<Option<T>> for QObject
> +where
> +    QObject: From<T>,
> +{
> +    fn from(o: Option<T>) -> Self {
> +        o.map_or_else(|| ().into(), Into::into)
> +    }
> +}
> +
> +impl From<bool> for QObject {
> +    fn from(b: bool) -> Self {
> +        let qobj = unsafe { &*bindings::qbool_from_bool(b) };
> +        unsafe { QObject::from_base(addr_of!(qobj.base)) }
> +    }
> +}
> +
> +macro_rules! from_int {
> +    ($t:ty) => {
> +        impl From<$t> for QObject {
> +            fn from(n: $t) -> Self {
> +                let qobj = unsafe { &*bindings::qnum_from_int(n.into()) };
> +                unsafe { QObject::from_base(addr_of!(qobj.base)) }
> +            }
> +        }
> +    };
> +}
> +
> +from_int!(i8);
> +from_int!(i16);
> +from_int!(i32);
> +from_int!(i64);
> +
> +macro_rules! from_uint {
> +    ($t:ty) => {
> +        impl From<$t> for QObject {
> +            fn from(n: $t) -> Self {
> +                let qobj = unsafe { &*bindings::qnum_from_uint(n.into()) };
> +                unsafe { QObject::from_base(addr_of!(qobj.base)) }
> +            }
> +        }
> +    };
> +}
> +
> +from_uint!(u8);
> +from_uint!(u16);
> +from_uint!(u32);
> +from_uint!(u64);
> +
> +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?

> +from_double!(f64);

Can you briefly explain why we need more than i64, u64, and double?

> +
> +impl From<CString> for QObject {
> +    fn from(s: CString) -> Self {
> +        let qobj = unsafe { &*bindings::qstring_from_str(s.as_ptr()) };
> +        unsafe { QObject::from_base(addr_of!(qobj.base)) }
> +    }
> +}
> +
> +impl<A> FromIterator<A> for QObject
> +where
> +    Self: From<A>,
> +{
> +    fn from_iter<I: IntoIterator<Item = A>>(it: I) -> Self {
> +        let qlist = unsafe { &mut *bindings::qlist_new() };
> +        for elem in it {
> +            let elem: QObject = elem.into();
> +            let elem = elem.into_raw();
> +            unsafe {
> +                bindings::qlist_append_obj(qlist, elem);
> +            }
> +        }
> +        unsafe { QObject::from_base(addr_of!(qlist.base)) }
> +    }
> +}
> +
> +impl<A> FromIterator<(CString, A)> for QObject
> +where
> +    Self: From<A>,
> +{
> +    fn from_iter<I: IntoIterator<Item = (CString, A)>>(it: I) -> Self {
> +        let qdict = unsafe { &mut *bindings::qdict_new() };
> +        for (key, val) in it {
> +            let val: QObject = val.into();
> +            let val = val.into_raw();
> +            unsafe {
> +                bindings::qdict_put_obj(qdict, key.as_ptr().cast::<c_char>(), val);
> +            }
> +        }
> +        unsafe { QObject::from_base(addr_of!(qdict.base)) }
> +    }
> +}
> +
> +impl Clone for QObject {
> +    fn clone(&self) -> Self {
> +        self.refcnt().fetch_add(1, Ordering::Acquire);
> +        QObject(self.0)
> +    }
> +}
> +
> +impl Drop for QObject {
> +    fn drop(&mut self) {
> +        if self.refcnt().fetch_sub(1, Ordering::Release) == 1 {
> +            unsafe {
> +                bindings::qobject_destroy(self.0.get());
> +            }
> +        }
> +    }
> +}
> +

Skipping the remainder, it's too much macro magic for poor, ignorant me
:)

> +#[allow(unused)]
> +macro_rules! match_qobject {
> +    (@internal ($qobj:expr) =>
> +        $(() => $unit:expr,)?
> +        $(bool($boolvar:tt) => $bool:expr,)?
> +        $(i64($i64var:tt) => $i64:expr,)?
> +        $(u64($u64var:tt) => $u64:expr,)?
> +        $(f64($f64var:tt) => $f64:expr,)?
> +        $(CStr($cstrvar:tt) => $cstr:expr,)?
> +        $(QList($qlistvar:tt) => $qlist:expr,)?
> +        $(QDict($qdictvar:tt) => $qdict:expr,)?
> +        $(_ => $other:expr,)?
> +    ) => {
> +        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
> +                },)?
> +                $crate::bindings::QTYPE_QNUM => {
> +                    let qnum__: *mut $crate::bindings::QNum = qobj_.cast();
> +                    let qnum__ = unsafe { &*qnum__ };
> +                    match qnum__.kind {
> +                        $crate::bindings::QNUM_I64 |
> +                        $crate::bindings::QNUM_U64 |
> +                        $crate::bindings::QNUM_DOUBLE => {}
> +                        _ => {
> +                            panic!("unreachable");
> +                        }
> +                    }
> +
> +                    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
> +                        },)?
> +                        _ => {}
> +                    }
> +                },
> +                $($crate::bindings::QTYPE_QSTRING => break {
> +                    let qstring__: *mut $crate::bindings::QString = qobj_.cast();
> +                    let $cstrvar = unsafe { ::core::ffi::CStr::from_ptr((&*qstring__).string) };
> +                    $cstr
> +                },)?
> +                $($crate::bindings::QTYPE_QLIST => break {
> +                    let qlist__: *mut $crate::bindings::QList = qobj_.cast();
> +                    let $qlistvar = unsafe { &*qlist__ };
> +                    $qlist
> +                },)?
> +                $($crate::bindings::QTYPE_QDICT => break {
> +                    let qdict__: *mut $crate::bindings::QDict = qobj_.cast();
> +                    let $qdictvar = unsafe { &*qdict__ };
> +                    $qdict
> +                },)?
> +                _ => ()
> +            };
> +            $(break $other;)?
> +            #[allow(unreachable_code)]
> +            {
> +                panic!("unreachable");
> +            }
> +        }
> +    };
> +
> +    // first cleanup the syntax a bit, checking that there's at least
> +    // one pattern and always adding a trailing comma
> +    (($qobj:expr) =>
> +        $($type:tt$(($val:tt))? => $code:expr ),+ $(,)?) => {
> +            match_qobject!(@internal ($qobj) =>
> +                $($type $(($val))? => $code,)+)
> +    };
> +}
> +#[allow(unused_imports)]
> +use match_qobject;