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

2025-06-05 Thread Zhao Liu
On Thu, Jun 05, 2025 at 12:15:35PM +0200, Paolo Bonzini wrote:
> Date: Thu, 5 Jun 2025 12:15:35 +0200
> From: Paolo Bonzini 
> Subject: [PATCH 06/14] rust: qemu-api: add bindings to Error
> X-Mailer: git-send-email 2.49.0
> 
> Provide an implementation of std::error::Error that bridges the Rust
> anyhow::Error and std::panic::Location types with QEMU's Error*.
> 
> It also has several utility methods, analogous to error_propagate(),
> that convert a Result into a return value + Error** pair.  One important
> difference is that these propagation methods *panic* if *errp is NULL,

nit: *panic* if *errp is not NULL

> unlike error_propagate() which eats subsequent errors[1].  The reason
> for this is that in C you have an error_set*() call at the site where
> the error is created, and calls to error_propagate() are relatively rare.
> 
> In Rust instead, even though these functions do "propagate" a
> qemu_api::Error into a C Error**, there is no error_setg() anywhere that
> could check for non-NULL errp and call abort().  error_propagate()'s
> behavior of ignoring subsequent errors is generally considered weird,
> and there would be a bigger risk of triggering it from Rust code.
> 
> [1] This is actually a violation of the preconditions of error_propagate(),
> so it should not happen.  But you never know...
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  docs/devel/rust.rst|   5 +
>  rust/Cargo.lock|  17 ++
>  rust/Cargo.toml|   1 +
>  rust/qemu-api/Cargo.toml   |   2 +
>  rust/qemu-api/meson.build  |   1 +
>  rust/qemu-api/src/error.rs | 312 +
>  rust/qemu-api/src/lib.rs   |   3 +
>  7 files changed, 341 insertions(+)
>  create mode 100644 rust/qemu-api/src/error.rs

Reviewed-by: Zhao Liu 




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

2025-06-05 Thread Markus Armbruster
Paolo Bonzini  writes:

> Provide an implementation of std::error::Error that bridges the Rust
> anyhow::Error and std::panic::Location types with QEMU's Error*.
>
> It also has several utility methods, analogous to error_propagate(),
> that convert a Result into a return value + Error** pair.  One important
> difference is that these propagation methods *panic* if *errp is NULL,
> unlike error_propagate() which eats subsequent errors[1].  The reason
> for this is that in C you have an error_set*() call at the site where
> the error is created, and calls to error_propagate() are relatively rare.
>
> In Rust instead, even though these functions do "propagate" a
> qemu_api::Error into a C Error**, there is no error_setg() anywhere that
> could check for non-NULL errp and call abort().  error_propagate()'s
> behavior of ignoring subsequent errors is generally considered weird,
> and there would be a bigger risk of triggering it from Rust code.
>
> [1] This is actually a violation of the preconditions of error_propagate(),
> so it should not happen.  But you never know...
>
> Signed-off-by: Paolo Bonzini 

[...]

> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 000..80157f6ea1b
> --- /dev/null
> +++ b/rust/qemu-api/src/error.rs

[...]

> +/// Equivalent of the C function `error_propagate`.  Fill `*errp`
> +/// with the information container in `self` if `errp` is not NULL;
> +/// then consume it.
> +///
> +/// This is similar to the C API `error_propagate`, but it panics if
> +/// `*errp` is not `NULL`.
> +///
> +/// # Safety
> +///
> +/// `errp` must be a valid argument to `error_propagate`; it can be
> +/// `NULL` or it can point to any of:
> +/// * `error_abort`
> +/// * `error_fatal`
> +/// * a local variable of (C) type `Error *`

This local variable must contain NULL.

> +///
> +/// Typically `errp` is received from C code and need not be
> +/// checked further at the Rust↔C boundary.
> +pub unsafe fn propagate(self, errp: *mut *mut bindings::Error) {
> +if errp.is_null() {
> +return;
> +}
> +
> +// SAFETY: caller guarantees that errp and *errp are valid
> +unsafe {
> +assert_eq!(*errp, ptr::null_mut());
> +bindings::error_propagate(errp, self.clone_to_foreign_ptr());
> +}
> +}

[...]

With the comment tightened:
Reviewed-by: Markus Armbruster 

The commit message and comment improvements are lovely!




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

2025-06-04 Thread Markus Armbruster
Paolo Bonzini  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.




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

2025-06-04 Thread Paolo Bonzini

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.


The reason to use error_propagate() is that these functions do look a 
lot more like error_propagate() than error_setg().  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().

Paolo




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

2025-06-03 Thread Markus Armbruster
Paolo Bonzini  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 :)




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

2025-06-03 Thread Paolo Bonzini

On 6/2/25 15:18, Markus Armbruster wrote:

Paolo Bonzini  writes:


Provide an implementation of std::error::Error that bridges the Rust
anyhow::Error and std::panic::Location types with QEMU's Error*.
It also has several utility methods, analogous to error_propagate(),
that convert a Result into a return value + Error** pair.

Signed-off-by: Paolo Bonzini 


[...]


diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
new file mode 100644
index 000..0bdd413a0a2
--- /dev/null
+++ b/rust/qemu-api/src/error.rs
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Error propagation for QEMU Rust code
+//!
+//! In QEMU, an `Error` usually consists of a message and an errno value.


Uh, it actually consists of a message and an ErrorClass value.  
You completely ignore ErrorClass in your Rust interface.  I approve.

There are convenience functions that accept an errno, but they don't
store the errno in the Error struct, they append ": " and
strerror(errno) to the message.  Same for Windows GetLastError() values.


Good point - bad wording choice on my part.

I was referring exactly to the construction: whereas C constructs an 
Error (for convenience) from a message and an errno, Rust replaces the 
errno with an anyhow::Error.  The function however is the same, namely 
to include the description of the error when it comes from code that 
doesn't speak Error*.



+//! In this wrapper, the errno value is replaced by an [`anyhow::Error`]


I'm not sure the anyhow::Error replaces anything.  It's simply the
bridge to idiomatic Rust errors.


And errno is the bridge to "idiomatic" C errors. :)  But I agree that it 
should not be mentioned in the first sentence.



+//! Note that the backtrace that is provided by `anyhow` is not used yet,
+//! only the message ends up in the QEMU `Error*`.
+//!
+//! The message part can be used to clarify the inner error, similar to
+//! `error_prepend`, and of course to describe an erroneous condition that


Clarify you're talking about C error_prepend() here?


Yes.  But I'll rephrase to eliminate this reference.


+impl std::error::Error for Error {
+fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+self.cause.as_ref().map(AsRef::as_ref)
+}
+
+#[allow(deprecated)]
+fn description(&self) -> &str {
+self.msg
+.as_deref()
+.or_else(|| 
self.cause.as_deref().map(std::error::Error::description))
+.unwrap_or("unknown error")


Can "unknown error" still happen now you dropped the Default trait?


No, but I prefer to limit undocumented unwrap() to the bare minimum. 
I'll use .expect() which also panics on the unexpected case, but 
includes an error.



+}
+}
+
+impl Display for Error {
+fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+let mut prefix = "";
+if let Some(ref msg) = self.msg {
+write!(f, "{msg}")?;
+prefix = ": ";
+}
+if let Some(ref cause) = self.cause {
+write!(f, "{prefix}{cause}")?;
+} else if prefix.is_empty() {
+f.write_str("unknown error")?;


Can we still get here now you dropped the Default trait?


Same as above.


Uh, is it?  Let's see...


+/// with the information container in `self` if `errp` is not NULL;
+/// then consume it.
+///
+/// # Safety
+///
+/// `errp` must be a valid argument to `error_propagate`;


Reminder for later: the valid @errp arguments for C error_propagate()
are

* NULL

* &error_abort

* &error_fatal

* Address of some Error * variable containing NULL

* Address of some Error * variable containing non-NULL


I will add this note.


What does errp.write(err) do?  I *guess* it simply stores @err in @errp.
Matches neither behavior.

If that's true, then passing &error_abort or &error_fatal to Rust does
not work, and neither does error accumulation.  Not equivalent of C
error_propagate().

Is "propagate" semantics what you want here?

If not, use another name.


As replied elsewhere, yes.

Paolo




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

2025-06-03 Thread Paolo Bonzini

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.


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


Paolo




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

2025-06-03 Thread Markus Armbruster
Zhao Liu  writes:
> Markus Armbruster  writes:

[...]

>> Let's examine the other aspect: how exactly "storing" behaves.
>> 
>> error_setg() according to its contract:
>> 
>> If @errp is NULL, the error is ignored.  [...]
>> 
>> If @errp is &error_abort, print a suitable message and abort().
>> 
>> If @errp is &error_fatal, print a suitable message and exit(1).
>> 
>> If @errp is anything else, *@errp must be NULL.
>> 
>> error_propagate() according to its contract:
>> 
>> [...] if @dst_errp is NULL, errors are being ignored.  Free the
>> error object.
>> 
>> Else, if @dst_errp is &error_abort, print a suitable message and
>> abort().
>> 
>> Else, if @dst_errp is &error_fatal, print a suitable message and
>> exit(1).
>> 
>> Else, if @dst_errp already contains an error, ignore this one: free
>> the error object.
>> 
>> Else, move the error object from @local_err to *@dst_errp.
>> 
>> The second to last clause is where its storing differs from
>> error_setg().
>> 
>> What does errp.write(err) do?  I *guess* it simply stores @err in @errp.
>> Matches neither behavior.
>> 
>> If that's true, then passing &error_abort or &error_fatal to Rust does
>> not work, and neither does error accumulation.  Not equivalent of C
>> error_propagate().
>
> I did some simple tests. yes, &error_abort or &error_fatal doesn't work.
> Current @errp of realize() can work because @errp points to @local_err
> in device_set_realized().

Thank you!

>> Is "propagate" semantics what you want here?
>> 
>> If not, use another name.
>
> I guess here we should call C version's error_propagate() instead of
> write():
>
> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> index a91ce6fefaf4..56622065ad22 100644
> --- a/rust/qemu-api/src/error.rs
> +++ b/rust/qemu-api/src/error.rs
> @@ -205,7 +205,7 @@ pub unsafe fn propagate(self, errp: *mut *mut 
> bindings::Error) {
>
>  // SAFETY: caller guarantees errp is valid
>  unsafe {
> -errp.write(err);
> +bindings::error_propagate(errp, err);
>  }
>  }
>
> ---
>
> 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.

If we don't: use error_handle()?




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

2025-06-03 Thread Zhao Liu
> > +/// Equivalent of the C function `error_propagate`.  Fill `*errp`
> 
> Uh, is it?  Let's see...
> 
> > +/// with the information container in `self` if `errp` is not NULL;
> > +/// then consume it.
> > +///
> > +/// # Safety
> > +///
> > +/// `errp` must be a valid argument to `error_propagate`;
> 
> Reminder for later: the valid @errp arguments for C error_propagate()
> are
> 
> * NULL
> 
> * &error_abort
> 
> * &error_fatal
> 
> * Address of some Error * variable containing NULL
> 
> * Address of some Error * variable containing non-NULL
> 
> The last one is *not* valid with error_setg().
> 
> > +/// typically it is received from C code and need not be
> > +/// checked further at the Rust↔C boundary.
> > +pub unsafe fn propagate(self, errp: *mut *mut bindings::Error) {
> 
> Reminder, just to avoid confusion: C error_propagate() has the arguments
> in the opposite order.
> 
> > +if errp.is_null() {
> > +return;
> > +}
> > +
> > +let err = self.clone_to_foreign_ptr();
> > +
> > +// SAFETY: caller guarantees errp is valid
> > +unsafe {
> > +errp.write(err);
> > +}
> > +}
> 
> In C, we have two subtly different ways to store into some Error **errp
> argument: error_setg() and error_propagate().
> 
> Their obvious difference is that error_setg() creates the Error object
> to store, while error_propagate() stores an existing Error object if
> any, else does nothing.
> 
> Their unobvious difference is behavior when the destination already
> contains an Error.  With error_setg(), this must not happen.
> error_propagate() instead throws away the new error.  This permits
> "first one wins" error accumulation.  Design mistake if you ask me.
> 
> Your Rust propagate() also stores an existing bindings::Error.  Note
> that "else does nothing" doesn't apply, because we always have an
> existing error object here, namely @self.  In the error_propagate() camp
> so far.
> 
> Let's examine the other aspect: how exactly "storing" behaves.
> 
> error_setg() according to its contract:
> 
> If @errp is NULL, the error is ignored.  [...]
> 
> If @errp is &error_abort, print a suitable message and abort().
> 
> If @errp is &error_fatal, print a suitable message and exit(1).
> 
> If @errp is anything else, *@errp must be NULL.
> 
> error_propagate() according to its contract:
> 
> [...] if @dst_errp is NULL, errors are being ignored.  Free the
> error object.
> 
> Else, if @dst_errp is &error_abort, print a suitable message and
> abort().
> 
> Else, if @dst_errp is &error_fatal, print a suitable message and
> exit(1).
> 
> Else, if @dst_errp already contains an error, ignore this one: free
> the error object.
> 
> Else, move the error object from @local_err to *@dst_errp.
> 
> The second to last clause is where its storing differs from
> error_setg().
> 
> What does errp.write(err) do?  I *guess* it simply stores @err in @errp.
> Matches neither behavior.
> 
> If that's true, then passing &error_abort or &error_fatal to Rust does
> not work, and neither does error accumulation.  Not equivalent of C
> error_propagate().

I did some simple tests. yes, &error_abort or &error_fatal doesn't work.
Current @errp of realize() can work because @errp points to @local_err
in device_set_realized().

> Is "propagate" semantics what you want here?
> 
> If not, use another name.

I guess here we should call C version's error_propagate() instead of
write():

diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
index a91ce6fefaf4..56622065ad22 100644
--- a/rust/qemu-api/src/error.rs
+++ b/rust/qemu-api/src/error.rs
@@ -205,7 +205,7 @@ pub unsafe fn propagate(self, errp: *mut *mut 
bindings::Error) {

 // SAFETY: caller guarantees errp is valid
 unsafe {
-errp.write(err);
+bindings::error_propagate(errp, err);
 }
 }

---

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.)

Regards,
Zhao





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

2025-06-02 Thread Markus Armbruster
Paolo Bonzini  writes:

> Provide an implementation of std::error::Error that bridges the Rust
> anyhow::Error and std::panic::Location types with QEMU's Error*.
> It also has several utility methods, analogous to error_propagate(),
> that convert a Result into a return value + Error** pair.
>
> Signed-off-by: Paolo Bonzini 

[...]

> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 000..0bdd413a0a2
> --- /dev/null
> +++ b/rust/qemu-api/src/error.rs
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +//! Error propagation for QEMU Rust code
> +//!
> +//! In QEMU, an `Error` usually consists of a message and an errno value.

Uh, it actually consists of a message and an ErrorClass value.  However,
use of anything but ERROR_CLASS_GENERIC_ERROR is strongly discouraged.
Historical reasons...

You completely ignore ErrorClass in your Rust interface.  I approve.

There are convenience functions that accept an errno, but they don't
store the errno in the Error struct, they append ": " and
strerror(errno) to the message.  Same for Windows GetLastError() values.

> +//! In this wrapper, the errno value is replaced by an [`anyhow::Error`]

I'm not sure the anyhow::Error replaces anything.  It's simply the
bridge to idiomatic Rust errors.

> +//! so that it is easy to pass any other Rust error type up to C code.

This is true.

> +//! Note that the backtrace that is provided by `anyhow` is not used yet,
> +//! only the message ends up in the QEMU `Error*`.
> +//!
> +//! The message part can be used to clarify the inner error, similar to
> +//! `error_prepend`, and of course to describe an erroneous condition that

Clarify you're talking about C error_prepend() here?

> +//! does not come from another [`Error`](std::error::Error) (for example an
> +//! invalid argument).
> +//!
> +//! On top of implementing [`std::error::Error`], [`Error`] provides 
> functions

Suggest to wrap comments a bit earlier.

> +//! to simplify conversion between [`Result<>`](std::result::Result) and
> +//! C `Error**` conventions.  In particular:
> +//!
> +//! * [`ok_or_propagate`](qemu_api::Error::ok_or_propagate),
> +//!   [`bool_or_propagate`](qemu_api::Error::bool_or_propagate),
> +//!   [`ptr_or_propagate`](qemu_api::Error::ptr_or_propagate) can be used to
> +//!   build a C return value while also propagating an error condition
> +//!
> +//! * [`err_or_else`](qemu_api::Error::err_or_else) and
> +//!   [`err_or_unit`](qemu_api::Error::err_or_unit) can be used to build a
> +//!   `Result`
> +//!
> +//! While these facilities are useful at the boundary between C and Rust 
> code,
> +//! other Rust code need not care about the existence of this module; it can
> +//! just use the [`qemu_api::Result`] type alias and rely on the `?` 
> operator as
> +//! usual.
> +//!
> +//! @author Paolo Bonzini
> +
> +use std::{
> +borrow::Cow,
> +ffi::{c_char, c_int, c_void, CStr},
> +fmt::{self, Display},
> +panic, ptr,
> +};
> +
> +use foreign::{prelude::*, OwnedPointer};
> +
> +use crate::bindings;
> +
> +pub type Result = std::result::Result;
> +
> +#[derive(Debug)]
> +pub struct Error {
> +msg: Option>,
> +/// Appends the print string of the error to the msg if not None
> +cause: Option,
> +file: &'static str,
> +line: u32,
> +}
> +
> +impl std::error::Error for Error {
> +fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
> +self.cause.as_ref().map(AsRef::as_ref)
> +}
> +
> +#[allow(deprecated)]
> +fn description(&self) -> &str {
> +self.msg
> +.as_deref()
> +.or_else(|| 
> self.cause.as_deref().map(std::error::Error::description))
> +.unwrap_or("unknown error")

Can "unknown error" still happen now you dropped the Default trait?

> +}
> +}
> +
> +impl Display for Error {
> +fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> +let mut prefix = "";
> +if let Some(ref msg) = self.msg {
> +write!(f, "{msg}")?;
> +prefix = ": ";
> +}
> +if let Some(ref cause) = self.cause {
> +write!(f, "{prefix}{cause}")?;
> +} else if prefix.is_empty() {
> +f.write_str("unknown error")?;

Can we still get here now you dropped the Default trait?

> +}
> +Ok(())
> +}
> +}
> +
> +impl From for Error {
> +#[track_caller]
> +fn from(msg: String) -> Self {
> +let location = panic::Location::caller();
> +Error {
> +msg: Some(Cow::Owned(msg)),
> +cause: None,
> +file: location.file(),
> +line: location.line(),
> +}
> +}
> +}
> +
> +impl From<&'static str> for Error {
> +#[track_caller]
> +fn from(msg: &'static str) -> Self {
> +let location = panic::Location::caller();
> +Error {
> +msg: Some(Cow::Borrowed(msg)),
> +cause: None,
> +