* [PATCH 01/14] subprojects: add the anyhow crate
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-05-30 8:02 ` [PATCH 02/14] subprojects: add the foreign crate Paolo Bonzini
` (14 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
This is a standard replacement for Box<dyn Error> which is more efficient (it only
occcupies one word) and provides a backtrace of the error. This could be plumbed
into &error_abort in the future.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/meson.build | 2 ++
rust/qemu-api/meson.build | 2 +-
scripts/archive-source.sh | 2 +-
scripts/make-release | 2 +-
subprojects/.gitignore | 1 +
subprojects/anyhow-1-rs.wrap | 7 ++++
.../packagefiles/anyhow-1.0-rs/meson.build | 33 +++++++++++++++++++
7 files changed, 46 insertions(+), 3 deletions(-)
create mode 100644 subprojects/anyhow-1-rs.wrap
create mode 100644 subprojects/packagefiles/anyhow-1.0-rs/meson.build
diff --git a/rust/meson.build b/rust/meson.build
index afce62f4772..6227e01f32a 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,7 +1,9 @@
+subproject('anyhow-1-rs', required: true)
subproject('bilge-0.2-rs', required: true)
subproject('bilge-impl-0.2-rs', required: true)
subproject('libc-0.2-rs', required: true)
+anyhow_rs = dependency('anyhow-1-rs')
bilge_rs = dependency('bilge-0.2-rs')
bilge_impl_rs = dependency('bilge-impl-0.2-rs')
libc_rs = dependency('libc-0.2-rs')
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index d19f52af4da..181ceca9536 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -35,7 +35,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, qemuutil_rs,
+ dependencies: [anyhow_rs, libc_rs, qemu_api_macros, qemuutil_rs,
qom, hwcore, chardev, migration],
)
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index e461c1531ed..816062fee94 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -27,7 +27,7 @@ sub_file="${sub_tdir}/submodule.tar"
# in their checkout, because the build environment is completely
# different to the host OS.
subprojects="keycodemapdb libvfio-user berkeley-softfloat-3
- berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs
+ berkeley-testfloat-3 anyhow-1-rs arbitrary-int-1-rs bilge-0.2-rs
bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
syn-2-rs unicode-ident-1-rs"
diff --git a/scripts/make-release b/scripts/make-release
index 8c3594a1a47..ea65bdcc0cf 100755
--- a/scripts/make-release
+++ b/scripts/make-release
@@ -40,7 +40,7 @@ fi
# Only include wraps that are invoked with subproject()
SUBPROJECTS="libvfio-user keycodemapdb berkeley-softfloat-3
- berkeley-testfloat-3 arbitrary-int-1-rs bilge-0.2-rs
+ berkeley-testfloat-3 anyhow-1-rs arbitrary-int-1-rs bilge-0.2-rs
bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
syn-2-rs unicode-ident-1-rs"
diff --git a/subprojects/.gitignore b/subprojects/.gitignore
index d12d34618cc..b9ae507b85a 100644
--- a/subprojects/.gitignore
+++ b/subprojects/.gitignore
@@ -6,6 +6,7 @@
/keycodemapdb
/libvfio-user
/slirp
+/anyhow-1.0.98
/arbitrary-int-1.2.7
/bilge-0.2.0
/bilge-impl-0.2.0
diff --git a/subprojects/anyhow-1-rs.wrap b/subprojects/anyhow-1-rs.wrap
new file mode 100644
index 00000000000..a69a3645b49
--- /dev/null
+++ b/subprojects/anyhow-1-rs.wrap
@@ -0,0 +1,7 @@
+[wrap-file]
+directory = anyhow-1.0.98
+source_url = https://crates.io/api/v1/crates/anyhow/1.0.98/download
+source_filename = anyhow-1.0.98.tar.gz
+source_hash = e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487
+#method = cargo
+patch_directory = anyhow-1-rs
diff --git a/subprojects/packagefiles/anyhow-1.0-rs/meson.build b/subprojects/packagefiles/anyhow-1.0-rs/meson.build
new file mode 100644
index 00000000000..348bab98b9f
--- /dev/null
+++ b/subprojects/packagefiles/anyhow-1.0-rs/meson.build
@@ -0,0 +1,33 @@
+project('anyhow-1-rs', 'rust',
+ meson_version: '>=1.5.0',
+ version: '1.0.98',
+ license: 'MIT OR Apache-2.0',
+ default_options: [])
+
+rustc = meson.get_compiler('rust')
+
+rust_args = ['--cap-lints', 'allow']
+rust_args += ['--cfg', 'feature="std"']
+if rustc.version().version_compare('<1.65.0')
+ error('rustc version ' + rustc.version() + ' is unsupported. Please upgrade to at least 1.65.0')
+endif
+rust_args += [ '--cfg', 'std_backtrace' ] # >= 1.65.0
+if rustc.version().version_compare('<1.81.0')
+ rust_args += [ '--cfg', 'anyhow_no_core_error' ]
+endif
+
+_anyhow_rs = static_library(
+ 'anyhow',
+ files('src/lib.rs'),
+ gnu_symbol_visibility: 'hidden',
+ override_options: ['rust_std=2018', 'build.rust_std=2018'],
+ rust_abi: 'rust',
+ rust_args: rust_args,
+ dependencies: [],
+)
+
+anyhow_dep = declare_dependency(
+ link_with: _anyhow_rs,
+)
+
+meson.override_dependency('anyhow-1-rs', anyhow_dep)
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 02/14] subprojects: add the foreign crate
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
2025-05-30 8:02 ` [PATCH 01/14] subprojects: add the anyhow crate Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-05-30 8:02 ` [PATCH 03/14] util/error: expose Error definition to Rust code Paolo Bonzini
` (13 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
This is a cleaned up and separated version of the patches at
https://lore.kernel.org/all/20240701145853.1394967-4-pbonzini@redhat.com/
https://lore.kernel.org/all/20240701145853.1394967-5-pbonzini@redhat.com/
Its first user will be the Error bindings; for example a QEMU Error ** can be
converted to a Rust Option using
unsafe { Option::<Error>::from_foreign(c_error) }
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/meson.build | 2 ++
rust/qemu-api/meson.build | 2 +-
scripts/archive-source.sh | 3 ++-
scripts/make-release | 3 ++-
subprojects/.gitignore | 1 +
subprojects/foreign-0.3-rs.wrap | 7 +++++
.../packagefiles/foreign-0.3-rs/meson.build | 26 +++++++++++++++++++
7 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 subprojects/foreign-0.3-rs.wrap
create mode 100644 subprojects/packagefiles/foreign-0.3-rs/meson.build
diff --git a/rust/meson.build b/rust/meson.build
index 6227e01f32a..59c7ed2736b 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,11 +1,13 @@
subproject('anyhow-1-rs', required: true)
subproject('bilge-0.2-rs', required: true)
subproject('bilge-impl-0.2-rs', required: true)
+subproject('foreign-0.3-rs', required: true)
subproject('libc-0.2-rs', required: true)
anyhow_rs = dependency('anyhow-1-rs')
bilge_rs = dependency('bilge-0.2-rs')
bilge_impl_rs = dependency('bilge-impl-0.2-rs')
+foreign_rs = dependency('foreign-0.3-rs')
libc_rs = dependency('libc-0.2-rs')
subproject('proc-macro2-1-rs', required: true)
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 181ceca9536..aa22252866d 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -35,7 +35,7 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
- dependencies: [anyhow_rs, libc_rs, qemu_api_macros, qemuutil_rs,
+ dependencies: [anyhow_rs, foreign_rs, libc_rs, qemu_api_macros, qemuutil_rs,
qom, hwcore, chardev, migration],
)
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index 816062fee94..035828c532e 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -28,7 +28,8 @@ sub_file="${sub_tdir}/submodule.tar"
# different to the host OS.
subprojects="keycodemapdb libvfio-user berkeley-softfloat-3
berkeley-testfloat-3 anyhow-1-rs arbitrary-int-1-rs bilge-0.2-rs
- bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-rs proc-macro2-1-rs
+ bilge-impl-0.2-rs either-1-rs foreign-0.3-rs itertools-0.11-rs
+ libc-0.2-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
syn-2-rs unicode-ident-1-rs"
sub_deinit=""
diff --git a/scripts/make-release b/scripts/make-release
index ea65bdcc0cf..4509a9fabf5 100755
--- a/scripts/make-release
+++ b/scripts/make-release
@@ -41,7 +41,8 @@ fi
# Only include wraps that are invoked with subproject()
SUBPROJECTS="libvfio-user keycodemapdb berkeley-softfloat-3
berkeley-testfloat-3 anyhow-1-rs arbitrary-int-1-rs bilge-0.2-rs
- bilge-impl-0.2-rs either-1-rs itertools-0.11-rs libc-0.2-rs proc-macro2-1-rs
+ bilge-impl-0.2-rs either-1-rs foreign-0.3-rs itertools-0.11-rs
+ libc-0.2-rs proc-macro2-1-rs
proc-macro-error-1-rs proc-macro-error-attr-1-rs quote-1-rs
syn-2-rs unicode-ident-1-rs"
diff --git a/subprojects/.gitignore b/subprojects/.gitignore
index b9ae507b85a..f4281934ce1 100644
--- a/subprojects/.gitignore
+++ b/subprojects/.gitignore
@@ -11,6 +11,7 @@
/bilge-0.2.0
/bilge-impl-0.2.0
/either-1.12.0
+/foreign-0.3.1
/itertools-0.11.0
/libc-0.2.162
/proc-macro-error-1.0.4
diff --git a/subprojects/foreign-0.3-rs.wrap b/subprojects/foreign-0.3-rs.wrap
new file mode 100644
index 00000000000..0d218ec2c25
--- /dev/null
+++ b/subprojects/foreign-0.3-rs.wrap
@@ -0,0 +1,7 @@
+[wrap-file]
+directory = foreign-0.3.1
+source_url = https://crates.io/api/v1/crates/foreign/0.3.1/download
+source_filename = foreign-0.3.1.tar.gz
+source_hash = 17ca1b5be8c9d320daf386f1809c7acc0cb09accbae795c2001953fa50585846
+#method = cargo
+patch_directory = foreign-0.3-rs
diff --git a/subprojects/packagefiles/foreign-0.3-rs/meson.build b/subprojects/packagefiles/foreign-0.3-rs/meson.build
new file mode 100644
index 00000000000..0901c02c527
--- /dev/null
+++ b/subprojects/packagefiles/foreign-0.3-rs/meson.build
@@ -0,0 +1,26 @@
+project('foreign-0.3-rs', 'rust',
+ meson_version: '>=1.5.0',
+ version: '0.2.0',
+ license: 'MIT OR Apache-2.0',
+ default_options: [])
+
+subproject('libc-0.2-rs', required: true)
+libc_rs = dependency('libc-0.2-rs')
+
+_foreign_rs = static_library(
+ 'foreign',
+ files('src/lib.rs'),
+ gnu_symbol_visibility: 'hidden',
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ rust_args: [
+ '--cap-lints', 'allow',
+ ],
+ dependencies: [libc_rs],
+)
+
+foreign_dep = declare_dependency(
+ link_with: _foreign_rs,
+)
+
+meson.override_dependency('foreign-0.3-rs', foreign_dep)
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 03/14] util/error: expose Error definition to Rust code
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
2025-05-30 8:02 ` [PATCH 01/14] subprojects: add the anyhow crate Paolo Bonzini
2025-05-30 8:02 ` [PATCH 02/14] subprojects: add the foreign crate Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-06-03 3:06 ` Zhao Liu
2025-05-30 8:02 ` [PATCH 04/14] util/error: allow non-NUL-terminated err->src Paolo Bonzini
` (12 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
This is used to preserve the file and line in a roundtrip from
C Error to Rust and back to C.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qapi/error-internal.h | 26 ++++++++++++++++++++++++++
rust/wrapper.h | 1 +
util/error.c | 10 +---------
3 files changed, 28 insertions(+), 9 deletions(-)
create mode 100644 include/qapi/error-internal.h
diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
new file mode 100644
index 00000000000..d5c3904adec
--- /dev/null
+++ b/include/qapi/error-internal.h
@@ -0,0 +1,26 @@
+/*
+ * QEMU Error Objects - struct definition
+ *
+ * Copyright IBM, Corp. 2011
+ * Copyright (C) 2011-2015 Red Hat, Inc.
+ *
+ * Authors:
+ * Anthony Liguori <aliguori@us.ibm.com>
+ * Markus Armbruster <armbru@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2. See
+ * the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef QAPI_ERROR_INTERNAL_H
+
+struct Error
+{
+ char *msg;
+ ErrorClass err_class;
+ const char *src, *func;
+ int line;
+ GString *hint;
+};
+
+#endif
diff --git a/rust/wrapper.h b/rust/wrapper.h
index beddd9aab2f..6060d3ba1ab 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -60,6 +60,7 @@ typedef enum memory_order {
#include "hw/qdev-properties-system.h"
#include "hw/irq.h"
#include "qapi/error.h"
+#include "qapi/error-internal.h"
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
#include "exec/memattrs.h"
diff --git a/util/error.c b/util/error.c
index 673011b89e9..e5bcb7c0225 100644
--- a/util/error.c
+++ b/util/error.c
@@ -15,15 +15,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
-
-struct Error
-{
- char *msg;
- ErrorClass err_class;
- const char *src, *func;
- int line;
- GString *hint;
-};
+#include "qapi/error-internal.h"
Error *error_abort;
Error *error_fatal;
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 03/14] util/error: expose Error definition to Rust code
2025-05-30 8:02 ` [PATCH 03/14] util/error: expose Error definition to Rust code Paolo Bonzini
@ 2025-06-03 3:06 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-06-03 3:06 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, May 30, 2025 at 10:02:55AM +0200, Paolo Bonzini wrote:
> Date: Fri, 30 May 2025 10:02:55 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/14] util/error: expose Error definition to Rust code
> X-Mailer: git-send-email 2.49.0
>
> This is used to preserve the file and line in a roundtrip from
> C Error to Rust and back to C.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qapi/error-internal.h | 26 ++++++++++++++++++++++++++
> rust/wrapper.h | 1 +
> util/error.c | 10 +---------
> 3 files changed, 28 insertions(+), 9 deletions(-)
> create mode 100644 include/qapi/error-internal.h
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 04/14] util/error: allow non-NUL-terminated err->src
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (2 preceding siblings ...)
2025-05-30 8:02 ` [PATCH 03/14] util/error: expose Error definition to Rust code Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-06-02 10:47 ` Markus Armbruster
2025-05-30 8:02 ` [PATCH 05/14] util/error: make func optional Paolo Bonzini
` (11 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Rust makes the current file available as a statically-allocated string,
but without a NUL terminator. Allow this by storing an optional maximum
length in the Error.
Note that for portability I am not relying on fprintf's precision
specifier not accessing memory beyond what will be printed.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qapi/error-internal.h | 1 +
util/error.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
index d5c3904adec..f5eb8ad2379 100644
--- a/include/qapi/error-internal.h
+++ b/include/qapi/error-internal.h
@@ -19,6 +19,7 @@ struct Error
char *msg;
ErrorClass err_class;
const char *src, *func;
+ int src_len;
int line;
GString *hint;
};
diff --git a/util/error.c b/util/error.c
index e5bcb7c0225..3449ecc0b92 100644
--- a/util/error.c
+++ b/util/error.c
@@ -24,8 +24,8 @@ Error *error_warn;
static void error_handle(Error **errp, Error *err)
{
if (errp == &error_abort) {
- fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
- err->func, err->src, err->line);
+ fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
+ err->func, err->src_len, err->src, err->line);
error_report("%s", error_get_pretty(err));
if (err->hint) {
error_printf("%s", err->hint->str);
@@ -67,6 +67,7 @@ static void error_setv(Error **errp,
g_free(msg);
}
err->err_class = err_class;
+ err->src_len = -1;
err->src = src;
err->line = line;
err->func = func;
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 04/14] util/error: allow non-NUL-terminated err->src
2025-05-30 8:02 ` [PATCH 04/14] util/error: allow non-NUL-terminated err->src Paolo Bonzini
@ 2025-06-02 10:47 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-06-02 10:47 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> Rust makes the current file available as a statically-allocated string,
> but without a NUL terminator. Allow this by storing an optional maximum
> length in the Error.
>
> Note that for portability I am not relying on fprintf's precision
> specifier not accessing memory beyond what will be printed.
Stale paragraph :)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qapi/error-internal.h | 1 +
> util/error.c | 5 +++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
> index d5c3904adec..f5eb8ad2379 100644
> --- a/include/qapi/error-internal.h
> +++ b/include/qapi/error-internal.h
> @@ -19,6 +19,7 @@ struct Error
> char *msg;
> ErrorClass err_class;
> const char *src, *func;
> + int src_len;
In actual usage, we have two cases:
* @src_len is -1 and @src is null-terminated
* @src_len is non-negative and @src is an array of at least that many
characters, not necessarily null-terminated
This is locally unobvious, and therefore deserves a comment.
Unterminated char * pretty much always deserve one :)
> int line;
> GString *hint;
> };
> diff --git a/util/error.c b/util/error.c
> index e5bcb7c0225..3449ecc0b92 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -24,8 +24,8 @@ Error *error_warn;
> static void error_handle(Error **errp, Error *err)
> {
> if (errp == &error_abort) {
> - fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> - err->func, err->src, err->line);
> + fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
> + err->func, err->src_len, err->src, err->line);
> error_report("%s", error_get_pretty(err));
> if (err->hint) {
> error_printf("%s", err->hint->str);
> @@ -67,6 +67,7 @@ static void error_setv(Error **errp,
> g_free(msg);
> }
> err->err_class = err_class;
> + err->src_len = -1;
> err->src = src;
> err->line = line;
> err->func = func;
This part looks fine to me.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 05/14] util/error: make func optional
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (3 preceding siblings ...)
2025-05-30 8:02 ` [PATCH 04/14] util/error: allow non-NUL-terminated err->src Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-06-02 10:52 ` Markus Armbruster
2025-05-30 8:02 ` [PATCH 06/14] rust: qemu-api: add bindings to Error Paolo Bonzini
` (10 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
The function name is not available in Rust, so make it optional.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/error.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/util/error.c b/util/error.c
index 3449ecc0b92..daea2142f30 100644
--- a/util/error.c
+++ b/util/error.c
@@ -24,8 +24,13 @@ Error *error_warn;
static void error_handle(Error **errp, Error *err)
{
if (errp == &error_abort) {
- fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
- err->func, err->src_len, err->src, err->line);
+ if (err->func) {
+ fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
+ err->func, err->src_len, err->src, err->line);
+ } else {
+ fprintf(stderr, "Unexpected error at %.*s:%d:\n",
+ err->src_len, err->src, err->line);
+ }
error_report("%s", error_get_pretty(err));
if (err->hint) {
error_printf("%s", err->hint->str);
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 05/14] util/error: make func optional
2025-05-30 8:02 ` [PATCH 05/14] util/error: make func optional Paolo Bonzini
@ 2025-06-02 10:52 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-06-02 10:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust, Zhao Liu
Paolo Bonzini <pbonzini@redhat.com> writes:
> The function name is not available in Rust, so make it optional.
>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/error.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/util/error.c b/util/error.c
> index 3449ecc0b92..daea2142f30 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -24,8 +24,13 @@ Error *error_warn;
> static void error_handle(Error **errp, Error *err)
> {
> if (errp == &error_abort) {
> - fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
> - err->func, err->src_len, err->src, err->line);
> + if (err->func) {
> + fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
> + err->func, err->src_len, err->src, err->line);
> + } else {
> + fprintf(stderr, "Unexpected error at %.*s:%d:\n",
> + err->src_len, err->src, err->line);
> + }
> error_report("%s", error_get_pretty(err));
> if (err->hint) {
> error_printf("%s", err->hint->str);
This changes struct Error member @func from non-null to maybe-null. An
argument could be made for a comment there. But since "can this be
null" questions are *everywhere* in C, it would be a rather weak
argument: competent programmers are unlikely to be misled.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (4 preceding siblings ...)
2025-05-30 8:02 ` [PATCH 05/14] util/error: make func optional Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-06-02 13:18 ` Markus Armbruster
2025-05-30 8:02 ` [PATCH 07/14] rust: qemu-api: add tests for Error bindings Paolo Bonzini
` (9 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-05-30 8:02 ` [PATCH 06/14] rust: qemu-api: add bindings to Error Paolo Bonzini
@ 2025-06-02 13:18 ` Markus Armbruster
2025-06-03 9:29 ` Zhao Liu
2025-06-03 15:37 ` Paolo Bonzini
0 siblings, 2 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-06-02 13:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
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(
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-02 13:18 ` Markus Armbruster
@ 2025-06-03 9:29 ` Zhao Liu
2025-06-03 10:32 ` Markus Armbruster
2025-06-03 15:37 ` Paolo Bonzini
1 sibling, 1 reply; 32+ messages in thread
From: Zhao Liu @ 2025-06-03 9:29 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel, qemu-rust
> > + /// 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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-03 9:29 ` Zhao Liu
@ 2025-06-03 10:32 ` Markus Armbruster
2025-06-03 15:05 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2025-06-03 10:32 UTC (permalink / raw)
To: Zhao Liu; +Cc: Paolo Bonzini, qemu-devel, qemu-rust
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()?
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-03 10:32 ` Markus Armbruster
@ 2025-06-03 15:05 ` Paolo Bonzini
2025-06-04 5:01 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-06-03 15:05 UTC (permalink / raw)
To: Markus Armbruster, Zhao Liu; +Cc: qemu-devel, qemu-rust
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-03 15:05 ` Paolo Bonzini
@ 2025-06-04 5:01 ` Markus Armbruster
2025-06-04 19:19 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2025-06-04 5:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Zhao Liu, qemu-devel, qemu-rust
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 :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-04 5:01 ` Markus Armbruster
@ 2025-06-04 19:19 ` Paolo Bonzini
2025-06-05 6:14 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-06-04 19:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Zhao Liu, qemu-devel, qemu-rust
On 6/4/25 07:01, Markus Armbruster wrote:
> 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.
>
> [...]
>
> 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?
I agree that there are arguments for both. The reason to use
error_setg() is that, even though these functions "propagate" a
qemu_api::Error into a C Error**, the error is born in the Rust callback
and therefore there is no error_setg() anywhere that could check for
non-NULL abort(). There is a bigger risk of triggering
error_propagate()'s weird behavior.
The reason to use error_propagate() is that these functions do look a
lot more like error_propagate() than error_setg(). I'm undecided. I
think I'll keep the error_setg() semantics, which is essentially
assert_eq!(unsafe { *errp }, ptr::null_mut());
followed by calling bindings::error_propagate().
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-04 19:19 ` Paolo Bonzini
@ 2025-06-05 6:14 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-06-05 6:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Zhao Liu, qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 6/4/25 07:01, Markus Armbruster wrote:
>> 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.
>>
>> [...]
>>
>> 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?
>>
> I agree that there are arguments for both. The reason to use
> error_setg() is that, even though these functions "propagate" a
> qemu_api::Error into a C Error**, the error is born in the Rust callback
> and therefore there is no error_setg() anywhere that could check for
> non-NULL abort(). There is a bigger risk of triggering
> error_propagate()'s weird behavior.
Yes.
> The reason to use error_propagate() is that these functions do look a
> lot more like error_propagate() than error_setg().
True.
> I'm undecided. I
> think I'll keep the error_setg() semantics, which is essentially
>
> assert_eq!(unsafe { *errp }, ptr::null_mut());
>
> followed by calling bindings::error_propagate().
Works for me.
The error_propagate() then does nothing but call error_handle().
However, error_handle() is static, and making it available for Rust
just to cut out a harmless middleman seems hardly worth the bother.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 06/14] rust: qemu-api: add bindings to Error
2025-06-02 13:18 ` Markus Armbruster
2025-06-03 9:29 ` Zhao Liu
@ 2025-06-03 15:37 ` Paolo Bonzini
1 sibling, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-06-03 15:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-rust
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 07/14] rust: qemu-api: add tests for Error bindings
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (5 preceding siblings ...)
2025-05-30 8:02 ` [PATCH 06/14] rust: qemu-api: add bindings to Error Paolo Bonzini
@ 2025-05-30 8:02 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 08/14] rust: qdev: support returning errors from realize Paolo Bonzini
` (8 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/error.rs | 104 +++++++++++++++++++++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
index 0bdd413a0a2..a91ce6fefaf 100644
--- a/rust/qemu-api/src/error.rs
+++ b/rust/qemu-api/src/error.rs
@@ -297,3 +297,107 @@ unsafe fn cloned_from_foreign(c_error: *const bindings::Error) -> Self {
}
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::ffi::CStr;
+
+ use anyhow::anyhow;
+ use foreign::OwnedPointer;
+
+ use super::*;
+ use crate::{assert_match, bindings};
+
+ #[track_caller]
+ fn error_for_test(msg: &CStr) -> OwnedPointer<Error> {
+ // SAFETY: all arguments are controlled by this function
+ let location = panic::Location::caller();
+ 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: msg.clone_to_foreign_ptr(),
+ err_class: bindings::ERROR_CLASS_GENERIC_ERROR,
+ src_len: location.file().len() as c_int,
+ src: location.file().as_ptr().cast::<c_char>(),
+ line: location.line() as c_int,
+ func: ptr::null_mut(),
+ hint: ptr::null_mut(),
+ };
+ OwnedPointer::new(err)
+ }
+ }
+
+ unsafe fn error_get_pretty<'a>(local_err: *mut bindings::Error) -> &'a CStr {
+ unsafe { CStr::from_ptr(bindings::error_get_pretty(local_err)) }
+ }
+
+ #[test]
+ #[allow(deprecated)]
+ fn test_description() {
+ use std::error::Error;
+
+ assert_eq!(super::Error::from("msg").description(), "msg");
+ assert_eq!(super::Error::from("msg".to_owned()).description(), "msg");
+ }
+
+ #[test]
+ fn test_display() {
+ assert_eq!(&*format!("{}", Error::from("msg")), "msg");
+ assert_eq!(&*format!("{}", Error::from("msg".to_owned())), "msg");
+ assert_eq!(&*format!("{}", Error::from(anyhow!("msg"))), "msg");
+
+ assert_eq!(
+ &*format!("{}", Error::with_error("msg", anyhow!("cause"))),
+ "msg: cause"
+ );
+ }
+
+ #[test]
+ fn test_bool_or_propagate() {
+ unsafe {
+ let mut local_err: *mut bindings::Error = ptr::null_mut();
+
+ assert!(Error::bool_or_propagate(Ok(()), &mut local_err));
+ assert_eq!(local_err, ptr::null_mut());
+
+ let my_err = Error::from("msg");
+ assert!(!Error::bool_or_propagate(Err(my_err), &mut local_err));
+ assert_ne!(local_err, ptr::null_mut());
+ assert_eq!(error_get_pretty(local_err), c"msg");
+ bindings::error_free(local_err);
+ }
+ }
+
+ #[test]
+ fn test_ptr_or_propagate() {
+ unsafe {
+ let mut local_err: *mut bindings::Error = ptr::null_mut();
+
+ let ret = Error::ptr_or_propagate(Ok("abc".to_owned()), &mut local_err);
+ assert_eq!(String::from_foreign(ret), "abc");
+ assert_eq!(local_err, ptr::null_mut());
+
+ let my_err = Error::from("msg");
+ assert_eq!(
+ Error::ptr_or_propagate(Err::<String, _>(my_err), &mut local_err),
+ ptr::null_mut()
+ );
+ assert_ne!(local_err, ptr::null_mut());
+ assert_eq!(error_get_pretty(local_err), c"msg");
+ bindings::error_free(local_err);
+ }
+ }
+
+ #[test]
+ fn test_err_or_unit() {
+ unsafe {
+ let result = Error::err_or_unit(ptr::null_mut());
+ assert_match!(result, Ok(()));
+
+ let err = error_for_test(c"msg");
+ let err = Error::err_or_unit(err.into_inner()).unwrap_err();
+ assert_eq!(&*format!("{err}"), "msg");
+ }
+ }
+}
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 08/14] rust: qdev: support returning errors from realize
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (6 preceding siblings ...)
2025-05-30 8:02 ` [PATCH 07/14] rust: qemu-api: add tests for Error bindings Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 09/14] rust/hpet: change type of num_timers to usize Paolo Bonzini
` (7 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 5 +++--
rust/hw/timer/hpet/src/device.rs | 5 +++--
rust/qemu-api/src/qdev.rs | 10 ++++++----
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index bd5cee04647..1e715e91bc3 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -175,7 +175,7 @@ fn properties() -> &'static [Property] {
fn vmsd() -> Option<&'static VMStateDescription> {
Some(&device_class::VMSTATE_PL011)
}
- const REALIZE: Option<fn(&Self)> = Some(Self::realize);
+ const REALIZE: Option<fn(&Self) -> qemu_api::Result<()>> = Some(Self::realize);
}
impl ResettablePhasesImpl for PL011State {
@@ -619,9 +619,10 @@ fn event(&self, event: Event) {
}
}
- fn realize(&self) {
+ fn realize(&self) -> qemu_api::Result<()> {
self.char_backend
.enable_handlers(self, Self::can_receive, Self::receive, Self::event);
+ Ok(())
}
fn reset_hold(&self, _type: ResetType) {
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index e3ba62b2875..68c82b09b60 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -724,7 +724,7 @@ fn post_init(&self) {
}
}
- fn realize(&self) {
+ fn realize(&self) -> qemu_api::Result<()> {
if self.int_route_cap == 0 {
// TODO: Add error binding: warn_report()
println!("Hpet's hpet-intcap property not initialized");
@@ -751,6 +751,7 @@ fn realize(&self) {
self.init_gpio_in(2, HPETState::handle_legacy_irq);
self.init_gpio_out(from_ref(&self.pit_enabled));
+ Ok(())
}
fn reset_hold(&self, _type: ResetType) {
@@ -1042,7 +1043,7 @@ fn vmsd() -> Option<&'static VMStateDescription> {
Some(&VMSTATE_HPET)
}
- const REALIZE: Option<fn(&Self)> = Some(Self::realize);
+ const REALIZE: Option<fn(&Self) -> qemu_api::Result<()>> = Some(Self::realize);
}
impl ResettablePhasesImpl for HPETState {
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 1279d7a58d5..81052097071 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -12,10 +12,11 @@
pub use bindings::{ClockEvent, DeviceClass, Property, ResetType};
use crate::{
- bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error, ResettableClass},
+ bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, ResettableClass},
callbacks::FnCall,
cell::{bql_locked, Opaque},
chardev::Chardev,
+ error::{Error, Result},
irq::InterruptSource,
prelude::*,
qom::{ObjectClass, ObjectImpl, Owned},
@@ -108,7 +109,7 @@ pub trait DeviceImpl: ObjectImpl + ResettablePhasesImpl + IsA<DeviceState> {
///
/// If not `None`, the parent class's `realize` method is overridden
/// with the function pointed to by `REALIZE`.
- const REALIZE: Option<fn(&Self)> = None;
+ const REALIZE: Option<fn(&Self) -> Result<()>> = None;
/// An array providing the properties that the user can set on the
/// device. Not a `const` because referencing statics in constants
@@ -134,10 +135,11 @@ fn vmsd() -> Option<&'static VMStateDescription> {
/// readable/writeable from one thread at any time.
unsafe extern "C" fn rust_realize_fn<T: DeviceImpl>(
dev: *mut bindings::DeviceState,
- _errp: *mut *mut Error,
+ errp: *mut *mut bindings::Error,
) {
let state = NonNull::new(dev).unwrap().cast::<T>();
- T::REALIZE.unwrap()(unsafe { state.as_ref() });
+ let result = T::REALIZE.unwrap()(unsafe { state.as_ref() });
+ unsafe { Error::ok_or_propagate(result, errp); }
}
unsafe impl InterfaceType for ResettableClass {
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/14] rust/hpet: change type of num_timers to usize
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (7 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 08/14] rust: qdev: support returning errors from realize Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 10/14] hpet: adjust VMState for consistency with Rust version Paolo Bonzini
` (6 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
Remove the need to convert after every read of the BqlCell. Because the
vmstate uses a u8 as the size of the VARRAY, this requires switching
the VARRAY to use num_timers_save; which in turn requires ensuring that
the num_timers_save is always there. For simplicity do this by
removing support for version 1, which QEMU has not been producing for
~15 years.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/device.rs | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index 68c82b09b60..a957de1e767 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -12,7 +12,7 @@
use qemu_api::{
bindings::{
address_space_memory, address_space_stl_le, qdev_prop_bit, qdev_prop_bool,
- qdev_prop_uint32, qdev_prop_uint8,
+ qdev_prop_uint32, qdev_prop_usize,
},
cell::{BqlCell, BqlRefCell},
irq::InterruptSource,
@@ -36,9 +36,9 @@
const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
/// Minimum recommended hardware implementation.
-const HPET_MIN_TIMERS: u8 = 3;
+const HPET_MIN_TIMERS: usize = 3;
/// Maximum timers in each timer block.
-const HPET_MAX_TIMERS: u8 = 32;
+const HPET_MAX_TIMERS: usize = 32;
/// Flags that HPETState.flags supports.
const HPET_FLAG_MSI_SUPPORT_SHIFT: usize = 0;
@@ -561,8 +561,8 @@ pub struct HPETState {
/// HPET timer array managed by this timer block.
#[doc(alias = "timer")]
- timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS as usize],
- num_timers: BqlCell<u8>,
+ timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS],
+ num_timers: BqlCell<usize>,
num_timers_save: BqlCell<u8>,
/// Instance id (HPET timer block ID).
@@ -572,7 +572,7 @@ pub struct HPETState {
impl HPETState {
// Get num_timers with `usize` type, which is useful to play with array index.
fn get_num_timers(&self) -> usize {
- self.num_timers.get().into()
+ self.num_timers.get()
}
const fn has_msi_flag(&self) -> bool {
@@ -854,7 +854,7 @@ fn pre_save(&self) -> i32 {
* also added to the migration stream. Check that it matches the value
* that was configured.
*/
- self.num_timers_save.set(self.num_timers.get());
+ self.num_timers_save.set(self.num_timers.get() as u8);
0
}
@@ -884,7 +884,7 @@ fn is_offset_needed(&self) -> bool {
}
fn validate_num_timers(&self, _version_id: u8) -> bool {
- self.num_timers.get() == self.num_timers_save.get()
+ self.num_timers.get() == self.num_timers_save.get().into()
}
}
@@ -911,7 +911,7 @@ impl ObjectImpl for HPETState {
c"timers",
HPETState,
num_timers,
- unsafe { &qdev_prop_uint8 },
+ unsafe { &qdev_prop_usize },
u8,
default = HPET_MIN_TIMERS
),
@@ -1016,16 +1016,16 @@ impl ObjectImpl for HPETState {
static VMSTATE_HPET: VMStateDescription = VMStateDescription {
name: c"hpet".as_ptr(),
version_id: 2,
- minimum_version_id: 1,
+ minimum_version_id: 2,
pre_save: Some(hpet_pre_save),
post_load: Some(hpet_post_load),
fields: vmstate_fields! {
vmstate_of!(HPETState, config),
vmstate_of!(HPETState, int_status),
vmstate_of!(HPETState, counter),
- vmstate_of!(HPETState, num_timers_save).with_version_id(2),
+ vmstate_of!(HPETState, num_timers_save),
vmstate_validate!(HPETState, VALIDATE_TIMERS_NAME, HPETState::validate_num_timers),
- vmstate_struct!(HPETState, timers[0 .. num_timers], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
+ vmstate_struct!(HPETState, timers[0 .. num_timers_save], &VMSTATE_HPET_TIMER, BqlRefCell<HPETTimer>, HPETState::validate_num_timers).with_version_id(0),
},
subsections: vmstate_subsections! {
VMSTATE_HPET_RTC_IRQ_LEVEL,
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 10/14] hpet: adjust VMState for consistency with Rust version
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (8 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 09/14] rust/hpet: change type of num_timers to usize Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-06-03 3:11 ` Zhao Liu
2025-05-30 8:03 ` [PATCH 11/14] hpet: return errors from realize if properties are incorrect Paolo Bonzini
` (5 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
No functional change intended.
Suggested-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/hpet.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 0fd1337a156..9db027cf76f 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -328,16 +328,16 @@ static const VMStateDescription vmstate_hpet_timer = {
static const VMStateDescription vmstate_hpet = {
.name = "hpet",
.version_id = 2,
- .minimum_version_id = 1,
+ .minimum_version_id = 2,
.pre_save = hpet_pre_save,
.post_load = hpet_post_load,
.fields = (const VMStateField[]) {
VMSTATE_UINT64(config, HPETState),
VMSTATE_UINT64(isr, HPETState),
VMSTATE_UINT64(hpet_counter, HPETState),
- VMSTATE_UINT8_V(num_timers_save, HPETState, 2),
+ VMSTATE_UINT8(num_timers_save, HPETState),
VMSTATE_VALIDATE("num_timers must match", hpet_validate_num_timers),
- VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,
+ VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers_save, 0,
vmstate_hpet_timer, HPETTimer),
VMSTATE_END_OF_LIST()
},
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/14] hpet: return errors from realize if properties are incorrect
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (9 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 10/14] hpet: adjust VMState for consistency with Rust version Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 12/14] rust/hpet: " Paolo Bonzini
` (4 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Markus Armbruster, Zhao Liu
Do not silently adjust num_timers, and fail if intcap is 0.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/timer/hpet.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 9db027cf76f..cb48cc151f1 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -691,8 +691,14 @@ static void hpet_realize(DeviceState *dev, Error **errp)
int i;
HPETTimer *timer;
+ if (s->num_timers < HPET_MIN_TIMERS || s->num_timers > HPET_MAX_TIMERS) {
+ error_setg(errp, "hpet.num_timers must be between %d and %d",
+ HPET_MIN_TIMERS, HPET_MAX_TIMERS);
+ return;
+ }
if (!s->intcap) {
- warn_report("Hpet's intcap not initialized");
+ error_setg(errp, "hpet.hpet-intcap not initialized");
+ return;
}
if (hpet_fw_cfg.count == UINT8_MAX) {
/* first instance */
@@ -700,7 +706,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
}
if (hpet_fw_cfg.count == 8) {
- error_setg(errp, "Only 8 instances of HPET is allowed");
+ error_setg(errp, "Only 8 instances of HPET are allowed");
return;
}
@@ -710,11 +716,6 @@ static void hpet_realize(DeviceState *dev, Error **errp)
sysbus_init_irq(sbd, &s->irqs[i]);
}
- if (s->num_timers < HPET_MIN_TIMERS) {
- s->num_timers = HPET_MIN_TIMERS;
- } else if (s->num_timers > HPET_MAX_TIMERS) {
- s->num_timers = HPET_MAX_TIMERS;
- }
for (i = 0; i < HPET_MAX_TIMERS; i++) {
timer = &s->timer[i];
timer->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, hpet_timer, timer);
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 12/14] rust/hpet: return errors from realize if properties are incorrect
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (10 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 11/14] hpet: return errors from realize if properties are incorrect Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 13/14] rust/hpet: Drop BqlCell wrapper for num_timers Paolo Bonzini
` (3 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
Match the code in hpet.c; this also allows removing the
BqlCell from the num_timers field.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/device.rs | 16 +++++++---------
rust/hw/timer/hpet/src/fw_cfg.rs | 7 +++----
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index a957de1e767..cd439f90b7e 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -725,18 +725,16 @@ fn post_init(&self) {
}
fn realize(&self) -> qemu_api::Result<()> {
+ if self.num_timers.get() < HPET_MIN_TIMERS || self.num_timers.get() > HPET_MAX_TIMERS {
+ Err(format!(
+ "hpet.num_timers must be between {HPET_MIN_TIMERS} and {HPET_MAX_TIMERS}"
+ ))?;
+ }
if self.int_route_cap == 0 {
- // TODO: Add error binding: warn_report()
- println!("Hpet's hpet-intcap property not initialized");
+ Err("hpet.hpet-intcap property not initialized")?;
}
- self.hpet_id.set(HPETFwConfig::assign_hpet_id());
-
- if self.num_timers.get() < HPET_MIN_TIMERS {
- self.num_timers.set(HPET_MIN_TIMERS);
- } else if self.num_timers.get() > HPET_MAX_TIMERS {
- self.num_timers.set(HPET_MAX_TIMERS);
- }
+ self.hpet_id.set(HPETFwConfig::assign_hpet_id()?);
self.init_timer();
// 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
index 6c10316104c..619d662ee1e 100644
--- a/rust/hw/timer/hpet/src/fw_cfg.rs
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -36,7 +36,7 @@ unsafe impl Zeroable for HPETFwConfig {}
};
impl HPETFwConfig {
- pub(crate) fn assign_hpet_id() -> usize {
+ pub(crate) fn assign_hpet_id() -> Result<usize, &'static str> {
assert!(bql_locked());
// SAFETY: all accesses go through these methods, which guarantee
// that the accesses are protected by the BQL.
@@ -48,13 +48,12 @@ pub(crate) fn assign_hpet_id() -> usize {
}
if fw_cfg.count == 8 {
- // TODO: Add error binding: error_setg()
- panic!("Only 8 instances of HPET is allowed");
+ Err("Only 8 instances of HPET are allowed")?;
}
let id: usize = fw_cfg.count.into();
fw_cfg.count += 1;
- id
+ Ok(id)
}
pub(crate) fn update_hpet_cfg(hpet_id: usize, timer_block_id: u32, address: u64) {
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 13/14] rust/hpet: Drop BqlCell wrapper for num_timers
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (11 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 12/14] rust/hpet: " Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-05-30 8:03 ` [PATCH 14/14] docs: update Rust module status Paolo Bonzini
` (2 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust, Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
Now that the num_timers field is initialized as a property, someone may
change its default value using qdev_prop_set_uint8(), but the value is
fixed after the Rust code sees it first. Since there is no need to modify
it after realize(), it is not to be necessary to have a BqlCell wrapper.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20250520152750.2542612-4-zhao1.liu@intel.com
[Remove .into() as well. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/device.rs | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/rust/hw/timer/hpet/src/device.rs b/rust/hw/timer/hpet/src/device.rs
index cd439f90b7e..735b2fbef79 100644
--- a/rust/hw/timer/hpet/src/device.rs
+++ b/rust/hw/timer/hpet/src/device.rs
@@ -562,7 +562,7 @@ pub struct HPETState {
/// HPET timer array managed by this timer block.
#[doc(alias = "timer")]
timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS],
- num_timers: BqlCell<usize>,
+ num_timers: usize,
num_timers_save: BqlCell<u8>,
/// Instance id (HPET timer block ID).
@@ -570,11 +570,6 @@ pub struct HPETState {
}
impl HPETState {
- // Get num_timers with `usize` type, which is useful to play with array index.
- fn get_num_timers(&self) -> usize {
- self.num_timers.get()
- }
-
const fn has_msi_flag(&self) -> bool {
self.flags & (1 << HPET_FLAG_MSI_SUPPORT_SHIFT) != 0
}
@@ -636,7 +631,7 @@ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
self.hpet_offset
.set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
- for timer in self.timers.iter().take(self.get_num_timers()) {
+ for timer in self.timers.iter().take(self.num_timers) {
let mut t = timer.borrow_mut();
if t.is_int_enabled() && t.is_int_active() {
@@ -648,7 +643,7 @@ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
// Halt main counter and disable interrupt generation.
self.counter.set(self.get_ticks());
- for timer in self.timers.iter().take(self.get_num_timers()) {
+ for timer in self.timers.iter().take(self.num_timers) {
timer.borrow_mut().del_timer();
}
}
@@ -671,7 +666,7 @@ fn set_int_status_reg(&self, shift: u32, _len: u32, val: u64) {
let new_val = val << shift;
let cleared = new_val & self.int_status.get();
- for (index, timer) in self.timers.iter().take(self.get_num_timers()).enumerate() {
+ for (index, timer) in self.timers.iter().take(self.num_timers).enumerate() {
if cleared & (1 << index) != 0 {
timer.borrow_mut().update_irq(false);
}
@@ -725,7 +720,7 @@ fn post_init(&self) {
}
fn realize(&self) -> qemu_api::Result<()> {
- if self.num_timers.get() < HPET_MIN_TIMERS || self.num_timers.get() > HPET_MAX_TIMERS {
+ if self.num_timers < HPET_MIN_TIMERS || self.num_timers > HPET_MAX_TIMERS {
Err(format!(
"hpet.num_timers must be between {HPET_MIN_TIMERS} and {HPET_MAX_TIMERS}"
))?;
@@ -743,7 +738,7 @@ fn realize(&self) -> qemu_api::Result<()> {
1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
1 << HPET_CAP_LEG_RT_CAP_SHIFT |
HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
- ((self.get_num_timers() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
+ ((self.num_timers - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
(HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
);
@@ -753,7 +748,7 @@ fn realize(&self) -> qemu_api::Result<()> {
}
fn reset_hold(&self, _type: ResetType) {
- for timer in self.timers.iter().take(self.get_num_timers()) {
+ for timer in self.timers.iter().take(self.num_timers) {
timer.borrow_mut().reset();
}
@@ -781,7 +776,7 @@ fn decode(&self, mut addr: hwaddr, size: u32) -> HPETAddrDecode {
GlobalRegister::try_from(addr).map(HPETRegister::Global)
} else {
let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
- if timer_id <= self.get_num_timers() {
+ if timer_id < self.num_timers {
// TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id)
TimerRegister::try_from(addr & 0x18)
.map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
@@ -852,12 +847,12 @@ fn pre_save(&self) -> i32 {
* also added to the migration stream. Check that it matches the value
* that was configured.
*/
- self.num_timers_save.set(self.num_timers.get() as u8);
+ self.num_timers_save.set(self.num_timers as u8);
0
}
fn post_load(&self, _version_id: u8) -> i32 {
- for timer in self.timers.iter().take(self.get_num_timers()) {
+ for timer in self.timers.iter().take(self.num_timers) {
let mut t = timer.borrow_mut();
t.cmp64 = t.calculate_cmp64(t.get_state().counter.get(), t.cmp);
@@ -882,7 +877,7 @@ fn is_offset_needed(&self) -> bool {
}
fn validate_num_timers(&self, _version_id: u8) -> bool {
- self.num_timers.get() == self.num_timers_save.get().into()
+ self.num_timers == self.num_timers_save.get().into()
}
}
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 14/14] docs: update Rust module status
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (12 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 13/14] rust/hpet: Drop BqlCell wrapper for num_timers Paolo Bonzini
@ 2025-05-30 8:03 ` Paolo Bonzini
2025-06-03 3:09 ` Zhao Liu
2025-06-02 7:49 ` [PATCH v2 00/14] rust: bindings for Error Markus Armbruster
2025-06-03 9:35 ` Zhao Liu
15 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-30 8:03 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-rust
error is new; offset_of is gone.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
docs/devel/rust.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/devel/rust.rst b/docs/devel/rust.rst
index 34d9c7945b7..29ae59395e8 100644
--- a/docs/devel/rust.rst
+++ b/docs/devel/rust.rst
@@ -155,10 +155,10 @@ module status
``callbacks`` complete
``cell`` stable
``errno`` complete
+``error`` stable
``irq`` complete
``memory`` stable
``module`` complete
-``offset_of`` stable
``qdev`` stable
``qom`` stable
``sysbus`` stable
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v2 00/14] rust: bindings for Error
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (13 preceding siblings ...)
2025-05-30 8:03 ` [PATCH 14/14] docs: update Rust module status Paolo Bonzini
@ 2025-06-02 7:49 ` Markus Armbruster
2025-06-02 9:45 ` Paolo Bonzini
2025-06-03 9:35 ` Zhao Liu
15 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2025-06-02 7:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> As explained for v1, the impetus for this series is to remove BqlCell<>
> from HPETState::num_timers. However, it's also an important step for QAPI:
> error propagation is pretty central for example to QMP, and the series
> is also a first example of two-way conversion between C and native-Rust
> structs (i.e. not using bindgen-generated structs or their opaque wrappers).
>
> As an aside, support for NUL-terminated file is now scheduled for
> inclusion in Rust as "panic::Location::file_with_nul()", but it will be
> quite a while before QEMU can use it. For more information, see
> https://github.com/rust-lang/rust/issues/141727.
Doen't apply for me on current master (3e82ddaa8db). What's your base?
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 00/14] rust: bindings for Error
2025-06-02 7:49 ` [PATCH v2 00/14] rust: bindings for Error Markus Armbruster
@ 2025-06-02 9:45 ` Paolo Bonzini
0 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-06-02 9:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-rust
On 6/2/25 09:49, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> As explained for v1, the impetus for this series is to remove BqlCell<>
>> from HPETState::num_timers. However, it's also an important step for QAPI:
>> error propagation is pretty central for example to QMP, and the series
>> is also a first example of two-way conversion between C and native-Rust
>> structs (i.e. not using bindgen-generated structs or their opaque wrappers).
>>
>> As an aside, support for NUL-terminated file is now scheduled for
>> inclusion in Rust as "panic::Location::file_with_nul()", but it will be
>> quite a while before QEMU can use it. For more information, see
>> https://github.com/rust-lang/rust/issues/141727.
>
> Doen't apply for me on current master (3e82ddaa8db). What's your base?
I have a few more patches queued before this series. For something that
applies directly on top of master, you can check branch rust-next at
https://gitlab.com/bonzini/qemu.
You'll also need Meson 1.8.1, which you can install via pip, because
Daniel preferred not to have such a new version required for everyone.
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/14] rust: bindings for Error
2025-05-30 8:02 [PATCH v2 00/14] rust: bindings for Error Paolo Bonzini
` (14 preceding siblings ...)
2025-06-02 7:49 ` [PATCH v2 00/14] rust: bindings for Error Markus Armbruster
@ 2025-06-03 9:35 ` Zhao Liu
15 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-06-03 9:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
On Fri, May 30, 2025 at 10:02:52AM +0200, Paolo Bonzini wrote:
> Date: Fri, 30 May 2025 10:02:52 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH v2 00/14] rust: bindings for Error
> X-Mailer: git-send-email 2.49.0
>
> As explained for v1, the impetus for this series is to remove BqlCell<>
> from HPETState::num_timers. However, it's also an important step for QAPI:
> error propagation is pretty central for example to QMP, and the series
> is also a first example of two-way conversion between C and native-Rust
> structs (i.e. not using bindgen-generated structs or their opaque wrappers).
>
> As an aside, support for NUL-terminated file is now scheduled for
> inclusion in Rust as "panic::Location::file_with_nul()", but it will be
> quite a while before QEMU can use it. For more information, see
> https://github.com/rust-lang/rust/issues/141727.
Apart from Markus's comments, the rest of the code (the specific
implementation of error binding) looks great to me. The interaction with
Foreign crate is quite insightful. I'll go through other patches on the
rust-next branch.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 32+ messages in thread