Re: [PATCH 06/14] rust: qemu-api: add bindings to Error

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 6/4/25 07:01, Markus Armbruster wrote:
>> This is what your FOO_or_propagate() functions are for.
>> 
>> The rule glosses over a subtle detail: the difference between
>> error_setg() and error_propagate() isn't just create a new error vs. use
>> an existing one, namely error_setg() makes the precondition violation
>> mentioned above a programming error, whereas error_propagate() does not,
>> it instead *ignores* the error it's supposed to propagate.
>> 
>> I consider this difference a design mistake.  Note that GError avoids
>> this mistake: g_error_propagate() requieres the destination to NULL or
>> point to NULL.  We deviated from GError, because we thought we were
>> smarter.  We weren't.
>> 
>> Mostly harmless in practice, as behavior is identical for callers that
>> satisfy the preconditions.
>> 
>> [...]
>> 
>> So here's the bottom line.  We want a Rust function to use C Error
>> according to its written rules.  Due to a design mistake, C functions
>> can behave in two different ways when their caller violates a certain
>> precondition, depending on how the function transmits the error to the
>> caller.  For Rust functions, we can
>> 
>> * Always behave the more common way, i.e. like a C function using
>>   error_setg() to transmit.
>> 
>> * Always behave the less common way, i.e. like a C function using
>>   error_propagate() to transmit.
>> 
>> * Sometimes one way, sometimes the other way.
>> 
>> This is actually in order of decreasing personal preference.  But what
>> do *you* think?
>>
> I agree that there are arguments for both.  The reason to use 
> error_setg() is that, even though these functions "propagate" a 
> qemu_api::Error into a C Error**, the error is born in the Rust callback 
> and therefore there is no error_setg() anywhere that could check for 
> non-NULL abort().  There is a bigger risk of triggering 
> error_propagate()'s weird behavior.

Yes.

> The reason to use error_propagate() is that these functions do look a 
> lot more like error_propagate() than error_setg().

True.

>                                                     I'm undecided.  I 
> think I'll keep the error_setg() semantics, which is essentially
>
>      assert_eq!(unsafe { *errp }, ptr::null_mut());
>
> followed by calling bindings::error_propagate().

Works for me.

The error_propagate() then does nothing but call error_handle().
However, error_handle() is static, and making it available for Rust
just to cut out a harmless middleman seems hardly worth the bother.