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

On 2/24/26 11:29, Markus Armbruster wrote:
> 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.

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.

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.

Paolo