We added @error_warn some two years ago in commit 3ffef1a55ca (error:
add global &error_warn destination). It has multiple issues:
* error.h's big comment was not updated for it.
* Function contracts were not updated for it.
* ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
error_prepend() and such. These crash on @error_warn, as pointed
out by Akihiko Odaki.
All fixable. However, after more than two years, we had just of 15
uses, of which the last few patches removed eight as unclean or
otherwise undesirable. I didn't look closely enough at the remaining
seven to decide whether they are desirable or not.
I don't think this feature earns its keep. Drop it.
Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/error.h | 6 ------
hw/display/virtio-gpu.c | 8 ++++++--
hw/net/virtio-net.c | 8 +++++++-
tests/unit/test-error-report.c | 17 -----------------
ui/gtk.c | 6 +++++-
util/error.c | 5 +----
6 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 41e3816380..b16c6303f8 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -533,12 +533,6 @@ static inline void error_propagator_cleanup(ErrorPropagator *prop)
G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
-/*
- * Special error destination to warn on error.
- * See error_setg() and error_propagate() for details.
- */
-extern Error *error_warn;
-
/*
* Special error destination to abort on error.
* See error_setg() and error_propagate() for details.
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 0a1a625b0e..de35902213 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -242,6 +242,7 @@ static uint32_t calc_image_hostmem(pixman_format_code_t pformat,
static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
{
+ Error *err = NULL;
pixman_format_code_t pformat;
struct virtio_gpu_simple_resource *res;
struct virtio_gpu_resource_create_2d c2d;
@@ -293,7 +294,8 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
c2d.width,
c2d.height,
c2d.height ? res->hostmem / c2d.height : 0,
- &error_warn)) {
+ &err)) {
+ warn_report_err(err);
goto end;
}
}
@@ -1282,6 +1284,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
const VMStateField *field)
{
VirtIOGPU *g = opaque;
+ Error *err = NULL;
struct virtio_gpu_simple_resource *res;
uint32_t resource_id, pformat;
int i;
@@ -1317,7 +1320,8 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
res->width,
res->height,
res->height ? res->hostmem / res->height : 0,
- &error_warn)) {
+ &err)) {
+ warn_report_err(err);
g_free(res);
return -EINVAL;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 6b5b5dace3..7848e26278 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1289,6 +1289,8 @@ exit:
static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
{
+ Error *err = NULL;
+
if (!virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
return true;
}
@@ -1306,7 +1308,11 @@ static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
return virtio_net_load_ebpf_fds(n, errp);
}
- ebpf_rss_load(&n->ebpf_rss, &error_warn);
+ ebpf_rss_load(&n->ebpf_rss, &err);
+ /* Beware, ebpf_rss_load() can return false with @err unset */
+ if (err) {
+ warn_report_err(err);
+ }
return true;
}
diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c
index 54319c86c9..0cbde3c4cf 100644
--- a/tests/unit/test-error-report.c
+++ b/tests/unit/test-error-report.c
@@ -104,22 +104,6 @@ test_error_report_timestamp(void)
");
}
-static void
-test_error_warn(void)
-{
- if (g_test_subprocess()) {
- error_setg(&error_warn, "Testing &error_warn");
- return;
- }
-
- g_test_trap_subprocess(NULL, 0, 0);
- g_test_trap_assert_passed();
- g_test_trap_assert_stderr("\
-test-error-report: warning: Testing &error_warn*\
-");
-}
-
-
int
main(int argc, char *argv[])
{
@@ -133,7 +117,6 @@ main(int argc, char *argv[])
g_test_add_func("/error-report/glog", test_error_report_glog);
g_test_add_func("/error-report/once", test_error_report_once);
g_test_add_func("/error-report/timestamp", test_error_report_timestamp);
- g_test_add_func("/error-report/warn", test_error_warn);
return g_test_run();
}
diff --git a/ui/gtk.c b/ui/gtk.c
index e91d093a49..9a08cadc88 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1181,6 +1181,7 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch,
void *opaque)
{
VirtualConsole *vc = opaque;
+ Error *err = NULL;
uint64_t num_slot = GPOINTER_TO_UINT(touch->sequence);
int type = -1;
@@ -1203,7 +1204,10 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch,
console_handle_touch_event(vc->gfx.dcl.con, touch_slots,
num_slot, surface_width(vc->gfx.ds),
surface_height(vc->gfx.ds), touch->x,
- touch->y, type, &error_warn);
+ touch->y, type, &err);
+ if (err) {
+ warn_report_err(err);
+ }
return TRUE;
}
diff --git a/util/error.c b/util/error.c
index daea2142f3..0ae08225c0 100644
--- a/util/error.c
+++ b/util/error.c
@@ -19,7 +19,6 @@
Error *error_abort;
Error *error_fatal;
-Error *error_warn;
static void error_handle(Error **errp, Error *err)
{
@@ -41,9 +40,7 @@ static void error_handle(Error **errp, Error *err)
error_report_err(err);
exit(1);
}
- if (errp == &error_warn) {
- warn_report_err(err);
- } else if (errp && !*errp) {
+ if (errp && !*errp) {
*errp = err;
} else {
error_free(err);
--
2.49.0On Fri, Aug 08, 2025 at 10:08:23AM +0200, Markus Armbruster wrote:
> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
> add global &error_warn destination). It has multiple issues:
>
> * error.h's big comment was not updated for it.
>
> * Function contracts were not updated for it.
>
> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
> error_prepend() and such. These crash on @error_warn, as pointed
> out by Akihiko Odaki.
>
> All fixable. However, after more than two years, we had just of 15
> uses, of which the last few patches removed eight as unclean or
> otherwise undesirable. I didn't look closely enough at the remaining
> seven to decide whether they are desirable or not.
>
> I don't think this feature earns its keep. Drop it.
>
> Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/error.h | 6 ------
> hw/display/virtio-gpu.c | 8 ++++++--
> hw/net/virtio-net.c | 8 +++++++-
> tests/unit/test-error-report.c | 17 -----------------
> ui/gtk.c | 6 +++++-
> util/error.c | 5 +----
> 6 files changed, 19 insertions(+), 31 deletions(-)
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6b5b5dace3..7848e26278 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1289,6 +1289,8 @@ exit:
>
> static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
> {
> + Error *err = NULL;
> +
> if (!virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
> return true;
> }
> @@ -1306,7 +1308,11 @@ static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
> return virtio_net_load_ebpf_fds(n, errp);
> }
>
> - ebpf_rss_load(&n->ebpf_rss, &error_warn);
> + ebpf_rss_load(&n->ebpf_rss, &err);
> + /* Beware, ebpf_rss_load() can return false with @err unset */
Per our other mail, this is a bug we should fix
> + if (err) {
> + warn_report_err(err);
> + }
> return true;
> }
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Aug 08, 2025 at 10:08:23AM +0200, Markus Armbruster wrote:
>> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
>> add global &error_warn destination). It has multiple issues:
>>
>> * error.h's big comment was not updated for it.
>>
>> * Function contracts were not updated for it.
>>
>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>> error_prepend() and such. These crash on @error_warn, as pointed
>> out by Akihiko Odaki.
>>
>> All fixable. However, after more than two years, we had just of 15
>> uses, of which the last few patches removed eight as unclean or
>> otherwise undesirable. I didn't look closely enough at the remaining
>> seven to decide whether they are desirable or not.
>>
>> I don't think this feature earns its keep. Drop it.
>>
>> Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/error.h | 6 ------
>> hw/display/virtio-gpu.c | 8 ++++++--
>> hw/net/virtio-net.c | 8 +++++++-
>> tests/unit/test-error-report.c | 17 -----------------
>> ui/gtk.c | 6 +++++-
>> util/error.c | 5 +----
>> 6 files changed, 19 insertions(+), 31 deletions(-)
>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 6b5b5dace3..7848e26278 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1289,6 +1289,8 @@ exit:
>>
>> static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
>> {
>> + Error *err = NULL;
>> +
>> if (!virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
>> return true;
>> }
>> @@ -1306,7 +1308,11 @@ static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
>> return virtio_net_load_ebpf_fds(n, errp);
>> }
>>
>> - ebpf_rss_load(&n->ebpf_rss, &error_warn);
>> + ebpf_rss_load(&n->ebpf_rss, &err);
>> + /* Beware, ebpf_rss_load() can return false with @err unset */
>
> Per our other mail, this is a bug we should fix
Yes, but I still don't know how to fix it. The remaining open question
is "Is it a programming error when it happens?" in
Message-ID: <87sehfsife.fsf@pond.sub.org>
Help!
>> + if (err) {
>> + warn_report_err(err);
>> + }
>> return true;
>> }
>>
>
> With regards,
> Daniel
On 2025/08/08 17:08, Markus Armbruster wrote:
> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
> add global &error_warn destination). It has multiple issues:
>
> * error.h's big comment was not updated for it.
>
> * Function contracts were not updated for it.
>
> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
> error_prepend() and such. These crash on @error_warn, as pointed
> out by Akihiko Odaki.
>
> All fixable. However, after more than two years, we had just of 15
> uses, of which the last few patches removed eight as unclean or
> otherwise undesirable. I didn't look closely enough at the remaining
> seven to decide whether they are desirable or not.
>
> I don't think this feature earns its keep. Drop it.
I want to note that the following patch series temporarily use
&error_warn during its conversion to add errp parameters:
https://lore.kernel.org/qemu-devel/20250808-propagate_tpm_error-v10-0-3e81a1d419b2@redhat.com/
("[PATCH v10 00/27] migration: propagate vTPM errors using Error objects")
I think this series needs to be rebased on top of the migration change.
I'm not sure if seven uses are insufficient to keep it.
I also have a general impression that perhaps a special error
destination for error_report_err() is more useful. Today, there are many
functions use Error, but there are also functions that still follow old
error handling patterns. When legacy functions call functions with
Error, a common pattern is to use error_report_err() and return -1.
"[PATCH 01/12] monitor: Clean up HMP gdbserver error reporting" and
"[PATCH 09/12] ui/pixman: Consistent error handling in
qemu_pixman_shareable_free()" are examples that will benefit from
error_report_err() as an error destination. The migration patch series I
mentioned earlier can also use one.
Perhaps it is nicer if there is an infrastructure shared by the special
destinations. In particular, we can have common solutions for the three
problems you pointed out:
> * error.h's big comment was not updated for it.
>
> * Function contracts were not updated for it.
For these two problems, they can refer to "special error destinations"
instead of listing them, and delegate explanations of them to
corresponding ones.
>
> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
> error_prepend() and such. These crash on @error_warn, as pointed
> out by Akihiko Odaki.
For this problem, Error can tell that it is a special destination by
leaving msg NULL, for example. ERRP_GUARD() then rewrites errp it is not
&error_abort and msg is NULL.
Special destinations can also have a function pointer void (*)(Error*),
which will be called by error_handle(). This way, we can ensure that
having a special destination will not require changes to the common code.
By the way, I also asked for a comment with the migration patch series.
Please reply the following if you have anything to say:
https://lore.kernel.org/qemu-devel/9c552525-72fa-4d1e-89a2-b5c0e446935a@rsg.ci.i.u-tokyo.ac.jp/
There is also an additional context:
https://lore.kernel.org/qemu-devel/aJMsRBd9-XOMRG78@armenon-kvm.bengluru.csb/
Regards,
Akihiko Odaki
>
> Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/error.h | 6 ------
> hw/display/virtio-gpu.c | 8 ++++++--
> hw/net/virtio-net.c | 8 +++++++-
> tests/unit/test-error-report.c | 17 -----------------
> ui/gtk.c | 6 +++++-
> util/error.c | 5 +----
> 6 files changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 41e3816380..b16c6303f8 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -533,12 +533,6 @@ static inline void error_propagator_cleanup(ErrorPropagator *prop)
>
> G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>
> -/*
> - * Special error destination to warn on error.
> - * See error_setg() and error_propagate() for details.
> - */
> -extern Error *error_warn;
> -
> /*
> * Special error destination to abort on error.
> * See error_setg() and error_propagate() for details.
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 0a1a625b0e..de35902213 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -242,6 +242,7 @@ static uint32_t calc_image_hostmem(pixman_format_code_t pformat,
> static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
> struct virtio_gpu_ctrl_command *cmd)
> {
> + Error *err = NULL;
> pixman_format_code_t pformat;
> struct virtio_gpu_simple_resource *res;
> struct virtio_gpu_resource_create_2d c2d;
> @@ -293,7 +294,8 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
> c2d.width,
> c2d.height,
> c2d.height ? res->hostmem / c2d.height : 0,
> - &error_warn)) {
> + &err)) {
> + warn_report_err(err);
> goto end;
> }
> }
> @@ -1282,6 +1284,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> const VMStateField *field)
> {
> VirtIOGPU *g = opaque;
> + Error *err = NULL;
> struct virtio_gpu_simple_resource *res;
> uint32_t resource_id, pformat;
> int i;
> @@ -1317,7 +1320,8 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> res->width,
> res->height,
> res->height ? res->hostmem / res->height : 0,
> - &error_warn)) {
> + &err)) {
> + warn_report_err(err);
> g_free(res);
> return -EINVAL;
> }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 6b5b5dace3..7848e26278 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1289,6 +1289,8 @@ exit:
>
> static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
> {
> + Error *err = NULL;
> +
> if (!virtio_net_attach_ebpf_to_backend(n->nic, -1)) {
> return true;
> }
> @@ -1306,7 +1308,11 @@ static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp)
> return virtio_net_load_ebpf_fds(n, errp);
> }
>
> - ebpf_rss_load(&n->ebpf_rss, &error_warn);
> + ebpf_rss_load(&n->ebpf_rss, &err);
> + /* Beware, ebpf_rss_load() can return false with @err unset */
> + if (err) {
> + warn_report_err(err);
> + }
> return true;
> }
>
> diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c
> index 54319c86c9..0cbde3c4cf 100644
> --- a/tests/unit/test-error-report.c
> +++ b/tests/unit/test-error-report.c
> @@ -104,22 +104,6 @@ test_error_report_timestamp(void)
> ");
> }
>
> -static void
> -test_error_warn(void)
> -{
> - if (g_test_subprocess()) {
> - error_setg(&error_warn, "Testing &error_warn");
> - return;
> - }
> -
> - g_test_trap_subprocess(NULL, 0, 0);
> - g_test_trap_assert_passed();
> - g_test_trap_assert_stderr("\
> -test-error-report: warning: Testing &error_warn*\
> -");
> -}
> -
> -
> int
> main(int argc, char *argv[])
> {
> @@ -133,7 +117,6 @@ main(int argc, char *argv[])
> g_test_add_func("/error-report/glog", test_error_report_glog);
> g_test_add_func("/error-report/once", test_error_report_once);
> g_test_add_func("/error-report/timestamp", test_error_report_timestamp);
> - g_test_add_func("/error-report/warn", test_error_warn);
>
> return g_test_run();
> }
> diff --git a/ui/gtk.c b/ui/gtk.c
> index e91d093a49..9a08cadc88 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1181,6 +1181,7 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch,
> void *opaque)
> {
> VirtualConsole *vc = opaque;
> + Error *err = NULL;
> uint64_t num_slot = GPOINTER_TO_UINT(touch->sequence);
> int type = -1;
>
> @@ -1203,7 +1204,10 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch,
> console_handle_touch_event(vc->gfx.dcl.con, touch_slots,
> num_slot, surface_width(vc->gfx.ds),
> surface_height(vc->gfx.ds), touch->x,
> - touch->y, type, &error_warn);
> + touch->y, type, &err);
> + if (err) {
> + warn_report_err(err);
> + }
> return TRUE;
> }
>
> diff --git a/util/error.c b/util/error.c
> index daea2142f3..0ae08225c0 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -19,7 +19,6 @@
>
> Error *error_abort;
> Error *error_fatal;
> -Error *error_warn;
>
> static void error_handle(Error **errp, Error *err)
> {
> @@ -41,9 +40,7 @@ static void error_handle(Error **errp, Error *err)
> error_report_err(err);
> exit(1);
> }
> - if (errp == &error_warn) {
> - warn_report_err(err);
> - } else if (errp && !*errp) {
> + if (errp && !*errp) {
> *errp = err;
> } else {
> error_free(err);
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> On 2025/08/08 17:08, Markus Armbruster wrote:
>> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
>> add global &error_warn destination). It has multiple issues:
>>
>> * error.h's big comment was not updated for it.
>>
>> * Function contracts were not updated for it.
>>
>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>> error_prepend() and such. These crash on @error_warn, as pointed
>> out by Akihiko Odaki.
>>
>> All fixable. However, after more than two years, we had just of 15
>> uses, of which the last few patches removed eight as unclean or
>> otherwise undesirable. I didn't look closely enough at the remaining
>> seven to decide whether they are desirable or not.
>>
>> I don't think this feature earns its keep. Drop it.
>
> I want to note that the following patch series temporarily use &error_warn during its conversion to add errp parameters:
> https://lore.kernel.org/qemu-devel/20250808-propagate_tpm_error-v10-0-3e81a1d419b2@redhat.com/
> ("[PATCH v10 00/27] migration: propagate vTPM errors using Error objects")
>
> I think this series needs to be rebased on top of the migration change.
Thanks for the heads-up.
> I'm not sure if seven uses are insufficient to keep it.
>
> I also have a general impression that perhaps a special error destination for error_report_err() is more useful. Today, there are many functions use Error, but there are also functions that still follow old error handling patterns. When legacy functions call functions with Error, a common pattern is to use error_report_err() and return -1.
You mean like &error_fatal less the exit(1)?
> "[PATCH 01/12] monitor: Clean up HMP gdbserver error reporting" and "[PATCH 09/12] ui/pixman: Consistent error handling in qemu_pixman_shareable_free()" are examples that will benefit from error_report_err() as an error destination. The migration patch series I mentioned earlier can also use one.
>
> Perhaps it is nicer if there is an infrastructure shared by the special destinations. In particular, we can have common solutions for the three problems you pointed out:
>
>> * error.h's big comment was not updated for it.
>>
>> * Function contracts were not updated for it.
>
> For these two problems, they can refer to "special error destinations" instead of listing them, and delegate explanations of them to corresponding ones.
>
>>
>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>> error_prepend() and such. These crash on @error_warn, as pointed
>> out by Akihiko Odaki.
>
> For this problem, Error can tell that it is a special destination by leaving msg NULL, for example. ERRP_GUARD() then rewrites errp it is not &error_abort and msg is NULL.
As I wrote, the defects are all fixable. However, there has been so
little use of &error_warn, and so much of it has been unclean or
otherwise undesirable. It's obviously prone to misuse. I think we're
better off without it.
See also the memo "Abuse of warnings for unhandled errors and
programming errors" I posted yesterday.
> Special destinations can also have a function pointer void (*)(Error*), which will be called by error_handle(). This way, we can ensure that having a special destination will not require changes to the common code.
>
> By the way, I also asked for a comment with the migration patch series. Please reply the following if you have anything to say:
> https://lore.kernel.org/qemu-devel/9c552525-72fa-4d1e-89a2-b5c0e446935a@rsg.ci.i.u-tokyo.ac.jp/
>
> There is also an additional context:
> https://lore.kernel.org/qemu-devel/aJMsRBd9-XOMRG78@armenon-kvm.bengluru.csb/
I replied there.
I'll be on vacation the next two weeks.
On 2025/08/09 17:30, Markus Armbruster wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>
>> On 2025/08/08 17:08, Markus Armbruster wrote:
>>> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
>>> add global &error_warn destination). It has multiple issues:
>>>
>>> * error.h's big comment was not updated for it.
>>>
>>> * Function contracts were not updated for it.
>>>
>>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>>> error_prepend() and such. These crash on @error_warn, as pointed
>>> out by Akihiko Odaki.
>>>
>>> All fixable. However, after more than two years, we had just of 15
>>> uses, of which the last few patches removed eight as unclean or
>>> otherwise undesirable. I didn't look closely enough at the remaining
>>> seven to decide whether they are desirable or not.
>>>
>>> I don't think this feature earns its keep. Drop it.
>>
>> I want to note that the following patch series temporarily use &error_warn during its conversion to add errp parameters:
>> https://lore.kernel.org/qemu-devel/20250808-propagate_tpm_error-v10-0-3e81a1d419b2@redhat.com/
>> ("[PATCH v10 00/27] migration: propagate vTPM errors using Error objects")
>>
>> I think this series needs to be rebased on top of the migration change.
>
> Thanks for the heads-up.
>
>> I'm not sure if seven uses are insufficient to keep it.
>>
>> I also have a general impression that perhaps a special error destination for error_report_err() is more useful. Today, there are many functions use Error, but there are also functions that still follow old error handling patterns. When legacy functions call functions with Error, a common pattern is to use error_report_err() and return -1.
>
> You mean like &error_fatal less the exit(1)?
Yes, that's what I meant.
>
>> "[PATCH 01/12] monitor: Clean up HMP gdbserver error reporting" and "[PATCH 09/12] ui/pixman: Consistent error handling in qemu_pixman_shareable_free()" are examples that will benefit from error_report_err() as an error destination. The migration patch series I mentioned earlier can also use one.
>>
>> Perhaps it is nicer if there is an infrastructure shared by the special destinations. In particular, we can have common solutions for the three problems you pointed out:
>>
>>> * error.h's big comment was not updated for it.
>>>
>>> * Function contracts were not updated for it.
>>
>> For these two problems, they can refer to "special error destinations" instead of listing them, and delegate explanations of them to corresponding ones.
>>
>>>
>>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>>> error_prepend() and such. These crash on @error_warn, as pointed
>>> out by Akihiko Odaki.
>>
>> For this problem, Error can tell that it is a special destination by leaving msg NULL, for example. ERRP_GUARD() then rewrites errp it is not &error_abort and msg is NULL.
>
> As I wrote, the defects are all fixable. However, there has been so
> little use of &error_warn, and so much of it has been unclean or
> otherwise undesirable. It's obviously prone to misuse. I think we're
> better off without it.
>
> See also the memo "Abuse of warnings for unhandled errors and
> programming errors" I posted yesterday.
It is insightful. The cover letter can have a reference to it and this
patch can have similar description.
However I still have a few counterarguments:
A reason of the &error_warn usage may be caused by the complexity
to deal with unhandled errors and programming errors. If so, adding
mechanisms to ease that may naturally sufficiently reduce the wrong
usage added in the future.
The "&error_fatal without exit(1)" may be good enough for unhandled
errors. For example, "[PATCH v10 00/27] migration: propagate vTPM errors
using Error objects" would not have used &error_warn if there is the
"&error_fatal without exit(1)".
There are also warn_report*() functions which also have the same
problem. A comprehensive solution needs to deal with them all. Removing
all of them will do but may not be practical. Another possibility is
that to write a documentation telling warning should be avoided for
unhandled/programming errors and let all refer to it.
I agree that there is little valid usage of &error_warn and removing it
may not cause a problem at all, but it is still nice if there is a
reasoning for the removal, ideally addressing these counterarguments as
well.
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> On 2025/08/09 17:30, Markus Armbruster wrote:
[...]
>> As I wrote, the defects are all fixable. However, there has been so
>> little use of &error_warn, and so much of it has been unclean or
>> otherwise undesirable. It's obviously prone to misuse. I think we're
>> better off without it.
>>
>> See also the memo "Abuse of warnings for unhandled errors and
>> programming errors" I posted yesterday.
>
> It is insightful. The cover letter can have a reference to it and this patch can have similar description.
>
> However I still have a few counterarguments:
>
> A reason of the &error_warn usage may be caused by the complexity
> to deal with unhandled errors and programming errors. If so, adding mechanisms to ease that may naturally sufficiently reduce the wrong usage added in the future.
>
> The "&error_fatal without exit(1)" may be good enough for unhandled errors. For example, "[PATCH v10 00/27] migration: propagate vTPM errors using Error objects" would not have used &error_warn if there is the "&error_fatal without exit(1)".
I fear it would be prone to misuse.
Consider
Error *err = NULL;
if (!frobnicate(a, b, c, &err)) {
error_report_err(err);
// cleanup here, if any
return false;
}
With a special &error_fatal_without_exit (terrible name, but you get
what I mean), we could shorten this to
if (!frobnicate(a, b, c, &error_fatal_without_exit)) {
// cleanup here, if any
return false;
}
However, this invites
frobnicate(a, b, c, &error_fatal_without_exit);
which is almost always wrong. If it's an error, we normally can't just
continue as if nothing had happened.
We should make the common case as easy to get rigtht as we can.
Making the exceptions even easier invites misuse.
> There are also warn_report*() functions which also have the same problem. A comprehensive solution needs to deal with them all. Removing all of them will do but may not be practical. Another possibility is that to write a documentation telling warning should be avoided for unhandled/programming errors and let all refer to it.
>
> I agree that there is little valid usage of &error_warn and removing it may not cause a problem at all, but it is still nice if there is a reasoning for the removal, ideally addressing these counterarguments as well.
I can look into improving the commit message after my vacation.
Thanks!
On 8/8/25 10:08, Markus Armbruster wrote: > We added @error_warn some two years ago in commit 3ffef1a55ca (error: > add global &error_warn destination). It has multiple issues: > > * error.h's big comment was not updated for it. > > * Function contracts were not updated for it. > > * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from > error_prepend() and such. These crash on @error_warn, as pointed > out by Akihiko Odaki. > > All fixable. However, after more than two years, we had just of 15 > uses, of which the last few patches removed eight as unclean or > otherwise undesirable. I didn't look closely enough at the remaining > seven to decide whether they are desirable or not. Is it a call for help? If so, better to split this patch per maintained areas, and finally kill @error_warn. > > I don't think this feature earns its keep. Drop it. > > Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > include/qapi/error.h | 6 ------ > hw/display/virtio-gpu.c | 8 ++++++-- > hw/net/virtio-net.c | 8 +++++++- > tests/unit/test-error-report.c | 17 ----------------- > ui/gtk.c | 6 +++++- > util/error.c | 5 +---- > 6 files changed, 19 insertions(+), 31 deletions(-)
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 8/8/25 10:08, Markus Armbruster wrote: >> We added @error_warn some two years ago in commit 3ffef1a55ca (error: >> add global &error_warn destination). It has multiple issues: >> >> * error.h's big comment was not updated for it. >> >> * Function contracts were not updated for it. >> >> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from >> error_prepend() and such. These crash on @error_warn, as pointed >> out by Akihiko Odaki. >> >> All fixable. However, after more than two years, we had just of 15 >> uses, of which the last few patches removed eight as unclean or >> otherwise undesirable. I didn't look closely enough at the remaining >> seven to decide whether they are desirable or not. > > Is it a call for help? If so, better to split this patch per > maintained areas, and finally kill @error_warn. The patch does kill &error_warn. It's simple and small. I don't splitting it makes review any easier. >> I don't think this feature earns its keep. Drop it. >> >> Thanks-to: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> include/qapi/error.h | 6 ------ >> hw/display/virtio-gpu.c | 8 ++++++-- >> hw/net/virtio-net.c | 8 +++++++- >> tests/unit/test-error-report.c | 17 ----------------- >> ui/gtk.c | 6 +++++- >> util/error.c | 5 +---- >> 6 files changed, 19 insertions(+), 31 deletions(-)
© 2016 - 2025 Red Hat, Inc.