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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 6/3/25 12:32, Markus Armbruster wrote:
>>> Then Rust's propagate has the same behavior as C (Of course, here Rust
>>> is actually using C's error_propagate, so the two are equivalent.)
>> 
>> *If* we want propagate semantics.  I'm not sure we do.
>
> Yes, we do.  This function is used at the Rust-to-C boundary and should 
> behave exactly like C functions would: it will get an Error ** from the 
> callers and needs to propagate the just-created Error* into it.

Well, how *do* C functions behave?

 * = Rules =
 *
 * - Functions that use Error to report errors have an Error **errp
 *   parameter.  It should be the last parameter, except for functions
 *   taking variable arguments.
 *
 * - You may pass NULL to not receive the error, &error_abort to abort
 *   on error, &error_fatal to exit(1) on error, or a pointer to a
 *   variable containing NULL to receive the error.

For later...  This is a precondition.  passing a pointer to a variable
containing anything but NULL violates the precondition.

 *
 * - Separation of concerns: the function is responsible for detecting
 *   errors and failing cleanly; handling the error is its caller's
 *   job.  Since the value of @errp is about handling the error, the
 *   function should not examine it.
 *
 * - The function may pass @errp to functions it calls to pass on
 *   their errors to its caller.  If it dereferences @errp to check
 *   for errors, it must use ERRP_GUARD().
 *
 * - On success, the function should not touch *errp.  On failure, it
 *   should set a new error, e.g. with error_setg(errp, ...), or
 *   propagate an existing one, e.g. with error_propagate(errp, ...).

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.

 *
 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

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?

> In fact, I had found this issue already last Friday, but then didn't 
> inform you because of the (long) weekend.  Apologies for that.

No harm, no foul :)