Paolo Bonzini <pbonz...@redhat.com> writes:
> 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.
I think I understand how this works now.
>> 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()".
Fair enough.
>> 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.
I accept that making "need at least one of @msg, @cause" an invariant
could complicate the code a bit.
Having a way to create an Error with neither bothers me, even when it's
obscure. Making an interface hard to misuse is good. Making it
impossible to misuse is better.
>> 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.
Could we somehow get away with not implementing it?
>> > + 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.
I've since learned a bit more about Default. Since the only field being
defaulted is @cause, I guess I'd use "cause: None" myself for
simplicity. Matter of taste, you choose.
However, if this is the only real reason for Error having the Default
trait, I'd suggest to kill the trait with fire, because ...
>> > +}
>> > +
>> > +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()
... I do dislike being able to create such Errors :)
>> > + 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?
I'm fine with the functional style, and I like brevity.
>> > + }
>> > +
>> > + /// 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.
We received an idiomatic Rust Result<>. We want to pass it to a C-like
caller, i.e. satisfy the contract "on success, return a value and leave
argument @errp alone; on error, return an error value and store an Error
object in @errp".
(I'm ignoring void values for brevity)
The functions you mentioned split up the Result<>, do the storing, and
return the value to be returned.
> (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
We received a return value and possibly a C Error object. We want to
pass it to a Rust caller, i.e. return an idiomatic Result<>.
The functions you mentioned assemble the Result<> from the two values.
There's difference between Result<> and what we do in C: Result<> is
either a success value or an Error value. I.e. we have a success value
on success, an Error on failure. In C, we have a success value on
success, and both an error value and an Error on failure.
The (1) functions need to make up the missing error value. That's fine,
it's what we do in C, too.
The (2) functions throw away the error value. Usually, there's exactly
one error value, so no information is thrown away. But not necessarily.
I think it's okay to design for the common case now.
> 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.
"Understanding" is a bit strong. I relied on my voodoo-programming
skills.
Thank you!
