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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/24/26 11:29, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> 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 <marcandre.lureau@redhat.com>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> [...]
>> 
>>> 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.
>
> Changing the error is required in order to return an S::Error (that is, 
> the serializer asks for a type of error that it understands).  I can 
> also preserve the message with "|e| Err(ser::Error::custom(e))" or 
> something like that.

If it's not too much trouble...

> While it is undoubtedly a programming error, I didn't like that the 
> error (and thus the panic) happens in a different place than where the 
> QString is constructed.  I like panicking for *my* programming errors, 
> but not for someone else's :) especially if I can recover easily.

I ruthlessly panic when I find invariants violated, preconditions like
this one very much included.

If you'd rather not panic here, please make it locally obvious that the
error is not supposed to happen.  Suggest to put it into the error
message, so increase the likelihood of the programming error actually
getting fixed if it ever triggers.