Paolo Bonzini <pbonz...@redhat.com> 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 <pbonz...@redhat.com>
[...]
> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 00000000000..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<T> = std::result::Result<T, Error>;
> +
> +#[derive(Debug)]
> +pub struct Error {
> + msg: Option<Cow<'static, str>>,
> + /// Appends the print string of the error to the msg if not None
> + cause: Option<anyhow::Error>,
> + 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<String> 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,
> + file: location.file(),
> + line: location.line(),
> + }
> + }
> +}
> +
> +impl From<anyhow::Error> for Error {
> + #[track_caller]
> + fn from(error: anyhow::Error) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + msg: None,
> + cause: Some(error),
> + file: location.file(),
> + line: location.line(),
> + }
> + }
> +}
> +
> +impl Error {
> + /// Create a new error, prepending `msg` to the
> + /// description of `cause`
> + #[track_caller]
> + pub fn with_error(msg: impl Into<Cow<'static, str>>, cause: impl
> Into<anyhow::Error>) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + msg: Some(msg.into()),
> + cause: Some(cause.into()),
> + file: location.file(),
> + line: location.line(),
> + }
> + }
> +
> + /// Consume a result, returning `false` if it is an error and
> + /// `true` if it is successful. The error is propagated into
> + /// `errp` like the C API `error_propagate` would do.
> + ///
> + /// # Safety
> + ///
> + /// `errp` must be a valid argument to `error_propagate`;
> + /// typically it is received from C code and need not be
> + /// checked further at the Rust↔C boundary.
> + pub unsafe fn bool_or_propagate(result: Result<()>, errp: *mut *mut
> bindings::Error) -> bool {
> + // SAFETY: caller guarantees errp is valid
> + unsafe { Self::ok_or_propagate(result, errp) }.is_some()
> + }
> +
> + /// Consume a result, returning a `NULL` pointer if it is an
> + /// error and a C representation of the contents if it is
> + /// successful. The error is propagated into `errp` like
> + /// the C API `error_propagate` would do.
> + ///
> + /// # Safety
> + ///
> + /// `errp` must be a valid argument to `error_propagate`;
> + /// typically it is received from C code and need not be
> + /// checked further at the Rust↔C boundary.
> + #[must_use]
> + pub unsafe fn ptr_or_propagate<T: CloneToForeign>(
> + result: Result<T>,
> + errp: *mut *mut bindings::Error,
> + ) -> *mut T::Foreign {
> + // SAFETY: caller guarantees errp is valid
> + unsafe { Self::ok_or_propagate(result, errp) }.clone_to_foreign_ptr()
> + }
> +
> + /// Consume a result in the same way as `self.ok()`, but also propagate
> + /// a possible error into `errp`, like the C API `error_propagate`
> + /// would do.
> + ///
> + /// # Safety
> + ///
> + /// `errp` must be a valid argument to `error_propagate`;
> + /// typically it is received from C code and need not be
> + /// checked further at the Rust↔C boundary.
> + pub unsafe fn ok_or_propagate<T>(
> + result: Result<T>,
> + errp: *mut *mut bindings::Error,
> + ) -> Option<T> {
> + result.map_err(|err| unsafe { err.propagate(errp) }).ok()
> + }
> +
> + /// 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().
Is "propagate" semantics what you want here?
If not, use another name.
> +
> + /// Convert a C `Error*` into a Rust `Result`, using
> + /// `Ok(())` if `c_error` is NULL. Free the `Error*`.
> + ///
> + /// # Safety
> + ///
> + /// `c_error` must be `NULL` or valid; typically it was initialized
Double-checking: "valid" means it points to struct Error.
> + /// with `ptr::null_mut()` and passed by reference to a C function.
> + pub unsafe fn err_or_unit(c_error: *mut bindings::Error) -> Result<()> {
> + // SAFETY: caller guarantees c_error is valid
> + unsafe { Self::err_or_else(c_error, || ()) }
> + }
> +
> + /// Convert a C `Error*` into a Rust `Result`, calling `f()` to
> + /// obtain an `Ok` value if `c_error` is NULL. Free the `Error*`.
> + ///
> + /// # Safety
> + ///
> + /// `c_error` must be `NULL` or valid; typically it was initialized
> + /// with `ptr::null_mut()` and passed by reference to a C function.
> + pub unsafe fn err_or_else<T, F: FnOnce() -> T>(
> + c_error: *mut bindings::Error,
> + f: F,
> + ) -> Result<T> {
> + // SAFETY: caller guarantees c_error is valid
> + let err = unsafe { Option::<Self>::from_foreign(c_error) };
> + match err {
> + None => Ok(f()),
> + Some(err) => Err(err),
> + }
> + }
> +}
> +
> +impl FreeForeign for Error {
> + type Foreign = bindings::Error;
> +
> + unsafe fn free_foreign(p: *mut bindings::Error) {
> + // SAFETY: caller guarantees p is valid
> + unsafe {
> + bindings::error_free(p);
> + }
> + }
> +}
> +
> +impl CloneToForeign for Error {
> + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
> + // SAFETY: all arguments are controlled by this function
> + unsafe {
> + let err: *mut c_void =
> libc::malloc(std::mem::size_of::<bindings::Error>());
> + let err: &mut bindings::Error = &mut *err.cast();
> + *err = bindings::Error {
> + msg: format!("{self}").clone_to_foreign_ptr(),
> + err_class: bindings::ERROR_CLASS_GENERIC_ERROR,
> + src_len: self.file.len() as c_int,
> + src: self.file.as_ptr().cast::<c_char>(),
> + line: self.line as c_int,
> + func: ptr::null_mut(),
> + hint: ptr::null_mut(),
> + };
> + OwnedPointer::new(err)
> + }
> + }
> +}
> +
> +impl FromForeign for Error {
> + unsafe fn cloned_from_foreign(c_error: *const bindings::Error) -> Self {
> + // SAFETY: caller guarantees c_error is valid
> + unsafe {
> + let error = &*c_error;
> + let file = if error.src_len < 0 {
> + // NUL-terminated
> + CStr::from_ptr(error.src).to_str()
> + } else {
> + // Can become str::from_utf8 with Rust 1.87.0
> + std::str::from_utf8(std::slice::from_raw_parts(
> + &*error.src.cast::<u8>(),
> + error.src_len as usize,
> + ))
> + };
> +
> + Error {
> + msg: FromForeign::cloned_from_foreign(error.msg),
> + cause: None,
> + file: file.unwrap(),
> + line: error.line as u32,
> + }
> + }
> + }
> +}
> diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
> index 234a94e3c29..93902fc94bc 100644
> --- a/rust/qemu-api/src/lib.rs
> +++ b/rust/qemu-api/src/lib.rs
> @@ -19,6 +19,7 @@
> pub mod cell;
> pub mod chardev;
> pub mod errno;
> +pub mod error;
> pub mod irq;
> pub mod memory;
> pub mod module;
> @@ -34,6 +35,8 @@
> ffi::c_void,
> };
>
> +pub use error::{Error, Result};
> +
> #[cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)]
> extern "C" {
> fn g_aligned_alloc0(
