* [PATCH 01/12] rust: make declaration of dependent crates more consistent
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-27 9:35 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 02/12] subprojects: add the anyhow crate Paolo Bonzini
` (10 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
Crates like "bilge" and "libc" can be shared by more than one directory,
so declare them directly in rust/meson.build. While at it, make their
variable names end with "_rs" and always add a subproject() statement
(as that pinpoints the error better if the subproject is missing and
cannot be downloaded).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/meson.build | 12 +++---------
rust/meson.build | 16 ++++++++++++++++
rust/qemu-api-macros/meson.build | 14 +++-----------
rust/qemu-api/meson.build | 4 +---
4 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/rust/hw/char/pl011/meson.build b/rust/hw/char/pl011/meson.build
index 547cca5a96f..494b6c123cc 100644
--- a/rust/hw/char/pl011/meson.build
+++ b/rust/hw/char/pl011/meson.build
@@ -1,17 +1,11 @@
-subproject('bilge-0.2-rs', required: true)
-subproject('bilge-impl-0.2-rs', required: true)
-
-bilge_dep = dependency('bilge-0.2-rs')
-bilge_impl_dep = dependency('bilge-impl-0.2-rs')
-
_libpl011_rs = static_library(
'pl011',
files('src/lib.rs'),
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
dependencies: [
- bilge_dep,
- bilge_impl_dep,
+ bilge_rs,
+ bilge_impl_rs,
qemu_api,
qemu_api_macros,
],
@@ -21,6 +15,6 @@ rust_devices_ss.add(when: 'CONFIG_X_PL011_RUST', if_true: [declare_dependency(
link_whole: [_libpl011_rs],
# Putting proc macro crates in `dependencies` is necessary for Meson to find
# them when compiling the root per-target static rust lib.
- dependencies: [bilge_impl_dep, qemu_api_macros],
+ dependencies: [bilge_impl_rs, qemu_api_macros],
variables: {'crate': 'pl011'},
)])
diff --git a/rust/meson.build b/rust/meson.build
index 91e52b8fb8e..1f0dcce7d04 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,3 +1,19 @@
+subproject('bilge-0.2-rs', required: true)
+subproject('bilge-impl-0.2-rs', required: true)
+subproject('libc-0.2-rs', required: true)
+
+bilge_rs = dependency('bilge-0.2-rs')
+bilge_impl_rs = dependency('bilge-impl-0.2-rs')
+libc_rs = dependency('libc-0.2-rs')
+
+subproject('proc-macro2-1-rs', required: true)
+subproject('quote-1-rs', required: true)
+subproject('syn-2-rs', required: true)
+
+quote_rs_native = dependency('quote-1-rs', native: true)
+syn_rs_native = dependency('syn-2-rs', native: true)
+proc_macro2_rs_native = dependency('proc-macro2-1-rs', native: true)
+
subdir('qemu-api-macros')
subdir('qemu-api')
diff --git a/rust/qemu-api-macros/meson.build b/rust/qemu-api-macros/meson.build
index 6f94a4bb3c2..8610ce1c844 100644
--- a/rust/qemu-api-macros/meson.build
+++ b/rust/qemu-api-macros/meson.build
@@ -1,11 +1,3 @@
-subproject('proc-macro2-1-rs', required: true)
-subproject('quote-1-rs', required: true)
-subproject('syn-2-rs', required: true)
-
-quote_dep = dependency('quote-1-rs', native: true)
-syn_dep = dependency('syn-2-rs', native: true)
-proc_macro2_dep = dependency('proc-macro2-1-rs', native: true)
-
_qemu_api_macros_rs = rust.proc_macro(
'qemu_api_macros',
files('src/lib.rs'),
@@ -16,9 +8,9 @@ _qemu_api_macros_rs = rust.proc_macro(
'--cfg', 'feature="proc-macro"',
],
dependencies: [
- proc_macro2_dep,
- quote_dep,
- syn_dep,
+ proc_macro2_rs_native,
+ quote_rs_native,
+ syn_rs_native,
],
)
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 1696df705bf..1ea86b8bbf1 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -2,8 +2,6 @@ _qemu_api_cfg = run_command(rustc_args,
'--config-headers', config_host_h, '--features', files('Cargo.toml'),
capture: true, check: true).stdout().strip().splitlines()
-libc_dep = dependency('libc-0.2-rs')
-
# _qemu_api_cfg += ['--cfg', 'feature="allocator"']
if get_option('debug_mutex')
_qemu_api_cfg += ['--cfg', 'feature="debug_cell"']
@@ -37,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_dep, qemu_api_macros],
+ dependencies: [libc_rs, qemu_api_macros],
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 01/12] rust: make declaration of dependent crates more consistent
2025-05-26 14:24 ` [PATCH 01/12] rust: make declaration of dependent crates more consistent Paolo Bonzini
@ 2025-05-27 9:35 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-27 9:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:44PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:44 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 01/12] rust: make declaration of dependent crates more
> consistent
> X-Mailer: git-send-email 2.49.0
>
> Crates like "bilge" and "libc" can be shared by more than one directory,
> so declare them directly in rust/meson.build. While at it, make their
> variable names end with "_rs" and always add a subproject() statement
> (as that pinpoints the error better if the subproject is missing and
> cannot be downloaded).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/meson.build | 12 +++---------
> rust/meson.build | 16 ++++++++++++++++
> rust/qemu-api-macros/meson.build | 14 +++-----------
> rust/qemu-api/meson.build | 4 +---
> 4 files changed, 23 insertions(+), 23 deletions(-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 02/12] subprojects: add the anyhow crate
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
2025-05-26 14:24 ` [PATCH 01/12] rust: make declaration of dependent crates more consistent Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-27 9:45 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 03/12] subprojects: add the foreign crate Paolo Bonzini
` (9 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
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.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/meson.build | 2 ++
subprojects/.gitignore | 1 +
subprojects/anyhow-1.0-rs.wrap | 7 ++++
.../packagefiles/anyhow-1.0-rs/meson.build | 33 +++++++++++++++++++
4 files changed, 43 insertions(+)
create mode 100644 subprojects/anyhow-1.0-rs.wrap
create mode 100644 subprojects/packagefiles/anyhow-1.0-rs/meson.build
diff --git a/rust/meson.build b/rust/meson.build
index 1f0dcce7d04..3e0b6ed4afa 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,7 +1,9 @@
+subproject('anyhow-1.0-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.0-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/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.0-rs.wrap b/subprojects/anyhow-1.0-rs.wrap
new file mode 100644
index 00000000000..53f20b2a315
--- /dev/null
+++ b/subprojects/anyhow-1.0-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.0-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..67278c25af9
--- /dev/null
+++ b/subprojects/packagefiles/anyhow-1.0-rs/meson.build
@@ -0,0 +1,33 @@
+project('anyhow-1.0-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.0-rs', anyhow_dep)
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 02/12] subprojects: add the anyhow crate
2025-05-26 14:24 ` [PATCH 02/12] subprojects: add the anyhow crate Paolo Bonzini
@ 2025-05-27 9:45 ` Zhao Liu
2025-05-27 9:52 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Zhao Liu @ 2025-05-27 9:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:45PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:45 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 02/12] subprojects: add the anyhow crate
> X-Mailer: git-send-email 2.49.0
>
> 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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/meson.build | 2 ++
> subprojects/.gitignore | 1 +
> subprojects/anyhow-1.0-rs.wrap | 7 ++++
> .../packagefiles/anyhow-1.0-rs/meson.build | 33 +++++++++++++++++++
> 4 files changed, 43 insertions(+)
> create mode 100644 subprojects/anyhow-1.0-rs.wrap
> create mode 100644 subprojects/packagefiles/anyhow-1.0-rs/meson.build
Missed to change scripts/archive-source.sh & scripts/make-release?
Otherwise,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/12] subprojects: add the anyhow crate
2025-05-27 9:45 ` Zhao Liu
@ 2025-05-27 9:52 ` Paolo Bonzini
0 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-27 9:52 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, armbru, qemu-rust
On 5/27/25 11:45, Zhao Liu wrote:
> On Mon, May 26, 2025 at 04:24:45PM +0200, Paolo Bonzini wrote:
>> Date: Mon, 26 May 2025 16:24:45 +0200
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> Subject: [PATCH 02/12] subprojects: add the anyhow crate
>> X-Mailer: git-send-email 2.49.0
>>
>> 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.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> rust/meson.build | 2 ++
>> subprojects/.gitignore | 1 +
>> subprojects/anyhow-1.0-rs.wrap | 7 ++++
>> .../packagefiles/anyhow-1.0-rs/meson.build | 33 +++++++++++++++++++
>> 4 files changed, 43 insertions(+)
>> create mode 100644 subprojects/anyhow-1.0-rs.wrap
>> create mode 100644 subprojects/packagefiles/anyhow-1.0-rs/meson.build
>
> Missed to change scripts/archive-source.sh & scripts/make-release?
Yes, also the wrap name should be anyhow-1-rs because only the "0"
version gets an extra component.
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 03/12] subprojects: add the foreign crate
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
2025-05-26 14:24 ` [PATCH 01/12] rust: make declaration of dependent crates more consistent Paolo Bonzini
2025-05-26 14:24 ` [PATCH 02/12] subprojects: add the anyhow crate Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-29 8:13 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 04/12] util/error: expose Error definition to Rust code Paolo Bonzini
` (8 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
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) }
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
subprojects/.gitignore | 1 +
subprojects/foreign-0.2-rs.wrap | 7 +++++
.../packagefiles/foreign-0.2-rs/meson.build | 26 +++++++++++++++++++
3 files changed, 34 insertions(+)
create mode 100644 subprojects/foreign-0.2-rs.wrap
create mode 100644 subprojects/packagefiles/foreign-0.2-rs/meson.build
diff --git a/subprojects/.gitignore b/subprojects/.gitignore
index b9ae507b85a..7b38c4f6281 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.2.0
/itertools-0.11.0
/libc-0.2.162
/proc-macro-error-1.0.4
diff --git a/subprojects/foreign-0.2-rs.wrap b/subprojects/foreign-0.2-rs.wrap
new file mode 100644
index 00000000000..bf3cab9407a
--- /dev/null
+++ b/subprojects/foreign-0.2-rs.wrap
@@ -0,0 +1,7 @@
+[wrap-file]
+directory = foreign-0.2.0
+source_url = https://crates.io/api/v1/crates/foreign/0.2.0/download
+source_filename = foreign-0.2.0.tar.gz
+source_hash = 37dd09e47ea8fd592a333f59fc52b894a97fe966ae9c6b7ef21ae38de6043462
+#method = cargo
+patch_directory = foreign-0.2-rs
diff --git a/subprojects/packagefiles/foreign-0.2-rs/meson.build b/subprojects/packagefiles/foreign-0.2-rs/meson.build
new file mode 100644
index 00000000000..56b835d3ba9
--- /dev/null
+++ b/subprojects/packagefiles/foreign-0.2-rs/meson.build
@@ -0,0 +1,26 @@
+project('foreign-0.2-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.2-rs', foreign_dep)
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 03/12] subprojects: add the foreign crate
2025-05-26 14:24 ` [PATCH 03/12] subprojects: add the foreign crate Paolo Bonzini
@ 2025-05-29 8:13 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 8:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:46PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:46 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/12] subprojects: add the foreign crate
> X-Mailer: git-send-email 2.49.0
>
> 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) }
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> subprojects/.gitignore | 1 +
> subprojects/foreign-0.2-rs.wrap | 7 +++++
It seems you've already released v0.3.0.
> .../packagefiles/foreign-0.2-rs/meson.build | 26 +++++++++++++++++++
> 3 files changed, 34 insertions(+)
> create mode 100644 subprojects/foreign-0.2-rs.wrap
> create mode 100644 subprojects/packagefiles/foreign-0.2-rs/meson.build
I went through foreign crate and it was really helpful (and there are also
missing changes as anyhow crate patch). With nits fixed,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Maybe off topic, it seems the Owned<T> wrapper is very similar to
OwnedPointer<T>. Is it possible to integrate the two? i.e., build
Owned<T: ObjectType> based on OwnedPointer<T: FreeForeign>?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 04/12] util/error: expose Error definition to Rust code
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (2 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 03/12] subprojects: add the foreign crate Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-27 13:33 ` Markus Armbruster
2025-05-26 14:24 ` [PATCH 05/12] util/error: allow non-NUL-terminated err->src Paolo Bonzini
` (7 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, 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 04/12] util/error: expose Error definition to Rust code
2025-05-26 14:24 ` [PATCH 04/12] util/error: expose Error definition to Rust code Paolo Bonzini
@ 2025-05-27 13:33 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-05-27 13:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> 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"
Please move this up to keep the #include sorted (except for
qemu/osdep.h, which is special).
>
> Error *error_abort;
> Error *error_fatal;
Since error.h is always included when error-internal.h is, we could make
error-internal.h include error.h. Matter of taste, up to you.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 05/12] util/error: allow non-NUL-terminated err->src
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (3 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 04/12] util/error: expose Error definition to Rust code Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-27 13:42 ` Markus Armbruster
2025-05-26 14:24 ` [PATCH 06/12] util/error: make func optional Paolo Bonzini
` (6 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, 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 | 8 +++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
index d5c3904adec..6178ce4a63d 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;
+ ssize_t src_len;
int line;
GString *hint;
};
diff --git a/util/error.c b/util/error.c
index e5bcb7c0225..6c1033eaba5 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) {
+ const char *src = err->src;
+ if (err->src_len >= 0) {
+ /* No need to free it, the program will abort very soon... */
+ src = g_strndup(err->src, err->src_len);
+ }
fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
- err->func, err->src, err->line);
+ err->func, src, err->line);
error_report("%s", error_get_pretty(err));
if (err->hint) {
error_printf("%s", err->hint->str);
@@ -67,6 +72,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 05/12] util/error: allow non-NUL-terminated err->src
2025-05-26 14:24 ` [PATCH 05/12] util/error: allow non-NUL-terminated err->src Paolo Bonzini
@ 2025-05-27 13:42 ` Markus Armbruster
2025-05-27 14:34 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2025-05-27 13:42 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.
Can you elaborate on the portability problem? I figure ...
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qapi/error-internal.h | 1 +
> util/error.c | 8 +++++++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/error-internal.h b/include/qapi/error-internal.h
> index d5c3904adec..6178ce4a63d 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;
> + ssize_t src_len;
> int line;
> GString *hint;
> };
> diff --git a/util/error.c b/util/error.c
> index e5bcb7c0225..6c1033eaba5 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) {
> + const char *src = err->src;
> + if (err->src_len >= 0) {
> + /* No need to free it, the program will abort very soon... */
> + src = g_strndup(err->src, err->src_len);
> + }
> fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> - err->func, err->src, err->line);
> + err->func, src, err->line);
... you're avoiding the simpler
fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
err->func, err->src_len, err->src, err->line);
because of it.
(@src_len needs to be int then, and its default value below INT_MAX)
> error_report("%s", error_get_pretty(err));
> if (err->hint) {
> error_printf("%s", err->hint->str);
> @@ -67,6 +72,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;
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 05/12] util/error: allow non-NUL-terminated err->src
2025-05-27 13:42 ` Markus Armbruster
@ 2025-05-27 14:34 ` Paolo Bonzini
2025-05-28 10:44 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-27 14:34 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-rust
On 5/27/25 15:42, Markus Armbruster wrote:
> 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.
>
> Can you elaborate on the portability problem? I figure ...
>
>> {
>> if (errp == &error_abort) {
>> + const char *src = err->src;
>> + if (err->src_len >= 0) {
>> + /* No need to free it, the program will abort very soon... */
>> + src = g_strndup(err->src, err->src_len);
>> + }
>> fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>> - err->func, err->src, err->line);
>> + err->func, src, err->line);
>
> ... you're avoiding the simpler
>
> fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
> err->func, err->src_len, err->src, err->line);
>
> because of it.
I couldn't find anything that says %s is allowed to not be
NUL-terminated if a precision is given. That is, whether something like
this:
char foo[] = {'H', 'e', 'l', 'l', 'o'};
printf("%.5s\n", foo);
is guaranteed to work.
This is opposed to:
1) strnlen
(https://pubs.opengroup.org/onlinepubs/9699919799/functions/strnlen.html),
which is guaranteed to examine no more than the number of bytes given by
the second character;
2) strndup, for which I found at least a clarification at
https://www.austingroupbugs.net/view.php?id=1397.
3) g_strndup, which guarantees that the allocated block is of length n+1
and padded with NULs (though in the case above there will be just one
NUL anyway)
And also, for strndup/g_strndup it would be quite asinine to implement
it using some kind of min(strlen(s), n) but for printf the complexity is
greater so you never know. I erred on the side of caution because
avoiding an allocation before an abort() isn't particularly interesting.
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 05/12] util/error: allow non-NUL-terminated err->src
2025-05-27 14:34 ` Paolo Bonzini
@ 2025-05-28 10:44 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-05-28 10:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 5/27/25 15:42, Markus Armbruster wrote:
>> 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.
>> Can you elaborate on the portability problem? I figure ...
>>
>>> {
>>> if (errp == &error_abort) {
>>> + const char *src = err->src;
>>> + if (err->src_len >= 0) {
>>> + /* No need to free it, the program will abort very soon... */
>>> + src = g_strndup(err->src, err->src_len);
>>> + }
>>> fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>>> - err->func, err->src, err->line);
>>> + err->func, src, err->line);
>> ... you're avoiding the simpler
>> fprintf(stderr, "Unexpected error in %s() at %.*s:%d:\n",
>> err->func, err->src_len, err->src, err->line);
>> because of it.
>
> I couldn't find anything that says %s is allowed to not be NUL-terminated if a precision is given. That is, whether something like this:
>
> char foo[] = {'H', 'e', 'l', 'l', 'o'};
> printf("%.5s\n", foo);
>
> is guaranteed to work.
From ISO/IEC 9899:1999 §7.19.6.1 "The fprintf function":
[#8] The conversion specifiers and their meanings are:
[...]
s If no l length modifier is present, the argument
shall be a pointer to the initial element of an
array of character type.237) Characters from the
array are written up to (but not including) the
terminating null character. If the precision is
specified, no more than that many bytes are written.
--> If the precision is not specified or is greater than
--> the size of the array, the array shall contain a
--> null character.
____________________
237No special provisions are made for multibyte characters.
This clearly implies that the string need not be null-terminated when a
suitable precision is specified. Which it is here.
> This is opposed to:
>
> 1) strnlen (https://pubs.opengroup.org/onlinepubs/9699919799/functions/strnlen.html), which is guaranteed to examine no more than the number of bytes given by the second character;
>
> 2) strndup, for which I found at least a clarification at https://www.austingroupbugs.net/view.php?id=1397.
>
> 3) g_strndup, which guarantees that the allocated block is of length n+1 and padded with NULs (though in the case above there will be just one NUL anyway)
>
> And also, for strndup/g_strndup it would be quite asinine to implement it using some kind of min(strlen(s), n) but for printf the complexity is greater so you never know. I erred on the side of caution because avoiding an allocation before an abort() isn't particularly interesting.
Keeping the code simple is always interesting, though :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 06/12] util/error: make func optional
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (4 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 05/12] util/error: allow non-NUL-terminated err->src Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-28 8:20 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 07/12] qemu-api: add bindings to Error Paolo Bonzini
` (5 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
The function name is not available in Rust, so make it optional.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
util/error.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/util/error.c b/util/error.c
index 6c1033eaba5..b977007faaf 100644
--- a/util/error.c
+++ b/util/error.c
@@ -29,8 +29,12 @@ static void error_handle(Error **errp, Error *err)
/* No need to free it, the program will abort very soon... */
src = g_strndup(err->src, err->src_len);
}
- fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
- err->func, src, err->line);
+ if (err->func) {
+ fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
+ err->func, src, err->line);
+ } else {
+ fprintf(stderr, "Unexpected error at %s:%d:\n", 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 06/12] util/error: make func optional
2025-05-26 14:24 ` [PATCH 06/12] util/error: make func optional Paolo Bonzini
@ 2025-05-28 8:20 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-28 8:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:49PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:49 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 06/12] util/error: make func optional
> X-Mailer: git-send-email 2.49.0
>
> The function name is not available in Rust, so make it optional.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> util/error.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
panic::Location does not provide function name information. Although
there are macros that could print function names [*] (as I'm sure you've
noticed :) ), that way - printing the information based on some macros -
would definitely require some wrapping or modification of Err().
Comparing with that, current implementation looks better in general.
[*]: https://stackoverflow.com/a/63904992/24336517
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 07/12] qemu-api: add bindings to Error
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (5 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 06/12] util/error: make func optional Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-28 9:49 ` Markus Armbruster
2025-05-26 14:24 ` [PATCH 08/12] rust: qdev: support returning errors from realize Paolo Bonzini
` (4 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, 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/meson.build | 2 +
rust/qemu-api/Cargo.toml | 2 +
rust/qemu-api/meson.build | 3 +-
rust/qemu-api/src/error.rs | 273 +++++++++++++++++++++++++++++++++++++
rust/qemu-api/src/lib.rs | 3 +
7 files changed, 300 insertions(+), 1 deletion(-)
create mode 100644 rust/qemu-api/src/error.rs
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index 13d580c693b..37fd10064f9 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -2,6 +2,12 @@
# It is not intended for manual editing.
version = 3
+[[package]]
+name = "anyhow"
+version = "1.0.98"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487"
+
[[package]]
name = "arbitrary-int"
version = "1.2.7"
@@ -37,6 +43,15 @@ version = "1.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
+[[package]]
+name = "foreign"
+version = "0.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "37dd09e47ea8fd592a333f59fc52b894a97fe966ae9c6b7ef21ae38de6043462"
+dependencies = [
+ "libc",
+]
+
[[package]]
name = "hpet"
version = "0.1.0"
@@ -106,6 +121,8 @@ dependencies = [
name = "qemu_api"
version = "0.1.0"
dependencies = [
+ "anyhow",
+ "foreign",
"libc",
"qemu_api_macros",
]
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index d9faeecb10b..2726b1f72e3 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -67,6 +67,7 @@ multiple_crate_versions = "deny"
mut_mut = "deny"
needless_bitwise_bool = "deny"
needless_pass_by_ref_mut = "deny"
+needless_update = "deny"
no_effect_underscore_binding = "deny"
option_option = "deny"
or_fun_call = "deny"
diff --git a/rust/meson.build b/rust/meson.build
index 3e0b6ed4afa..9eb69174dea 100644
--- a/rust/meson.build
+++ b/rust/meson.build
@@ -1,11 +1,13 @@
subproject('anyhow-1.0-rs', required: true)
subproject('bilge-0.2-rs', required: true)
subproject('bilge-impl-0.2-rs', required: true)
+subproject('foreign-0.2-rs', required: true)
subproject('libc-0.2-rs', required: true)
anyhow_rs = dependency('anyhow-1.0-rs')
bilge_rs = dependency('bilge-0.2-rs')
bilge_impl_rs = dependency('bilge-impl-0.2-rs')
+foreign_rs = dependency('foreign-0.2-rs')
libc_rs = dependency('libc-0.2-rs')
subproject('proc-macro2-1-rs', required: true)
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index c96cf50e7a1..ca6b81db10a 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -15,7 +15,9 @@ rust-version.workspace = true
[dependencies]
qemu_api_macros = { path = "../qemu-api-macros" }
+anyhow = "~1.0"
libc = "0.2.162"
+foreign = "~0.2"
[features]
default = ["debug_cell"]
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 1ea86b8bbf1..11cbd6d375a 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -19,6 +19,7 @@ _qemu_api_rs = static_library(
'src/cell.rs',
'src/chardev.rs',
'src/errno.rs',
+ 'src/error.rs',
'src/irq.rs',
'src/memory.rs',
'src/module.rs',
@@ -35,7 +36,7 @@ _qemu_api_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: _qemu_api_cfg,
- dependencies: [libc_rs, qemu_api_macros],
+ dependencies: [anyhow_rs, foreign_rs, libc_rs, qemu_api_macros],
)
rust.test('rust-qemu-api-tests', _qemu_api_rs,
diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
new file mode 100644
index 00000000000..f08fed81028
--- /dev/null
+++ b/rust/qemu-api/src/error.rs
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! Error class for QEMU Rust code
+//!
+//! @author Paolo Bonzini
+
+use std::{
+ borrow::Cow,
+ ffi::{c_char, c_int, c_void, CStr},
+ fmt::{self, Display},
+ panic, ptr,
+};
+
+use foreign::{prelude::*, OwnedPointer};
+
+use crate::{
+ bindings,
+ bindings::{error_free, error_get_pretty},
+};
+
+pub type Result<T> = std::result::Result<T, Error>;
+
+#[derive(Debug, Default)]
+pub struct Error {
+ 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("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)),
+ file: location.file(),
+ line: location.line(),
+ ..Default::default()
+ }
+ }
+}
+
+impl From<&'static str> for Error {
+ #[track_caller]
+ fn from(msg: &'static str) -> Self {
+ let location = panic::Location::caller();
+ Error {
+ msg: Some(Cow::Borrowed(msg)),
+ file: location.file(),
+ line: location.line(),
+ ..Default::default()
+ }
+ }
+}
+
+impl From<anyhow::Error> for Error {
+ #[track_caller]
+ fn from(error: anyhow::Error) -> Self {
+ let location = panic::Location::caller();
+ Error {
+ cause: Some(error),
+ file: location.file(),
+ line: location.line(),
+ ..Default::default()
+ }
+ }
+}
+
+impl Error {
+ /// Create a new error, prepending `msg` to the
+ /// description of `cause`
+ #[track_caller]
+ pub fn with_error<E: std::error::Error + Send + Sync + 'static>(msg: &'static str, cause: E) -> Self {
+ let location = panic::Location::caller();
+ Error {
+ msg: Some(Cow::Borrowed(msg)),
+ 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 valid; typically it is received from C code
+ 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 valid; typically it is received from C code
+ #[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 valid; typically it is received from C code
+ pub unsafe fn ok_or_propagate<T>(
+ result: Result<T>,
+ errp: *mut *mut bindings::Error,
+ ) -> Option<T> {
+ let Err(err) = result else {
+ return result.ok();
+ };
+
+ // SAFETY: caller guarantees errp is valid
+ unsafe {
+ err.propagate(errp);
+ }
+ None
+ }
+
+ /// Equivalent of the C function `error_propagate`. Fill `*errp`
+ /// with the information container in `result` if `errp` is not NULL;
+ /// then consume it.
+ ///
+ /// # Safety
+ ///
+ /// `errp` must be valid; typically it is received from C code
+ 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.
+ ///
+ /// # Safety
+ ///
+ /// `c_error` must be valid; typically it has been filled by 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.
+ ///
+ /// # Safety
+ ///
+ /// `c_error` must be valid; typically it has been filled by a C
+ /// function.
+ pub unsafe fn err_or_else<T, F: FnOnce() -> T>(
+ c_error: *mut bindings::Error,
+ f: F,
+ ) -> Result<T> {
+ // SAFETY: caller guarantees c_error is valid
+ let err = unsafe { Option::<Self>::from_foreign(c_error) };
+ match err {
+ None => Ok(f()),
+ Some(err) => Err(err),
+ }
+ }
+}
+
+impl FreeForeign for Error {
+ type Foreign = bindings::Error;
+
+ unsafe fn free_foreign(p: *mut bindings::Error) {
+ // SAFETY: caller guarantees p is valid
+ unsafe {
+ 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 isize,
+ src: self.file.as_ptr().cast::<c_char>(),
+ line: self.line as c_int,
+ func: ptr::null_mut(),
+ hint: ptr::null_mut(),
+ };
+ OwnedPointer::new(err)
+ }
+ }
+}
+
+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_get_pretty(error)),
+ 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 07/12] qemu-api: add bindings to Error
2025-05-26 14:24 ` [PATCH 07/12] qemu-api: add bindings to Error Paolo Bonzini
@ 2025-05-28 9:49 ` Markus Armbruster
2025-05-28 10:45 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2025-05-28 9:49 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>
My Rust-fu is far to weak for an actual review. But let me try to
stumble through your patch. I'll pretend to explain the Rust code to a
noob like myself. Please correct my mistakes.
> ---
> rust/Cargo.lock | 17 +++
> rust/Cargo.toml | 1 +
> rust/meson.build | 2 +
> rust/qemu-api/Cargo.toml | 2 +
> rust/qemu-api/meson.build | 3 +-
> rust/qemu-api/src/error.rs | 273 +++++++++++++++++++++++++++++++++++++
> rust/qemu-api/src/lib.rs | 3 +
> 7 files changed, 300 insertions(+), 1 deletion(-)
> create mode 100644 rust/qemu-api/src/error.rs
>
> diff --git a/rust/Cargo.lock b/rust/Cargo.lock
> index 13d580c693b..37fd10064f9 100644
> --- a/rust/Cargo.lock
> +++ b/rust/Cargo.lock
> @@ -2,6 +2,12 @@
> # It is not intended for manual editing.
> version = 3
>
> +[[package]]
> +name = "anyhow"
> +version = "1.0.98"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487"
> +
> [[package]]
> name = "arbitrary-int"
> version = "1.2.7"
> @@ -37,6 +43,15 @@ version = "1.12.0"
> source = "registry+https://github.com/rust-lang/crates.io-index"
> checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
>
> +[[package]]
> +name = "foreign"
> +version = "0.2.0"
> +source = "registry+https://github.com/rust-lang/crates.io-index"
> +checksum = "37dd09e47ea8fd592a333f59fc52b894a97fe966ae9c6b7ef21ae38de6043462"
> +dependencies = [
> + "libc",
> +]
> +
> [[package]]
> name = "hpet"
> version = "0.1.0"
> @@ -106,6 +121,8 @@ dependencies = [
> name = "qemu_api"
> version = "0.1.0"
> dependencies = [
> + "anyhow",
> + "foreign",
> "libc",
> "qemu_api_macros",
> ]
> diff --git a/rust/Cargo.toml b/rust/Cargo.toml
> index d9faeecb10b..2726b1f72e3 100644
> --- a/rust/Cargo.toml
> +++ b/rust/Cargo.toml
> @@ -67,6 +67,7 @@ multiple_crate_versions = "deny"
> mut_mut = "deny"
> needless_bitwise_bool = "deny"
> needless_pass_by_ref_mut = "deny"
> +needless_update = "deny"
> no_effect_underscore_binding = "deny"
> option_option = "deny"
> or_fun_call = "deny"
> diff --git a/rust/meson.build b/rust/meson.build
> index 3e0b6ed4afa..9eb69174dea 100644
> --- a/rust/meson.build
> +++ b/rust/meson.build
> @@ -1,11 +1,13 @@
> subproject('anyhow-1.0-rs', required: true)
> subproject('bilge-0.2-rs', required: true)
> subproject('bilge-impl-0.2-rs', required: true)
> +subproject('foreign-0.2-rs', required: true)
> subproject('libc-0.2-rs', required: true)
>
> anyhow_rs = dependency('anyhow-1.0-rs')
> bilge_rs = dependency('bilge-0.2-rs')
> bilge_impl_rs = dependency('bilge-impl-0.2-rs')
> +foreign_rs = dependency('foreign-0.2-rs')
> libc_rs = dependency('libc-0.2-rs')
>
> subproject('proc-macro2-1-rs', required: true)
> diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
> index c96cf50e7a1..ca6b81db10a 100644
> --- a/rust/qemu-api/Cargo.toml
> +++ b/rust/qemu-api/Cargo.toml
> @@ -15,7 +15,9 @@ rust-version.workspace = true
>
> [dependencies]
> qemu_api_macros = { path = "../qemu-api-macros" }
> +anyhow = "~1.0"
> libc = "0.2.162"
> +foreign = "~0.2"
>
> [features]
> default = ["debug_cell"]
> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index 1ea86b8bbf1..11cbd6d375a 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -19,6 +19,7 @@ _qemu_api_rs = static_library(
> 'src/cell.rs',
> 'src/chardev.rs',
> 'src/errno.rs',
> + 'src/error.rs',
> 'src/irq.rs',
> 'src/memory.rs',
> 'src/module.rs',
> @@ -35,7 +36,7 @@ _qemu_api_rs = static_library(
> override_options: ['rust_std=2021', 'build.rust_std=2021'],
> rust_abi: 'rust',
> rust_args: _qemu_api_cfg,
> - dependencies: [libc_rs, qemu_api_macros],
> + dependencies: [anyhow_rs, foreign_rs, libc_rs, qemu_api_macros],
> )
>
> rust.test('rust-qemu-api-tests', _qemu_api_rs,
Mere plumbing so far. I assume I don't need to understand it at this
point :)
> diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> new file mode 100644
> index 00000000000..f08fed81028
> --- /dev/null
> +++ b/rust/qemu-api/src/error.rs
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +//! Error class for QEMU Rust code
> +//!
> +//! @author Paolo Bonzini
> +
> +use std::{
> + borrow::Cow,
> + ffi::{c_char, c_int, c_void, CStr},
> + fmt::{self, Display},
> + panic, ptr,
> +};
> +
> +use foreign::{prelude::*, OwnedPointer};
> +
> +use crate::{
> + bindings,
> + bindings::{error_free, error_get_pretty},
> +};
> +
> +pub type Result<T> = std::result::Result<T, Error>;
> +
> +#[derive(Debug, Default)]
> +pub struct Error {
We're defining a custom error type Error for use with Result<>. This
requires implementing a number of traits. For trait Debug, we take the
auto-generated solution here. Other traits are implemented below, in
particular Display.
I don't yet understand the role of trait Default.
Does the name Error risk confusion with std::error::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,
> +}
This is the Rust equivalent to C struct Error. High-level differences:
* No @err_class. It's almost always ERROR_CLASS_GENERIC_ERROR in C
nowadays. You're hardcoding that value in Rust for now. Good.
* @cause, optional. This is the bridge to idiomatic Rust error types.
* @msg is optional. This is so you can wrap a @cause without having to
add some useless message.
Is having Errors with neither @msg nor @cause a good idea?
> +
> +impl std::error::Error for Error {
> + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
Provide the optional method source() that returns the lower-level source
of the error, if any. I guess the system will put it to good use :)
> + self.cause.as_ref().map(AsRef::as_ref)
It returns the std::error::Error that's wrapped in @cause, if any.
Reasonable.
> + }
> +
> + #[allow(deprecated)]
Needed for what? Oh, method description() is deprecated since 1.42.0:
"use the Display impl or to_string()". I figure we need it because the
new way doesn't work with our oldest supported Rust version. Could
perhaps use a comment to help future garbage collectors.
> + fn description(&self) -> &str {
> + self.msg
> + .as_deref()
> + .or_else(|| self.cause.as_deref().map(std::error::Error::description))
> + .unwrap_or("error")
This gives us @msg, or else @cause, or else "error".
Is it a good idea to ignore @cause when we have @msg?
Above, I doubted the wisdom of having Errors with neither @msg nor
@cause. description() returns the rather unhelpful "error" then.
> + }
> +}
> +
> +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(())
> + }
This combines @msg and @cause:
* If we have both: msg:cause
* If we have just msg: msg
* If we have just cause: cause
* If we have neither: "unknown error"
Differs from description(). Why?
> +}
> +
> +impl From<String> for Error {
> + #[track_caller]
This enables use of panic::Location::caller().file() and .line() right
below. Neat!
> + fn from(msg: String) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + msg: Some(Cow::Owned(msg)),
> + file: location.file(),
> + line: location.line(),
> + ..Default::default()
I don't understand this line, I'm afraid.
> + }
> + }
This creates an Error from an error message string, recording the spot
where it's done. The resulting Error has @msg, but not @cause.
> +}
> +
> +impl From<&'static str> for Error {
> + #[track_caller]
> + fn from(msg: &'static str) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + msg: Some(Cow::Borrowed(msg)),
> + file: location.file(),
> + line: location.line(),
> + ..Default::default()
> + }
> + }
> +}
Same for another string type.
> +
> +impl From<anyhow::Error> for Error {
> + #[track_caller]
> + fn from(error: anyhow::Error) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + cause: Some(error),
> + file: location.file(),
> + line: location.line(),
> + ..Default::default()
> + }
> + }
> +}
This creates an Error from an anyhow::Error, recording the spot where
it's done. The resulting Error has @cause, but not @msg.
> +
> +impl Error {
> + /// Create a new error, prepending `msg` to the
> + /// description of `cause`
> + #[track_caller]
> + pub fn with_error<E: std::error::Error + Send + Sync + 'static>(msg: &'static str, cause: E) -> Self {
> + let location = panic::Location::caller();
> + Error {
> + msg: Some(Cow::Borrowed(msg)),
> + cause: Some(cause.into()),
> + file: location.file(),
> + line: location.line(),
> + }
> + }
This creates an Error from an error message string and an anyhow::Error,
recording the spot where it's done. The resulting Error has both @msg
and @cause.
Is there a way to create an Error with neither @msg nor @cause?
> +
> + /// 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 valid; typically it is received from C code
> + 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()
> + }
Note: I jumped down to ok_or_propagate(), then returned here.
Note: I'm ignoring everything about safety in this first pass.
Alright, this is "return true on success (nothing propagated), return
false and propagate on error". Okay.
> +
> + /// 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 valid; typically it is received from C code
> + #[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()
> + }
And this is "return result as foreign pointer on success (nothing
propagated), return NULL and propagate on error". Okay.
> +
> + /// 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 valid; typically it is received from C code
> + pub unsafe fn ok_or_propagate<T>(
> + result: Result<T>,
@result is either a success value of type T, or an error value.
> + errp: *mut *mut bindings::Error,
> + ) -> Option<T> {
> + let Err(err) = result else {
If @result is an Error, put the error value in @err.
> + return result.ok();
Else succeed and return the success value.
> + };
> +
> + // SAFETY: caller guarantees errp is valid
> + unsafe {
> + err.propagate(errp);
> + }
> + None
@result is an Error. Propagate it, and return None.
This is indeed like self.ok() with propagation added.
> + }
> +
> + /// Equivalent of the C function `error_propagate`. Fill `*errp`
> + /// with the information container in `result` if `errp` is not NULL;
> + /// then consume it.
Note error_propagate() has the arguments in the opposite order.
> + ///
> + /// # Safety
> + ///
> + /// `errp` must be valid; typically it is received from C code
What does "valid" mean exactly?
C error_propagate()'s function comment:
/*
* Propagate error object (if any) from @local_err to @dst_errp.
* If @local_err is NULL, do nothing (because there's nothing to
* propagate).
* Else, 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.
* On return, @local_err is invalid.
* Please use ERRP_GUARD() instead when possible.
* Please don't error_propagate(&error_fatal, ...), use
* error_report_err() and exit(), because that's more obvious.
*/
> + pub unsafe fn propagate(self, errp: *mut *mut bindings::Error) {
The function comment mentions `result`. The function signature doesn't
have it. Do you mean self?
If yes, what does it mean to consume self?
Ignored for now: how exactly the code below consumes.
> + if errp.is_null() {
Errors are being ignored.
> + return;
> + }
> +
> + let err = self.clone_to_foreign_ptr();
> +
> + // SAFETY: caller guarantees errp is valid
> + unsafe {
> + errp.write(err);
I *guess* this writes the foreign pointer we just obtained to @errp.
> + }
> + }
Brief switch to the design level...
In C, you're almost always better off with ERRP_GUARD(). Would you like
me to elaborate?
You still have to use error_propagate() to accumulate multiple errors so
that the first one wins. That's commonly a dumb idea. Should we avoid
this pattern in Rust?
> +
> + /// Convert a C `Error*` into a Rust `Result`, using
> + /// `Ok(())` if `c_error` is NULL.
> + ///
> + /// # Safety
> + ///
> + /// `c_error` must be valid; typically it has been filled by a C
> + /// function.
What does "valid" mean exactly?
> + pub unsafe fn err_or_unit(c_error: *mut bindings::Error) -> Result<()> {
> + // SAFETY: caller guarantees c_error is valid
> + unsafe { Self::err_or_else(c_error, || ()) }
> + }
> +
> + /// Convert a C `Error*` into a Rust `Result`, calling `f()` to
> + /// obtain an `Ok` value if `c_error` is NULL.
> + ///
> + /// # Safety
> + ///
> + /// `c_error` must be valid; typically it has been filled by a C
> + /// function.
> + pub unsafe fn err_or_else<T, F: FnOnce() -> T>(
> + c_error: *mut bindings::Error,
> + f: F,
> + ) -> Result<T> {
> + // SAFETY: caller guarantees c_error is valid
> + let err = unsafe { Option::<Self>::from_foreign(c_error) };
> + match err {
> + None => Ok(f()),
> + Some(err) => Err(err),
> + }
> + }
> +}
Getting tired...
> +
> +impl FreeForeign for Error {
> + type Foreign = bindings::Error;
> +
> + unsafe fn free_foreign(p: *mut bindings::Error) {
> + // SAFETY: caller guarantees p is valid
> + unsafe {
> + 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 isize,
> + src: self.file.as_ptr().cast::<c_char>(),
> + line: self.line as c_int,
> + func: ptr::null_mut(),
> + hint: ptr::null_mut(),
> + };
> + OwnedPointer::new(err)
> + }
> + }
> +}
Plausible to this Rust ignoramus :)
> +
> +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_get_pretty(error)),
> + cause: None,
> + file: file.unwrap(),
> + line: error.line as u32,
> + }
> + }
> + }
> +}
Likewise.
> 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 07/12] qemu-api: add bindings to Error
2025-05-28 9:49 ` Markus Armbruster
@ 2025-05-28 10:45 ` Paolo Bonzini
2025-05-28 13:12 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-28 10:45 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-rust
On Wed, May 28, 2025 at 11:49 AM Markus Armbruster <armbru@redhat.com> wrote:
> > diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
> > new file mode 100644
> > index 00000000000..f08fed81028
> > --- /dev/null
> > +++ b/rust/qemu-api/src/error.rs
> > @@ -0,0 +1,273 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +//! Error class for QEMU Rust code
> > +//!
> > +//! @author Paolo Bonzini
> > +
> > +use std::{
> > + borrow::Cow,
> > + ffi::{c_char, c_int, c_void, CStr},
> > + fmt::{self, Display},
> > + panic, ptr,
> > +};
> > +
> > +use foreign::{prelude::*, OwnedPointer};
> > +
> > +use crate::{
> > + bindings,
> > + bindings::{error_free, error_get_pretty},
> > +};
> > +
> > +pub type Result<T> = std::result::Result<T, Error>;
> > +
> > +#[derive(Debug, Default)]
> > +pub struct Error {
>
> We're defining a custom error type Error for use with Result<>. This
> requires implementing a number of traits. For trait Debug, we take the
> auto-generated solution here. Other traits are implemented below, in
> particular Display.
>
> I don't yet understand the role of trait Default.
It defines an Error without any frills attached. It is used below but
on the other hand it results in those "unknown error"s that you
rightly despised.
> Does the name Error risk confusion with std::error::Error?
Maybe, but as you can see from e.g. ayhow::Error it's fairly common to
have each crate or module define its own Error type. In the end you
always convert them to another type with "?" or ".into()".
> This is the Rust equivalent to C struct Error. High-level differences:
>
> * No @err_class. It's almost always ERROR_CLASS_GENERIC_ERROR in C
> nowadays. You're hardcoding that value in Rust for now. Good.
>
> * @cause, optional. This is the bridge to idiomatic Rust error types.
>
> * @msg is optional. This is so you can wrap a @cause without having to
> add some useless message.
>
> Is having Errors with neither @msg nor @cause a good idea?
It makes for slightly nicer code, and avoids having to worry about
panics from ".unwrap()" in error handling code (where panicking
probably won't help much). Otherwise it's probably not a good idea,
but also not something that people will use since (see later patches)
it's easier to return a decent error message than an empty Error.
> Needed for what? Oh, method description() is deprecated since 1.42.0:
> "use the Display impl or to_string()". I figure we need it because the
> new way doesn't work with our oldest supported Rust version. Could
> perhaps use a comment to help future garbage collectors.
>
> > + fn description(&self) -> &str {
> > + self.msg
> > + .as_deref()
> > + .or_else(|| self.cause.as_deref().map(std::error::Error::description))
> > + .unwrap_or("error")
>
> This gives us @msg, or else @cause, or else "error".
>
> Is it a good idea to ignore @cause when we have @msg?
> > +impl Display for Error {
> > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
> > ...
> > + }
>
> This combines @msg and @cause:
>
> Differs from description(). Why?
Because description() cannot build a dynamically-allocated string, it
must return something that is already available in the Error. That
limitation is probably why it was deprecated.
Since it's deprecated we can expect that it won't be used and not
worry too much.
> > + fn from(msg: String) -> Self {
> > + let location = panic::Location::caller();
> > + Error {
> > + msg: Some(Cow::Owned(msg)),
> > + file: location.file(),
> > + line: location.line(),
> > + ..Default::default()
>
> I don't understand this line, I'm afraid.
It says "every other field comes from "..Default::default()". I can
replace it with "cause: None", and likewise below.
> > +}
> > +
> > +impl From<&'static str> for Error {
> > + #[track_caller]
> > + fn from(msg: &'static str) -> Self {
> > + let location = panic::Location::caller();
> > + Error {
> > + msg: Some(Cow::Borrowed(msg)),
> > + file: location.file(),
> > + line: location.line(),
> > + ..Default::default()
> > + }
> > + }
> > +}
>
> Same for another string type.
Yes, this is for strings that are not allocated and are always
valid---such as string constants.
> Is there a way to create an Error with neither @msg nor @cause?
Yes, Default::default()
> > + errp: *mut *mut bindings::Error,
> > + ) -> Option<T> {
> > + let Err(err) = result else {
> > + return result.ok();
> > + };
> > +
> > + // SAFETY: caller guarantees errp is valid
> > + unsafe {
> > + err.propagate(errp);
> > + }
> > + None
>
> @result is an Error. Propagate it, and return None.
>
> This is indeed like self.ok() with propagation added.
Alternatively:
result
.map_err(|err| unsafe { err.propagate(errp) })
.ok()
Shorter, but the functional style can be off putting. What do you prefer?
> > + }
> > +
> > + /// Equivalent of the C function `error_propagate`. Fill `*errp`
> > + /// with the information container in `result` if `errp` is not NULL;
> > + /// then consume it.
>
> Note error_propagate() has the arguments in the opposite order.
Yeah, here "self" must be first so that you use it as a method.
Though, s/result/self/ as you noted.
> > + /// # Safety
> > + ///
> > + /// `errp` must be valid; typically it is received from C code
>
> What does "valid" mean exactly?
I will copy from `error_propagate` in v2.
> Brief switch to the design level...
>
> In C, you're almost always better off with ERRP_GUARD(). Would you like
> me to elaborate?
>
> You still have to use error_propagate() to accumulate multiple errors so
> that the first one wins. That's commonly a dumb idea. Should we avoid
> this pattern in Rust?
In Rust there are three kinds of functions that use errors. Two are in qemu_api:
(1) bridges from C to Rust function pointers. They receive a Result<>
from Rust functions and use error_propagate() (probably via functions
such as ok_or_propagate, bool_or_propagate, etc.) to prepare a C
return value.
(2) bridges from Rust to C functions. They pass an Error** to the C
function and use err_or_else() or err_or_unit() to prepare a Rust
return value
Functions of kind (1) are like functions in C that do a single call
and just pass down errp, for example user_creatable_add_qapi(). These
do not need ERRP_GUARD() because:
* the conversion from Result<> to C is pretty much a necessity, and
it's done with functions that guarantee the propagation
* the conversion function *consumes* the Result<>, guaranteeing that
you do not propagate more than once with tragic results
Function of kind (2) do not need ERRP_GUARD() because they do not take
an Error** at all. They pass one down to C, but they return a
Result<>.
The third kind is Rust functions that are called from (1) and that
themselves call (2). These are where ERRP_GUARD() would be used in C,
but in Rust these do not see Error** at all. The "?" operator has the
same effect as ERRP_GUARD(), i.e. it handles passing the error from
called function to return value. What in C would be
ERRP_GUARD();
if (!func_that_returns_bool(...)) { return; }
In Rust is
func_that_returns_result(...)?;
which is a bit disconcerting in the beginning but grows on you.
(Generally I find that to be the experience with Rust. It's downright
weird, but unlike C++ the good parts outweigh the weirdness).
> > + pub unsafe fn err_or_unit(c_error: *mut bindings::Error) -> Result<()> {
> > + // SAFETY: caller guarantees c_error is valid
> > + unsafe { Self::err_or_else(c_error, || ()) }
> > + }
> > +
> > + /// Convert a C `Error*` into a Rust `Result`, calling `f()` to
> > + /// obtain an `Ok` value if `c_error` is NULL.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `c_error` must be valid; typically it has been filled by a C
> > + /// function.
> > + pub unsafe fn err_or_else<T, F: FnOnce() -> T>(
> > + c_error: *mut bindings::Error,
> > + f: F,
> > + ) -> Result<T> {
> > + // SAFETY: caller guarantees c_error is valid
> > + let err = unsafe { Option::<Self>::from_foreign(c_error) };
> > + match err {
> > + None => Ok(f()),
> > + Some(err) => Err(err),
> > + }
> > + }
> > +}
>
> Getting tired...
No problem. While this is kinda important, it's not used yet. The
clients would look like this:
fn type_get_or_load_by_name(name: &str) -> Result<&TypeImpl> {
unsafe {
let err: *mut bindings::Error = ptr::null_mut();
let typ: *mut TypeImpl = bindings::type_get_or_load_by_name(
name.clone_to_foreign().as_ptr(), &mut err);
// on success, "typ" can be accessed safely
// on failure, turn the Error* into a qemu_api::Error and free it
Result::err_or_else(err, || &*typ)
}
This is why I need to write tests.
> > +impl CloneToForeign for Error {
> > + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
> > + // SAFETY: all arguments are controlled by this function
> > + unsafe {
> > + let err: *mut c_void = libc::malloc(std::mem::size_of::<bindings::Error>());
> > + let err: &mut bindings::Error = &mut *err.cast();
> > + *err = bindings::Error {
> > + msg: format!("{self}").clone_to_foreign_ptr(),
> > + err_class: bindings::ERROR_CLASS_GENERIC_ERROR,
> > + src_len: self.file.len() as isize,
> > + src: self.file.as_ptr().cast::<c_char>(),
> > + line: self.line as c_int,
> > + func: ptr::null_mut(),
> > + hint: ptr::null_mut(),
> > + };
> > + OwnedPointer::new(err)
> > + }
> > + }
> > +}
>
> Plausible to this Rust ignoramus :)
Good, since these *Foreign traits are not part of the standard
library, but rather something that I concocted and published outside
QEMU. If you had no problems understanding how they were used (e.g.
stuff like into_native or clone_to_foreign_ptr), that is good.
Paolo
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 07/12] qemu-api: add bindings to Error
2025-05-28 10:45 ` Paolo Bonzini
@ 2025-05-28 13:12 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-05-28 13:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> On Wed, May 28, 2025 at 11:49 AM Markus Armbruster <armbru@redhat.com> wrote:
>> > diff --git a/rust/qemu-api/src/error.rs b/rust/qemu-api/src/error.rs
>> > new file mode 100644
>> > index 00000000000..f08fed81028
>> > --- /dev/null
>> > +++ b/rust/qemu-api/src/error.rs
>> > @@ -0,0 +1,273 @@
>> > +// SPDX-License-Identifier: GPL-2.0-or-later
>> > +
>> > +//! Error class for QEMU Rust code
>> > +//!
>> > +//! @author Paolo Bonzini
>> > +
>> > +use std::{
>> > + borrow::Cow,
>> > + ffi::{c_char, c_int, c_void, CStr},
>> > + fmt::{self, Display},
>> > + panic, ptr,
>> > +};
>> > +
>> > +use foreign::{prelude::*, OwnedPointer};
>> > +
>> > +use crate::{
>> > + bindings,
>> > + bindings::{error_free, error_get_pretty},
>> > +};
>> > +
>> > +pub type Result<T> = std::result::Result<T, Error>;
>> > +
>> > +#[derive(Debug, Default)]
>> > +pub struct Error {
>>
>> We're defining a custom error type Error for use with Result<>. This
>> requires implementing a number of traits. For trait Debug, we take the
>> auto-generated solution here. Other traits are implemented below, in
>> particular Display.
>>
>> I don't yet understand the role of trait Default.
>
> It defines an Error without any frills attached. It is used below but
> on the other hand it results in those "unknown error"s that you
> rightly despised.
I think I understand how this works now.
>> Does the name Error risk confusion with std::error::Error?
>
> Maybe, but as you can see from e.g. ayhow::Error it's fairly common to
> have each crate or module define its own Error type. In the end you
> always convert them to another type with "?" or ".into()".
Fair enough.
>> This is the Rust equivalent to C struct Error. High-level differences:
>>
>> * No @err_class. It's almost always ERROR_CLASS_GENERIC_ERROR in C
>> nowadays. You're hardcoding that value in Rust for now. Good.
>>
>> * @cause, optional. This is the bridge to idiomatic Rust error types.
>>
>> * @msg is optional. This is so you can wrap a @cause without having to
>> add some useless message.
>>
>> Is having Errors with neither @msg nor @cause a good idea?
>
> It makes for slightly nicer code, and avoids having to worry about
> panics from ".unwrap()" in error handling code (where panicking
> probably won't help much). Otherwise it's probably not a good idea,
> but also not something that people will use since (see later patches)
> it's easier to return a decent error message than an empty Error.
I accept that making "need at least one of @msg, @cause" an invariant
could complicate the code a bit.
Having a way to create an Error with neither bothers me, even when it's
obscure. Making an interface hard to misuse is good. Making it
impossible to misuse is better.
>> Needed for what? Oh, method description() is deprecated since 1.42.0:
>> "use the Display impl or to_string()". I figure we need it because the
>> new way doesn't work with our oldest supported Rust version. Could
>> perhaps use a comment to help future garbage collectors.
>>
>> > + fn description(&self) -> &str {
>> > + self.msg
>> > + .as_deref()
>> > + .or_else(|| self.cause.as_deref().map(std::error::Error::description))
>> > + .unwrap_or("error")
>>
>> This gives us @msg, or else @cause, or else "error".
>>
>> Is it a good idea to ignore @cause when we have @msg?
>
>> > +impl Display for Error {
>> > + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
>> > ...
>> > + }
>>
>> This combines @msg and @cause:
>>
>> Differs from description(). Why?
>
> Because description() cannot build a dynamically-allocated string, it
> must return something that is already available in the Error. That
> limitation is probably why it was deprecated.
>
> Since it's deprecated we can expect that it won't be used and not
> worry too much.
Could we somehow get away with not implementing it?
>> > + fn from(msg: String) -> Self {
>> > + let location = panic::Location::caller();
>> > + Error {
>> > + msg: Some(Cow::Owned(msg)),
>> > + file: location.file(),
>> > + line: location.line(),
>> > + ..Default::default()
>>
>> I don't understand this line, I'm afraid.
>
> It says "every other field comes from "..Default::default()". I can
> replace it with "cause: None", and likewise below.
I've since learned a bit more about Default. Since the only field being
defaulted is @cause, I guess I'd use "cause: None" myself for
simplicity. Matter of taste, you choose.
However, if this is the only real reason for Error having the Default
trait, I'd suggest to kill the trait with fire, because ...
>> > +}
>> > +
>> > +impl From<&'static str> for Error {
>> > + #[track_caller]
>> > + fn from(msg: &'static str) -> Self {
>> > + let location = panic::Location::caller();
>> > + Error {
>> > + msg: Some(Cow::Borrowed(msg)),
>> > + file: location.file(),
>> > + line: location.line(),
>> > + ..Default::default()
>> > + }
>> > + }
>> > +}
>>
>> Same for another string type.
>
> Yes, this is for strings that are not allocated and are always
> valid---such as string constants.
>
>> Is there a way to create an Error with neither @msg nor @cause?
>
> Yes, Default::default()
... I do dislike being able to create such Errors :)
>> > + errp: *mut *mut bindings::Error,
>> > + ) -> Option<T> {
>> > + let Err(err) = result else {
>> > + return result.ok();
>> > + };
>> > +
>> > + // SAFETY: caller guarantees errp is valid
>> > + unsafe {
>> > + err.propagate(errp);
>> > + }
>> > + None
>>
>> @result is an Error. Propagate it, and return None.
>>
>> This is indeed like self.ok() with propagation added.
>
> Alternatively:
>
> result
> .map_err(|err| unsafe { err.propagate(errp) })
> .ok()
>
> Shorter, but the functional style can be off putting. What do you prefer?
I'm fine with the functional style, and I like brevity.
>> > + }
>> > +
>> > + /// Equivalent of the C function `error_propagate`. Fill `*errp`
>> > + /// with the information container in `result` if `errp` is not NULL;
>> > + /// then consume it.
>>
>> Note error_propagate() has the arguments in the opposite order.
>
> Yeah, here "self" must be first so that you use it as a method.
> Though, s/result/self/ as you noted.
>
>> > + /// # Safety
>> > + ///
>> > + /// `errp` must be valid; typically it is received from C code
>>
>> What does "valid" mean exactly?
>
> I will copy from `error_propagate` in v2.
>
>> Brief switch to the design level...
>>
>> In C, you're almost always better off with ERRP_GUARD(). Would you like
>> me to elaborate?
>>
>> You still have to use error_propagate() to accumulate multiple errors so
>> that the first one wins. That's commonly a dumb idea. Should we avoid
>> this pattern in Rust?
>
> In Rust there are three kinds of functions that use errors. Two are in qemu_api:
>
> (1) bridges from C to Rust function pointers. They receive a Result<>
> from Rust functions and use error_propagate() (probably via functions
> such as ok_or_propagate, bool_or_propagate, etc.) to prepare a C
> return value.
We received an idiomatic Rust Result<>. We want to pass it to a C-like
caller, i.e. satisfy the contract "on success, return a value and leave
argument @errp alone; on error, return an error value and store an Error
object in @errp".
(I'm ignoring void values for brevity)
The functions you mentioned split up the Result<>, do the storing, and
return the value to be returned.
> (2) bridges from Rust to C functions. They pass an Error** to the C
> function and use err_or_else() or err_or_unit() to prepare a Rust
> return value
We received a return value and possibly a C Error object. We want to
pass it to a Rust caller, i.e. return an idiomatic Result<>.
The functions you mentioned assemble the Result<> from the two values.
There's difference between Result<> and what we do in C: Result<> is
either a success value or an Error value. I.e. we have a success value
on success, an Error on failure. In C, we have a success value on
success, and both an error value and an Error on failure.
The (1) functions need to make up the missing error value. That's fine,
it's what we do in C, too.
The (2) functions throw away the error value. Usually, there's exactly
one error value, so no information is thrown away. But not necessarily.
I think it's okay to design for the common case now.
> Functions of kind (1) are like functions in C that do a single call
> and just pass down errp, for example user_creatable_add_qapi(). These
> do not need ERRP_GUARD() because:
>
> * the conversion from Result<> to C is pretty much a necessity, and
> it's done with functions that guarantee the propagation
>
> * the conversion function *consumes* the Result<>, guaranteeing that
> you do not propagate more than once with tragic results
>
> Function of kind (2) do not need ERRP_GUARD() because they do not take
> an Error** at all. They pass one down to C, but they return a
> Result<>.
>
> The third kind is Rust functions that are called from (1) and that
> themselves call (2). These are where ERRP_GUARD() would be used in C,
> but in Rust these do not see Error** at all. The "?" operator has the
> same effect as ERRP_GUARD(), i.e. it handles passing the error from
> called function to return value. What in C would be
>
> ERRP_GUARD();
> if (!func_that_returns_bool(...)) { return; }
>
> In Rust is
>
> func_that_returns_result(...)?;
>
> which is a bit disconcerting in the beginning but grows on you.
> (Generally I find that to be the experience with Rust. It's downright
> weird, but unlike C++ the good parts outweigh the weirdness).
>
>> > + pub unsafe fn err_or_unit(c_error: *mut bindings::Error) -> Result<()> {
>> > + // SAFETY: caller guarantees c_error is valid
>> > + unsafe { Self::err_or_else(c_error, || ()) }
>> > + }
>> > +
>> > + /// Convert a C `Error*` into a Rust `Result`, calling `f()` to
>> > + /// obtain an `Ok` value if `c_error` is NULL.
>> > + ///
>> > + /// # Safety
>> > + ///
>> > + /// `c_error` must be valid; typically it has been filled by a C
>> > + /// function.
>> > + pub unsafe fn err_or_else<T, F: FnOnce() -> T>(
>> > + c_error: *mut bindings::Error,
>> > + f: F,
>> > + ) -> Result<T> {
>> > + // SAFETY: caller guarantees c_error is valid
>> > + let err = unsafe { Option::<Self>::from_foreign(c_error) };
>> > + match err {
>> > + None => Ok(f()),
>> > + Some(err) => Err(err),
>> > + }
>> > + }
>> > +}
>>
>> Getting tired...
>
> No problem. While this is kinda important, it's not used yet. The
> clients would look like this:
>
> fn type_get_or_load_by_name(name: &str) -> Result<&TypeImpl> {
> unsafe {
> let err: *mut bindings::Error = ptr::null_mut();
> let typ: *mut TypeImpl = bindings::type_get_or_load_by_name(
> name.clone_to_foreign().as_ptr(), &mut err);
> // on success, "typ" can be accessed safely
> // on failure, turn the Error* into a qemu_api::Error and free it
> Result::err_or_else(err, || &*typ)
> }
>
> This is why I need to write tests.
>
>> > +impl CloneToForeign for Error {
>> > + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
>> > + // SAFETY: all arguments are controlled by this function
>> > + unsafe {
>> > + let err: *mut c_void = libc::malloc(std::mem::size_of::<bindings::Error>());
>> > + let err: &mut bindings::Error = &mut *err.cast();
>> > + *err = bindings::Error {
>> > + msg: format!("{self}").clone_to_foreign_ptr(),
>> > + err_class: bindings::ERROR_CLASS_GENERIC_ERROR,
>> > + src_len: self.file.len() as isize,
>> > + src: self.file.as_ptr().cast::<c_char>(),
>> > + line: self.line as c_int,
>> > + func: ptr::null_mut(),
>> > + hint: ptr::null_mut(),
>> > + };
>> > + OwnedPointer::new(err)
>> > + }
>> > + }
>> > +}
>>
>> Plausible to this Rust ignoramus :)
>
> Good, since these *Foreign traits are not part of the standard
> library, but rather something that I concocted and published outside
> QEMU. If you had no problems understanding how they were used (e.g.
> stuff like into_native or clone_to_foreign_ptr), that is good.
"Understanding" is a bit strong. I relied on my voodoo-programming
skills.
Thank you!
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 08/12] rust: qdev: support returning errors from realize
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (6 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 07/12] qemu-api: add bindings to Error Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-29 9:18 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 09/12] rust/hpet: change timer of num_timers to usize Paolo Bonzini
` (3 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 5 +++--
rust/hw/timer/hpet/src/hpet.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 bde3be65c5b..95fe9a34cd2 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/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 779681d6509..55eef669a9d 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.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* Re: [PATCH 08/12] rust: qdev: support returning errors from realize
2025-05-26 14:24 ` [PATCH 08/12] rust: qdev: support returning errors from realize Paolo Bonzini
@ 2025-05-29 9:18 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 9:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:51PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:51 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 08/12] rust: qdev: support returning errors from realize
> X-Mailer: git-send-email 2.49.0
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/char/pl011/src/device.rs | 5 +++--
> rust/hw/timer/hpet/src/hpet.rs | 5 +++--
> rust/qemu-api/src/qdev.rs | 10 ++++++----
> 3 files changed, 12 insertions(+), 8 deletions(-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 09/12] rust/hpet: change timer of num_timers to usize
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (7 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 08/12] rust: qdev: support returning errors from realize Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-29 9:11 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 10/12] hpet: return errors from realize if properties are incorrect Paolo Bonzini
` (2 subsequent siblings)
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
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.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/hpet.rs | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 55eef669a9d..b2922e6a843 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.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* Re: [PATCH 09/12] rust/hpet: change timer of num_timers to usize
2025-05-26 14:24 ` [PATCH 09/12] rust/hpet: change timer of num_timers to usize Paolo Bonzini
@ 2025-05-29 9:11 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 9:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
> @@ -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),
> },
Here Rust and C take slightly different paths.
Either changes should be made in the C version as well, or the
"functional\ [#issues]_ replacements" issue note in docs/devel/rust.rst
needs to be updated as well. I think the former is simpler!
Otherwise,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 10/12] hpet: return errors from realize if properties are incorrect
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (8 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 09/12] rust/hpet: change timer of num_timers to usize Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-27 14:01 ` Markus Armbruster
2025-05-29 8:39 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 11/12] rust/hpet: " Paolo Bonzini
2025-05-26 14:24 ` [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers Paolo Bonzini
11 siblings, 2 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
Do not silently adjust num_timers, and fail if intcap is 0.
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 d1b7bc52b7b..d78aba04bcd 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -689,8 +689,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 */
@@ -698,7 +704,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;
}
@@ -708,11 +714,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* Re: [PATCH 10/12] hpet: return errors from realize if properties are incorrect
2025-05-26 14:24 ` [PATCH 10/12] hpet: return errors from realize if properties are incorrect Paolo Bonzini
@ 2025-05-27 14:01 ` Markus Armbruster
2025-05-29 8:39 ` Zhao Liu
1 sibling, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2025-05-27 14:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-rust
Paolo Bonzini <pbonzini@redhat.com> writes:
> Do not silently adjust num_timers, and fail if intcap is 0.
A bad habit of ours.
> 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 d1b7bc52b7b..d78aba04bcd 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -689,8 +689,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 */
> @@ -698,7 +704,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;
> }
>
> @@ -708,11 +714,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);
This device is not user-creatable. It is only ever created (and
realized) by board code. Errors should not happen. If they happen
anyway, it's a board code bug.
The code creating it is pc_basic_device_init():
if (pcms->hpet_enabled) {
qemu_irq rtc_irq;
hpet = qdev_try_new(TYPE_HPET);
if (!hpet) {
error_report("couldn't create HPET device");
exit(1);
}
Could just as well use qdev_new(). Differently confusing error message,
though.
/*
* For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-*,
* use IRQ16~23, IRQ8 and IRQ2. If the user has already set
* the property, use whatever mask they specified.
*/
uint8_t compat = object_property_get_uint(OBJECT(hpet),
HPET_INTCAP, NULL);
if (!compat) {
qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
}
sysbus_realize_and_unref(SYS_BUS_DEVICE(hpet), &error_fatal);
If this fails, it's a programming error, i.e. &error_abort is more
appropriate. Hmm, can the user mess with property values via -global?
If yes, it could be a user error.
[...]
}
I'm rambling. The patch is fine.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 10/12] hpet: return errors from realize if properties are incorrect
2025-05-26 14:24 ` [PATCH 10/12] hpet: return errors from realize if properties are incorrect Paolo Bonzini
2025-05-27 14:01 ` Markus Armbruster
@ 2025-05-29 8:39 ` Zhao Liu
1 sibling, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 8:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:53PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:53 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 10/12] hpet: return errors from realize if properties are
> incorrect
> X-Mailer: git-send-email 2.49.0
>
> Do not silently adjust num_timers, and fail if intcap is 0.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/timer/hpet.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 11/12] rust/hpet: return errors from realize if properties are incorrect
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (9 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 10/12] hpet: return errors from realize if properties are incorrect Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-29 9:15 ` Zhao Liu
2025-05-26 14:24 ` [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers Paolo Bonzini
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, qemu-rust
Match the code in hpet.c; this also allows removing the
BqlCell from the num_timers field.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/timer/hpet/src/fw_cfg.rs | 7 +++----
rust/hw/timer/hpet/src/hpet.rs | 16 +++++++---------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
index aa08d283519..140692062cd 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) {
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index b2922e6a843..b298938e4d5 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.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.
--
2.49.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] rust/hpet: return errors from realize if properties are incorrect
2025-05-26 14:24 ` [PATCH 11/12] rust/hpet: " Paolo Bonzini
@ 2025-05-29 9:15 ` Zhao Liu
2025-05-29 8:56 ` Paolo Bonzini
0 siblings, 1 reply; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 9:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:54PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:54 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 11/12] rust/hpet: return errors from realize if properties
> are incorrect
> X-Mailer: git-send-email 2.49.0
>
> Match the code in hpet.c; this also allows removing the
> BqlCell from the num_timers field.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> rust/hw/timer/hpet/src/fw_cfg.rs | 7 +++----
> rust/hw/timer/hpet/src/hpet.rs | 16 +++++++---------
> 2 files changed, 10 insertions(+), 13 deletions(-)
Patch is fine for me,
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> 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")?;
> }
Though here we don't need print warning...do we still need to provide
the warn_report() binding? Or println!() is enough in Rust side?
Thanks,
Zhao
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] rust/hpet: return errors from realize if properties are incorrect
2025-05-29 9:15 ` Zhao Liu
@ 2025-05-29 8:56 ` Paolo Bonzini
0 siblings, 0 replies; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-29 8:56 UTC (permalink / raw)
To: Zhao Liu; +Cc: qemu-devel, armbru, qemu-rust
On Thu, May 29, 2025 at 10:54 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > 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")?;
> > }
>
> Though here we don't need print warning...do we still need to provide
> the warn_report() binding? Or println!() is enough in Rust side?
I think it will be enough to have Error ** and
LOG_GUEST_ERROR/LOG_UNIMP... but we'll see.
Paolo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers
2025-05-26 14:22 [PATCH 00/12] rust: bindings for Error Paolo Bonzini
` (10 preceding siblings ...)
2025-05-26 14:24 ` [PATCH 11/12] rust/hpet: " Paolo Bonzini
@ 2025-05-26 14:24 ` Paolo Bonzini
2025-05-29 9:17 ` Zhao Liu
11 siblings, 1 reply; 32+ messages in thread
From: Paolo Bonzini @ 2025-05-26 14:24 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, 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/hpet.rs | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index b298938e4d5..3c5c65ff47d 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.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* Re: [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers
2025-05-26 14:24 ` [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers Paolo Bonzini
@ 2025-05-29 9:17 ` Zhao Liu
0 siblings, 0 replies; 32+ messages in thread
From: Zhao Liu @ 2025-05-29 9:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, qemu-rust
On Mon, May 26, 2025 at 04:24:55PM +0200, Paolo Bonzini wrote:
> Date: Mon, 26 May 2025 16:24:55 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 12/12] rust/hpet: Drop BqlCell wrapper for num_timers
> X-Mailer: git-send-email 2.49.0
>
> 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/hpet.rs | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
Thank you! Now we're finally here.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread