Paolo Bonzini <pbonz...@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.
