Re: [PATCH 07/12] qemu-api: add bindings to Error
Paolo Bonzini writes:
> On Wed, May 28, 2025 at 11:49 AM Markus Armbruster wrote:
>> > diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
>> > new file mode 100644
>> > index 000..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 = std::result::Result;
>> > +
>> > +#[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(),
>> > +
Re: [PATCH 07/12] qemu-api: add bindings to Error
On Wed, May 28, 2025 at 11:49 AM Markus Armbruster wrote:
> > diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> > new file mode 100644
> > index 000..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 = std::result::Result;
> > +
> > +#[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 {
> > +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()
Sho
Re: [PATCH 07/12] qemu-api: add bindings to Error
Paolo Bonzini 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
My Rust-fu is far to weak for an actual review. But let me try to
stumble through your patch. I'll pretend to explain the Rust code to a
noob like myself. Please correct my mistakes.
> ---
> rust/Cargo.lock| 17 +++
> rust/Cargo.toml| 1 +
> rust/meson.build | 2 +
> rust/qemu-api/Cargo.toml | 2 +
> rust/qemu-api/meson.build | 3 +-
> rust/qemu-api/src/error.rs | 273 +
> rust/qemu-api/src/lib.rs | 3 +
> 7 files changed, 300 insertions(+), 1 deletion(-)
> create mode 100644 rust/qemu-api/src/error.rs
>
> diff --git a/rust/Cargo.lock b/rust/Cargo.lock
> index 13d580c693b..37fd10064f9 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.2.0"
> +source = "registry+https://github.com/rust-lang/crates.io-index";
> +checksum = "37dd09e47ea8fd592a333f59fc52b894a97fe966ae9c6b7ef21ae38de6043462"
> +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 d9faeecb10b..2726b1f72e3 100644
> --- a/rust/Cargo.toml
> +++ b/rust/Cargo.toml
> @@ -67,6 +67,7 @@ multiple_crate_versions = "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/meson.build b/rust/meson.build
> index 3e0b6ed4afa..9eb69174dea 100644
> --- a/rust/meson.build
> +++ b/rust/meson.build
> @@ -1,11 +1,13 @@
> subproject('anyhow-1.0-rs', required: true)
> subproject('bilge-0.2-rs', required: true)
> subproject('bilge-impl-0.2-rs', required: true)
> +subproject('foreign-0.2-rs', required: true)
> subproject('libc-0.2-rs', required: true)
>
> anyhow_rs = dependency('anyhow-1.0-rs')
> bilge_rs = dependency('bilge-0.2-rs')
> bilge_impl_rs = dependency('bilge-impl-0.2-rs')
> +foreign_rs = dependency('foreign-0.2-rs')
> libc_rs = dependency('libc-0.2-rs')
>
> subproject('proc-macro2-1-rs', required: true)
> diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
> index c96cf50e7a1..ca6b81db10a 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.2"
>
> [features]
> default = ["debug_cell"]
> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index 1ea86b8bbf1..11cbd6d375a 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',
> @@ -35,7 +36,7 @@ _qemu_api_rs = static_library(
>override_options: ['rust_std=2021', 'build.rust_std=2021'],
>rust_abi: 'rust',
>rust_args: _qemu_api_cfg,
> - dependencies: [libc_rs, qemu_api_macros],
> + dependencies: [anyhow_rs, foreign_rs, libc_rs, qemu_api_macros],
> )
>
> rust.test('rust-qemu-api-tests', _qemu_api_rs,
Mere plumbing so far. I assume I don't need to understand it at this
point :)
> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 000..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},
> +};
> +
>
