Re: [PATCH v2 03/16] rust/qobject: add Serialize implementation

Paolo Bonzini <[email protected]> writes:

> This allows QObject to be converted to other formats, for example
> JSON via serde_json.
>
> This is not too useful, since QObjects are consumed by
> C code or deserialized into structs, but it can be used for testing
> and it is part of the full implementation of a serde format.
>
> Co-authored-by: Marc-André Lureau <[email protected]>
> Signed-off-by: Marc-André Lureau <[email protected]>
> Reviewed-by: Zhao Liu <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>

[...]

> diff --git a/rust/util/src/qobject/serialize.rs b/rust/util/src/qobject/serialize.rs
> new file mode 100644
> index 00000000000..34ec3847c1d
> --- /dev/null
> +++ b/rust/util/src/qobject/serialize.rs
> @@ -0,0 +1,59 @@
> +//! `QObject` serialization
> +//!
> +//! This module implements the [`Serialize`] trait for `QObject`,
> +//! allowing it to be converted to other formats, for example
> +//! JSON.
> +
> +use std::{ffi::CStr, mem::ManuallyDrop, ptr::addr_of};
> +
> +use serde::ser::{self, Serialize, SerializeMap, SerializeSeq};
> +
> +use super::{match_qobject, QObject};
> +use crate::bindings;
> +
> +impl Serialize for QObject {
> +    #[inline]
> +    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
> +    where
> +        S: ::serde::Serializer,
> +    {
> +        match_qobject! { (self) =>
> +            () => serializer.serialize_unit(),
> +            bool(b) => serializer.serialize_bool(b),
> +            i64(i) => serializer.serialize_i64(i),
> +            u64(u) => serializer.serialize_u64(u),
> +            f64(f) => serializer.serialize_f64(f),
> +            CStr(cstr) => cstr.to_str().map_or_else(
> +                |_| Err(ser::Error::custom("invalid UTF-8 in QString")),

.to_str() fails when its argument is invalid UTF-8.  It returns "an
error with details of where UTF-8 validation failed."[1]

Why are we replacing this error with a custom one?  I guess we add the
clue "in QString".  We also lose the details of where.  Feels like a
questionable trade.

Moreover, I believe this is a programming error: QString is not meant to
wrap arbitrary byte sequences, it's meant to wrap UTF-8 strings.
Programming errors should panic.

Can we use .expect()?  It panics "if the value is an Err, with a panic
message including the passed message, and the content of the Err."[2]

If we decide this isn't a programming error (because QString may contain
arbitrary zero-terminated byte sequences[3]): can we combine all the
readily available information like .expect() does?

In review of v2, you wrote

    I'd rather not special case this into the only abort (see the
    "#![deny(clippy::unwrap_used)]" in patch 4).

Is this an argument for pretending this isn't a programming error?  If
yes, I need a bit more help to understand it.

> +                |s| serializer.serialize_str(s),
> +            ),
> +            QList(l) => {
> +                let mut node_ptr = unsafe { l.head.tqh_first };
> +                let mut state = serializer.serialize_seq(None)?;
> +                while !node_ptr.is_null() {
> +                    let node = unsafe { &*node_ptr };
> +                    let elem = unsafe { ManuallyDrop::new(QObject::from_raw(addr_of!(*node.value))) };
> +                    state.serialize_element(&*elem)?;
> +                    node_ptr = unsafe { node.next.tqe_next };
> +                }
> +                state.end()
> +            },
> +            QDict(d) => {
> +                let mut state = serializer.serialize_map(Some(d.size))?;
> +                let mut e_ptr = unsafe { bindings::qdict_first(d) };
> +                while !e_ptr.is_null() {
> +                    let e = unsafe { &*e_ptr };
> +                    let key = unsafe { CStr::from_ptr(e.key) };
> +                    key.to_str().map_or_else(
> +                        |_| Err(ser::Error::custom("invalid UTF-8 in key")),
> +                        |k| state.serialize_key(k),
> +                    )?;
> +                    let value = unsafe { ManuallyDrop::new(QObject::from_raw(addr_of!(*e.value))) };
> +                    state.serialize_value(&*value)?;
> +                    e_ptr = unsafe { bindings::qdict_next(d, e) };
> +                }
> +                state.end()
> +            }
> +        }
> +    }
> +}

[1] https://doc.rust-lang.org/std/ffi/struct.CStr.html#method.to_str
[2] https://doc.rust-lang.org/std/result/enum.Result.html#method.expect
[3] Feels like a problematic idea to me.