> 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
