On Wed, May 28, 2025 at 11:49 AM Markus Armbruster <arm...@redhat.com> wrote:
> > diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> > new file mode 100644
> > index 00000000000..f08fed81028
> > --- /dev/null
> > +++ b/rust/qemu-api/src/error.rs
> > @@ -0,0 +1,273 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +//! Error class for QEMU Rust code
> > +//!
> > +//! @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,
> > + bindings::{error_free, error_get_pretty},
> > +};
> > +
> > +pub type Result<T> = std::result::Result<T, Error>;
> > +
> > +#[derive(Debug, Default)]
> > +pub struct Error {
>
> We're defining a custom error type Error for use with Result<>. This
> requires implementing a number of traits. For trait Debug, we take the
> auto-generated solution here. Other traits are implemented below, in
> particular Display.
>
> I don't yet understand the role of trait Default.
It defines an Error without any frills attached. It is used below but
on the other hand it results in those "unknown error"s that you
rightly despised.
> Does the name Error risk confusion with std::error::Error?
Maybe, but as you can see from e.g. ayhow::Error it's fairly common to
have each crate or module define its own Error type. In the end you
always convert them to another type with "?" or ".into()".
> This is the Rust equivalent to C struct Error. High-level differences:
>
> * No @err_class. It's almost always ERROR_CLASS_GENERIC_ERROR in C
> nowadays. You're hardcoding that value in Rust for now. Good.
>
> * @cause, optional. This is the bridge to idiomatic Rust error types.
>
> * @msg is optional. This is so you can wrap a @cause without having to
> add some useless message.
>
> Is having Errors with neither @msg nor @cause a good idea?
It makes for slightly nicer code, and avoids having to worry about
panics from ".unwrap()" in error handling code (where panicking
probably won't help much). Otherwise it's probably not a good idea,
but also not something that people will use since (see later patches)
it's easier to return a decent error message than an empty Error.
> Needed for what? Oh, method description() is deprecated since 1.42.0:
> "use the Display impl or to_string()". I figure we need it because the
> new way doesn't work with our oldest supported Rust version. Could
> perhaps use a comment to help future garbage collectors.
>
> > + fn description(&self) -> &str {
> > + self.msg
> > + .as_deref()
> > + .or_else(||
> > self.cause.as_deref().map(std::error::Error::description))
> > + .unwrap_or("error")
>
> This gives us @msg, or else @cause, or else "error".
>
> Is it a good idea to ignore @cause when we have @msg?
> > +impl Display for Error {
> > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > ...
> > + }
>
> This combines @msg and @cause:
>
> Differs from description(). Why?
Because description() cannot build a dynamically-allocated string, it
must return something that is already available in the Error. That
limitation is probably why it was deprecated.
Since it's deprecated we can expect that it won't be used and not
worry too much.
> > + fn from(msg: String) -> Self {
> > + let location = panic::Location::caller();
> > + Error {
> > + msg: Some(Cow::Owned(msg)),
> > + file: location.file(),
> > + line: location.line(),
> > + ..Default::default()
>
> I don't understand this line, I'm afraid.
It says "every other field comes from "..Default::default()". I can
replace it with "cause: None", and likewise below.
> > +}
> > +
> > +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)),
> > + file: location.file(),
> > + line: location.line(),
> > + ..Default::default()
> > + }
> > + }
> > +}
>
> Same for another string type.
Yes, this is for strings that are not allocated and are always
valid---such as string constants.
> Is there a way to create an Error with neither @msg nor @cause?
Yes, Default::default()
> > + errp: *mut *mut bindings::Error,
> > + ) -> Option<T> {
> > + let Err(err) = result else {
> > + return result.ok();
> > + };
> > +
> > + // SAFETY: caller guarantees errp is valid
> > + unsafe {
> > + err.propagate(errp);
> > + }
> > + None
>
> @result is an Error. Propagate it, and return None.
>
> This is indeed like self.ok() with propagation added.
Alternatively:
result
.map_err(|err| unsafe { err.propagate(errp) })
.ok()
Shorter, but the functional style can be off putting. What do you prefer?
> > + }
> > +
> > + /// Equivalent of the C function `error_propagate`. Fill `*errp`
> > + /// with the information container in `result` if `errp` is not NULL;
> > + /// then consume it.
>
> Note error_propagate() has the arguments in the opposite order.
Yeah, here "self" must be first so that you use it as a method.
Though, s/result/self/ as you noted.
> > + /// # Safety
> > + ///
> > + /// `errp` must be valid; typically it is received from C code
>
> What does "valid" mean exactly?
I will copy from `error_propagate` in v2.
> Brief switch to the design level...
>
> In C, you're almost always better off with ERRP_GUARD(). Would you like
> me to elaborate?
>
> You still have to use error_propagate() to accumulate multiple errors so
> that the first one wins. That's commonly a dumb idea. Should we avoid
> this pattern in Rust?
In Rust there are three kinds of functions that use errors. Two are in qemu_api:
(1) bridges from C to Rust function pointers. They receive a Result<>
from Rust functions and use error_propagate() (probably via functions
such as ok_or_propagate, bool_or_propagate, etc.) to prepare a C
return value.
(2) bridges from Rust to C functions. They pass an Error** to the C
function and use err_or_else() or err_or_unit() to prepare a Rust
return value
Functions of kind (1) are like functions in C that do a single call
and just pass down errp, for example user_creatable_add_qapi(). These
do not need ERRP_GUARD() because:
* the conversion from Result<> to C is pretty much a necessity, and
it's done with functions that guarantee the propagation
* the conversion function *consumes* the Result<>, guaranteeing that
you do not propagate more than once with tragic results
Function of kind (2) do not need ERRP_GUARD() because they do not take
an Error** at all. They pass one down to C, but they return a
Result<>.
The third kind is Rust functions that are called from (1) and that
themselves call (2). These are where ERRP_GUARD() would be used in C,
but in Rust these do not see Error** at all. The "?" operator has the
same effect as ERRP_GUARD(), i.e. it handles passing the error from
called function to return value. What in C would be
ERRP_GUARD();
if (!func_that_returns_bool(...)) { return; }
In Rust is
func_that_returns_result(...)?;
which is a bit disconcerting in the beginning but grows on you.
(Generally I find that to be the experience with Rust. It's downright
weird, but unlike C++ the good parts outweigh the weirdness).
> > + 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.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `c_error` must be valid; typically it has been filled by 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),
> > + }
> > + }
> > +}
>
> Getting tired...
No problem. While this is kinda important, it's not used yet. The
clients would look like this:
fn type_get_or_load_by_name(name: &str) -> Result<&TypeImpl> {
unsafe {
let err: *mut bindings::Error = ptr::null_mut();
let typ: *mut TypeImpl = bindings::type_get_or_load_by_name(
name.clone_to_foreign().as_ptr(), &mut err);
// on success, "typ" can be accessed safely
// on failure, turn the Error* into a qemu_api::Error and free it
Result::err_or_else(err, || &*typ)
}
This is why I need to write tests.
> > +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 isize,
> > + 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)
> > + }
> > + }
> > +}
>
> Plausible to this Rust ignoramus :)
Good, since these *Foreign traits are not part of the standard
library, but rather something that I concocted and published outside
QEMU. If you had no problems understanding how they were used (e.g.
stuff like into_native or clone_to_foreign_ptr), that is good.
Paolo
Paolo
