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 <pbonzini@redhat.com>
---
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 | 299 +++++++++++++++++++++++++++++++++++++
rust/qemu-api/src/lib.rs | 3 +
6 files changed, 323 insertions(+)
create mode 100644 rust/qemu-api/src/error.rs
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 13d580c693b..961005bb513 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -2,6 +2,12 @@
# It is not intended for manual editing.
version = 3
+[[package]]
+name = "anyhow"
+version = "1.0.98"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487"
+
[[package]]
name = "arbitrary-int"
version = "1.2.7"
@@ -37,6 +43,15 @@ version = "1.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
+[[package]]
+name = "foreign"
+version = "0.3.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "17ca1b5be8c9d320daf386f1809c7acc0cb09accbae795c2001953fa50585846"
+dependencies = [
+ "libc",
+]
+
[[package]]
name = "hpet"
version = "0.1.0"
@@ -106,6 +121,8 @@ dependencies = [
name = "qemu_api"
version = "0.1.0"
dependencies = [
+ "anyhow",
+ "foreign",
"libc",
"qemu_api_macros",
]
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index a00b8ad0bcd..26f3ec09d3d 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -66,6 +66,7 @@ missing_safety_doc = "deny"
mut_mut = "deny"
needless_bitwise_bool = "deny"
needless_pass_by_ref_mut = "deny"
+needless_update = "deny"
no_effect_underscore_binding = "deny"
option_option = "deny"
or_fun_call = "deny"
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index c96cf50e7a1..db7000dee44 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -15,7 +15,9 @@ rust-version.workspace = true
[dependencies]
qemu_api_macros = { path = "../qemu-api-macros" }
+anyhow = "~1.0"
libc = "0.2.162"
+foreign = "~0.3.1"
[features]
default = ["debug_cell"]
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index aa22252866d..ef0de51266b 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -19,6 +19,7 @@ _qemu_api_rs = static_library(
'src/cell.rs',
'src/chardev.rs',
'src/errno.rs',
+ 'src/error.rs',
'src/irq.rs',
'src/memory.rs',
'src/module.rs',
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.
+//! In this wrapper, the errno value is replaced by an [`anyhow::Error`]
+//! so that it is easy to pass any other Rust error type up to C code.
+//! 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
+//! does not come from another [`Error`](std::error::Error) (for example an
+//! invalid argument).
+//!
+//! On top of implementing [`std::error::Error`], [`Error`] provides functions
+//! 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")
+ }
+}
+
+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")?;
+ }
+ 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`
+ /// with the information container in `self` if `errp` is not NULL;
+ /// then consume it.
+ ///
+ /// # 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 propagate(self, errp: *mut *mut bindings::Error) {
+ if errp.is_null() {
+ return;
+ }
+
+ let err = self.clone_to_foreign_ptr();
+
+ // SAFETY: caller guarantees errp is valid
+ unsafe {
+ errp.write(err);
+ }
+ }
+
+ /// 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
+ /// 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(
--
2.49.0
Paolo Bonzini <pbonzini@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 <pbonzini@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(
On 6/2/25 15:18, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@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 <pbonzini@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.
> 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
> > + /// 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
Zhao Liu <zhao1.liu@intel.com> writes:
> Markus Armbruster <armbru@redhat.com> 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()?
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
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 :)
© 2016 - 2025 Red Hat, Inc.