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

[Zhao Liu <zhao1.liu@intel.com>]

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

> +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>);

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?

> +    /// Construct a [`QObject`] from a C `QObject` pointer.
> +    /// 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()
> +    }

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

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

> +        let qlist = unsafe { &mut *bindings::qlist_new() };
> +        for elem in it {
> +            let elem: QObject = elem.into();
> +            let elem: *mut bindings::QObject = elem.0.get();
> +            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 {

ditto.

> +        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)) }
> +    }
> +}

I think these the mappings of qdict and qlist look quite good.

> +#[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
> +                        },)?
> +                        _ => {}

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?

diff --git a/rust/util/src/qobject/mod.rs b/rust/util/src/qobject/mod.rs
index 292a3c9c2385..4053a3b7610d 100644
--- a/rust/util/src/qobject/mod.rs
+++ b/rust/util/src/qobject/mod.rs
@@ -280,14 +280,6 @@ macro_rules! match_qobject {
                 $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 {
@@ -302,7 +294,9 @@ macro_rules! match_qobject {
                             let $f64var = unsafe { qnum__.u.dbl };
                             $f64
                         },)?
-                        _ => {}
+                        _ => {
+                            panic!("unreachable");
+                        }
                     }
                 },
                 $($crate::bindings::QTYPE_QSTRING => break {


Regards,
Zhao