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

[Markus Armbruster <armbru@redhat.com>]

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(