[PATCH v3 00/13] Error reporting cleanup, a fix, and &error_warn removal

[PATCH v3 00/13] Error reporting cleanup, a fix, and &error_warn removal

[Markus Armbruster]

>From qapi/error.h:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

Not mentioned, but just as undesirable: error_setg(&error_warn, ...).

This series eliminates such uses, and gets rid of &error_warn.
&error_warn has multiple issues and little use.  PATCH 12 has full
rationale.

A note on warnings: we don't use warnings much, and when we use them,
they're often pretty bad.  See my memo "Abuse of warnings for
unhandled errors and programming errors"
Message-ID: <87h5yijh3b.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87h5yijh3b.fsf@pond.sub.org/

v3:
* PATCH 02: Whitespace cleanup [Akihiko Odaki]
* PATCH 06+07: Memory leaks on error paths [Akihiko Odaki]
* PATCH 08+13: Rebase conflicts
* PATCH 12: New

v2:
* PATCH 03: Mention change of cxl_fmws_link() return value in commit
  message [Jonathan]
* PATCH 04: Change exit(1) to g_assert_not_reached(), because it's a
  programming error.
* PATCH 06+07: Replace questions in commit message by answers from
  review.
* PATCH 06: Fix a format string
* PATCH 08: Keep warnings instead of reverting to silence [Daniel]
* PATCH 12: Adjusted for replaced PATCH 08

Issues raised in review I decided not to address in this series:
* PATCH 03: messages could be improved further, in particular the
  "gdbstub: " prefix could be dropped
* ebpf_rss_load() can return false without setting an error
* Capture the discussion on how to deal with undhandled errors in
  cover letter and/or commit messages.

The first two could be done on top.

Markus Armbruster (13):
  monitor: Clean up HMP gdbserver error reporting
  tcg: Fix error reporting on mprotect() failure in  tcg_region_init()
  hw/cxl: Convert cxl_fmws_link() to Error
  migration/cpr: Clean up error reporting in cpr_resave_fd()
  hw/remote/vfio-user: Clean up error reporting
  net/slirp: Clean up error reporting
  ui/spice-core: Clean up error reporting
  util/oslib-win32: Do not treat null @errp as &error_warn
  ui/pixman: Consistent error handling in qemu_pixman_shareable_free()
  ui/dbus: Clean up dbus_update_gl_cb() error checking
  ui/dbus: Consistent handling of texture mutex failure
  ivshmem-flat: Mark an instance of missing error handling FIXME
  error: Kill @error_warn

 include/exec/gdbstub.h         |  3 ---
 include/qapi/error.h           |  6 ------
 include/system/os-win32.h      |  5 ++++-
 hw/cxl/cxl-host.c              |  7 ++++---
 hw/display/virtio-gpu.c        |  8 ++++++--
 hw/misc/ivshmem-flat.c         |  8 ++++++--
 hw/net/virtio-net.c            |  8 +++++++-
 hw/remote/vfio-user-obj.c      |  9 +++------
 io/channel-socket.c            |  4 ++--
 io/channel-watch.c             |  6 +++---
 migration/cpr.c                |  9 +++++----
 monitor/hmp-cmds.c             |  7 ++++---
 net/slirp.c                    |  9 +++++++--
 tcg/region.c                   |  7 +++++--
 tests/unit/test-error-report.c | 17 -----------------
 ui/dbus-listener.c             | 22 +++++++++++++++-------
 ui/gtk.c                       |  6 +++++-
 ui/qemu-pixman.c               |  5 ++++-
 ui/spice-core.c                |  6 ++++--
 util/aio-win32.c               |  2 +-
 util/error.c                   |  5 +----
 util/oslib-win32.c             | 25 ++++++++++++++++++++-----
 22 files changed, 106 insertions(+), 78 deletions(-)

-- 
2.49.0

[PATCH v3 01/13] monitor: Clean up HMP gdbserver error reporting

[Markus Armbruster]

HMP command gdbserver used to emit two error messages for certain
errors.  For instance, with -M none:

    (qemu) gdbserver
    gdbstub: meaningless to attach gdb to a machine without any CPU.
    Could not open gdbserver on device 'tcp::1234'

The first message is the specific error, and the second one a generic
additional message that feels superfluous to me.

Commit c0e6b8b798b (system: propagate Error to gdbserver_start (and
other device setups)) turned the first message into a warning:

    warning: gdbstub: meaningless to attach gdb to a machine without any CPU.
    Could not open gdbserver on device 'tcp::1234'

This is arguably worse.

hmp_gdbserver() passes &error_warn to gdbserver_start(), so that
failure gets reported as warning, and then additionally emits the
generic error on failure.  This is a misuse of &error_warn.

Instead, receive the error in &err and report it, as usual.  With
this, gdbserver reports just the error:

    gdbstub: meaningless to attach gdb to a machine without any CPU.

Cc: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/exec/gdbstub.h | 3 ---
 monitor/hmp-cmds.c     | 7 ++++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index a16c0051ce..bd7182c4d3 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -55,9 +55,6 @@ void gdb_unregister_coprocessor_all(CPUState *cpu);
  * system emulation you can use a full chardev spec for your gdbserver
  * port.
  *
- * The error handle should be either &error_fatal (for start-up) or
- * &error_warn (for QMP/HMP initiated sessions).
- *
  * Returns true when server successfully started.
  */
 bool gdbserver_start(const char *port_or_device, Error **errp);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 74a0f56566..33a88ce205 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -280,14 +280,15 @@ void hmp_log(Monitor *mon, const QDict *qdict)
 
 void hmp_gdbserver(Monitor *mon, const QDict *qdict)
 {
+    Error *err = NULL;
     const char *device = qdict_get_try_str(qdict, "device");
+
     if (!device) {
         device = "tcp::" DEFAULT_GDBSTUB_PORT;
     }
 
-    if (!gdbserver_start(device, &error_warn)) {
-        monitor_printf(mon, "Could not open gdbserver on device '%s'\n",
-                       device);
+    if (!gdbserver_start(device, &err)) {
+        error_report_err(err);
     } else if (strcmp(device, "none") == 0) {
         monitor_printf(mon, "Disabled gdbserver\n");
     } else {
-- 
2.49.0

[PATCH v3 02/13] tcg: Fix error reporting on mprotect() failure in tcg_region_init()

[Markus Armbruster]

tcg_region_init() calls one of qemu_mprotect_rwx(),
qemu_mprotect_rw(), and mprotect(), then reports failure with
error_setg_errno(&error_fatal, errno, ...).

The use of &error_fatal is undesirable.  qapi/error.h advises:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.

The use of errno is wrong.  qemu_mprotect_rwx() and qemu_mprotect_rw()
wrap around qemu_mprotect__osdep().  qemu_mprotect__osdep() calls
mprotect() on POSIX, VirtualProtect() on Windows, and reports failure
with error_report().  VirtualProtect() doesn't set errno.  mprotect()
does, but error_report() may clobber it.

Fix tcg_region_init() to report errors only when it calls mprotect(),
and rely on qemu_mprotect_rwx()'s and qemu_mprotect_rw()'s error
reporting otherwise.  Use error_report(), not error_setg().

Fixes: 22c6a9938f75 (tcg: Merge buffer protection and guard page protection)
Fixes: 6bc144237a85 (tcg: Use Error with alloc_code_gen_buffer)
Cc: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/region.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tcg/region.c b/tcg/region.c
index 7ea0b37a84..2181267e48 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -832,13 +832,16 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_threads)
             } else {
 #ifdef CONFIG_POSIX
                 rc = mprotect(start, end - start, need_prot);
+                if (rc) {
+                    error_report("mprotect of jit buffer: %s",
+                                 strerror(errno));
+                }
 #else
                 g_assert_not_reached();
 #endif
             }
             if (rc) {
-                error_setg_errno(&error_fatal, errno,
-                                 "mprotect of jit buffer");
+                exit(1);
             }
         }
         if (have_prot != 0) {
-- 
2.49.0

[PATCH v3 03/13] hw/cxl: Convert cxl_fmws_link() to Error

[Markus Armbruster]

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job.  When the caller does, the error is reported twice.  When it
doesn't (because it recovered from the error), there is no error to
report, i.e. the report is bogus.

cxl_fmws_link_targets() violates this principle: it calls
error_setg(&error_fatal, ...) via cxl_fmws_link().  Goes back to
commit 584f722eb3ab (hw/cxl: Make the CXL fixed memory windows
devices.)  Currently harmless, because cxl_fmws_link_targets()'s
callers always pass &error_fatal.  Clean this up by converting
cxl_fmws_link() to Error.

Also change its return value on error from 1 to -1 to conform to the
rules laid in qapi/error.h.  It's call chain cxl_fmws_link_targets()
via object_child_foreach_recursive() is fine with that.

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/cxl/cxl-host.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 5c2ce25a19..0d891c651d 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -72,6 +72,7 @@ static void cxl_fixed_memory_window_config(CXLFixedMemoryWindowOptions *object,
 
 static int cxl_fmws_link(Object *obj, void *opaque)
 {
+    Error **errp = opaque;
     struct CXLFixedWindow *fw;
     int i;
 
@@ -87,9 +88,9 @@ static int cxl_fmws_link(Object *obj, void *opaque)
         o = object_resolve_path_type(fw->targets[i], TYPE_PXB_CXL_DEV,
                                      &ambig);
         if (!o) {
-            error_setg(&error_fatal, "Could not resolve CXLFM target %s",
+            error_setg(errp, "Could not resolve CXLFM target %s",
                        fw->targets[i]);
-            return 1;
+            return -1;
         }
         fw->target_hbs[i] = PXB_CXL_DEV(o);
     }
@@ -99,7 +100,7 @@ static int cxl_fmws_link(Object *obj, void *opaque)
 void cxl_fmws_link_targets(Error **errp)
 {
     /* Order doesn't matter for this, so no need to build list */
-    object_child_foreach_recursive(object_get_root(), cxl_fmws_link, NULL);
+    object_child_foreach_recursive(object_get_root(), cxl_fmws_link, errp);
 }
 
 static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
-- 
2.49.0

[PATCH v3 04/13] migration/cpr: Clean up error reporting in cpr_resave_fd()

[Markus Armbruster]

qapi/error.h advises:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.

Do that, and replace exit() by g_assert_not_reached(), since this is
actually a programming error.

Cc: Steve Sistare <steven.sistare@oracle.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Steve Sistare <steven.sistare@oracle.com>
---
 migration/cpr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/migration/cpr.c b/migration/cpr.c
index 42ad0b0d50..9848a21ea6 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -7,6 +7,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "hw/vfio/vfio-device.h"
 #include "migration/cpr.h"
 #include "migration/misc.h"
@@ -100,10 +101,10 @@ void cpr_resave_fd(const char *name, int id, int fd)
     if (old_fd < 0) {
         cpr_save_fd(name, id, fd);
     } else if (old_fd != fd) {
-        error_setg(&error_fatal,
-                   "internal error: cpr fd '%s' id %d value %d "
-                   "already saved with a different value %d",
-                   name, id, fd, old_fd);
+        error_report("internal error: cpr fd '%s' id %d value %d "
+                     "already saved with a different value %d",
+                     name, id, fd, old_fd);
+        g_assert_not_reached();
     }
 }
 
-- 
2.49.0

[PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Markus Armbruster]

VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
...) when auto-shutdown is enabled, else with error_report().

Issues:

1. The error is serious enough to warrant aborting the process when
auto-shutdown is enabled, yet harmless enough to permit carrying on
when it's disabled.  This makes no sense to me.

2. Like assert(), &error_abort is strictly for programming errors.  Is
this one?  Or should we exit(1) instead?

3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
assert()."

This patch addresses just 3.

Cc: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/remote/vfio-user-obj.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index ea6165ebdc..eb96982a3a 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
  */
 #define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
     {                                                                     \
-        if (vfu_object_auto_shutdown()) {                                 \
-            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
-        } else {                                                          \
-            error_report((fmt), ## __VA_ARGS__);                          \
-        }                                                                 \
-    }                                                                     \
+        error_report((fmt), ## __VA_ARGS__);                              \
+        assert(!vfu_object_auto_shutdown());                              \
+    }
 
 struct VfuObjectClass {
     ObjectClass parent_class;
-- 
2.49.0

[PATCH v3 06/13] net/slirp: Clean up error reporting

[Markus Armbruster]

net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
report WSAEventSelect() failure with error_setg(&error_warn, ...).
error_setg_win32(&error_warn, ...) is undesirable just like
error_setg(&error_fatal, ...) and error_setg(&error_abort, ...)  are.
Replace by warn_report().

The failures should probably be errors, but these functions implement
callbacks that cannot fail, exit(1) would be too harsh, and silent
failure we don't want.  Thus, warnings.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/slirp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index 9657e86a84..0a1c2a5eac 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -258,11 +258,13 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
 {
 #ifdef WIN32
     AioContext *ctxt = qemu_get_aio_context();
+    g_autofree char *msg = NULL;
 
     if (WSAEventSelect(fd, event_notifier_get_handle(&ctxt->notifier),
                        FD_READ | FD_ACCEPT | FD_CLOSE |
                        FD_CONNECT | FD_WRITE | FD_OOB) != 0) {
-        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
+        msg = g_win32_error_message(WSAGetLastError());
+        warn_report("failed to WSAEventSelect(): %s", msg);
     }
 #endif
 }
@@ -270,8 +272,11 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
 static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
 {
 #ifdef WIN32
+    g_autofree char *msg = NULL;
+
     if (WSAEventSelect(fd, NULL, 0) != 0) {
-        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
+        msg = g_win32_error_message(WSAGetLastError());
+        warn_report("failed to WSAEventSelect(): %s", msg);
     }
 #endif
 }
-- 
2.49.0

[PATCH v3 07/13] ui/spice-core: Clean up error reporting

[Markus Armbruster]

watch_add() reports _open_osfhandle() failure with
error_setg(&error_warn, ...).  error_setg_win32(&error_warn, ...) is
undesirable just like error_setg(&error_fatal, ...) and
error_setg(&error_abort, ...) are.  Replace by warn_report().

The failure should probably be an error, but this function implements
a callback that doesn't take Error **.  I believe the failure will
make spice_server_init() fail in qemu_spice_init(), which is treated
as a fatal error.  The warning here provides more detail than the
error message there.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 ui/spice-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5992f9daec..7836202664 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -128,11 +128,13 @@ static void watch_update_mask(SpiceWatch *watch, int event_mask)
 static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
 {
     SpiceWatch *watch;
-
 #ifdef WIN32
+    g_autofree char *msg = NULL;
+
     fd = _open_osfhandle(fd, _O_BINARY);
     if (fd < 0) {
-        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET");
+        msg = g_win32_error_message(WSAGetLastError());
+        warn_report("Couldn't associate a FD with the SOCKET: %s", msg);
         return NULL;
     }
 #endif
-- 
2.49.0

[PATCH v3 08/13] util/oslib-win32: Do not treat null @errp as &error_warn

[Markus Armbruster]

qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
null @errp as &error_warn.  This is wildly inappropriate.  A caller
passing null @errp specifies that errors are to be ignored.  If
warnings are wanted, the caller must pass &error_warn.

Change callers to do that, and drop the inappropriate treatment of
null @errp.

This assumes that warnings are wanted.  I'm not familiar with the
calling code, so I can't say whether it will work when the socket is
invalid, or WSAEventSelect() fails.  If it doesn't, then this should
be an error instead of a warning.  Invalid socket might even be a
programming error.

These warnings were introduced in commit f5fd677ae7cf (win32/socket:
introduce qemu_socket_select() helper).  I considered reverting to
silence, but Daniel Berrangé asked for the warnings to be preserved.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 io/channel-socket.c | 4 ++--
 io/channel-watch.c  | 2 +-
 util/aio-win32.c    | 2 +-
 util/oslib-win32.c  | 6 +-----
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index e53d9ac76f..fdc7670867 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -454,7 +454,7 @@ static void qio_channel_socket_finalize(Object *obj)
             }
         }
 #ifdef WIN32
-        qemu_socket_unselect(ioc->fd, NULL);
+        qemu_socket_unselect(ioc->fd, &error_warn);
 #endif
         close(ioc->fd);
         ioc->fd = -1;
@@ -929,7 +929,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 
     if (sioc->fd != -1) {
 #ifdef WIN32
-        qemu_socket_unselect(sioc->fd, NULL);
+        qemu_socket_unselect(sioc->fd, &error_warn);
 #endif
         if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
             socket_listen_cleanup(sioc->fd, errp);
diff --git a/io/channel-watch.c b/io/channel-watch.c
index 64b486e378..ec76bd1ec6 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -283,7 +283,7 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
 
     qemu_socket_select(sockfd, ioc->event,
                        FD_READ | FD_ACCEPT | FD_CLOSE |
-                       FD_CONNECT | FD_WRITE | FD_OOB, NULL);
+                       FD_CONNECT | FD_WRITE | FD_OOB, &error_warn);
 
     source = g_source_new(&qio_channel_socket_source_funcs,
                           sizeof(QIOChannelSocketSource));
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 6583d5c5f3..b125924433 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
 
         QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
         event = event_notifier_get_handle(&ctx->notifier);
-        qemu_socket_select(fd, event, bitmask, NULL);
+        qemu_socket_select(fd, event, bitmask, &error_warn);
     }
     if (old_node) {
         aio_remove_fd_handler(ctx, old_node);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b9ce2f96ee..6a2367c89d 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -182,7 +182,7 @@ bool qemu_set_blocking(int fd, bool block, Error **errp)
     unsigned long opt = block ? 0 : 1;
 
     if (block) {
-        qemu_socket_unselect(fd, NULL);
+        qemu_socket_unselect(fd, &error_warn);
     }
 
     if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) {
@@ -293,10 +293,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
 {
     SOCKET s = _get_osfhandle(sockfd);
 
-    if (errp == NULL) {
-        errp = &error_warn;
-    }
-
     if (s == INVALID_SOCKET) {
         error_setg(errp, "invalid socket fd=%d", sockfd);
         return false;
-- 
2.49.0

[PATCH v3 09/13] ui/pixman: Consistent error handling in qemu_pixman_shareable_free()

[Markus Armbruster]

qemu_pixman_shareable_free() wraps around either qemu_memfd_free() or
qemu_win32_map_free().  The former reports trouble as error, with
error_report(), then succeeds.  The latter reports it as warning (we
pass it &error_warn), then succeeds.

Change the latter to report as error, too.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/qemu-pixman.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index ef4e71da11..e46c6232cf 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -288,7 +288,10 @@ qemu_pixman_shareable_free(qemu_pixman_shareable handle,
                            void *ptr, size_t size)
 {
 #ifdef WIN32
-    qemu_win32_map_free(ptr, handle, &error_warn);
+    Error *err = NULL;
+
+    qemu_win32_map_free(ptr, handle, &err);
+    error_report_err(err);
 #else
     qemu_memfd_free(ptr, size, handle);
 #endif
-- 
2.49.0

[PATCH v3 10/13] ui/dbus: Clean up dbus_update_gl_cb() error checking

[Markus Armbruster]

From GLib "Rules for use of GError":

    A GError* must be initialized to NULL before passing its address
    to a function that can report errors.

dbus_update_gl_cb() seemingly violates this rule: it passes &err to
qemu_dbus_display1_listener_call_update_dmabuf_finish() and to
qemu_dbus_display1_listener_win32_d3d11_call_update_texture2d_finish()
without clearing it in between.  Harmless, because the first call is
guarded by #ifdef CONFIG_GBM, the second by #ifdef WIN32, and the two
are mutually exclusive.  I think.

Clean this up to be obviously correct.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 42875b8eed..09d7a319b1 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -221,18 +221,21 @@ static void dbus_update_gl_cb(GObject *source_object,
 #ifdef CONFIG_GBM
     success = qemu_dbus_display1_listener_call_update_dmabuf_finish(
         ddl->proxy, res, &err);
+    if (!success) {
+        error_report("Failed to call update: %s", err->message);
+    }
 #endif
 
 #ifdef WIN32
     success = qemu_dbus_display1_listener_win32_d3d11_call_update_texture2d_finish(
         ddl->d3d11_proxy, res, &err);
-    d3d_texture2d_acquire0(ddl->d3d_texture, &error_warn);
-#endif
-
     if (!success) {
         error_report("Failed to call update: %s", err->message);
     }
 
+    d3d_texture2d_acquire0(ddl->d3d_texture, &error_warn);
+#endif
+
     graphic_hw_gl_block(ddl->dcl.con, false);
     g_object_unref(ddl);
 }
-- 
2.49.0

[PATCH v3 11/13] ui/dbus: Consistent handling of texture mutex failure

[Markus Armbruster]

We report d3d_texture2d_acquire0() and d3d_texture2d_release0()
failure as error, except in dbus_update_gl_cb(), where we report it as
warning.  Report it as error there as well.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 09d7a319b1..b82e7c7115 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -214,26 +214,31 @@ static void dbus_update_gl_cb(GObject *source_object,
                               GAsyncResult *res,
                               gpointer user_data)
 {
-    g_autoptr(GError) err = NULL;
+    g_autoptr(GError) gerr = NULL;
+#ifdef WIN32
+    Error **err = NULL;
+#endif
     DBusDisplayListener *ddl = user_data;
     bool success;
 
 #ifdef CONFIG_GBM
     success = qemu_dbus_display1_listener_call_update_dmabuf_finish(
-        ddl->proxy, res, &err);
+        ddl->proxy, res, &gerr);
     if (!success) {
-        error_report("Failed to call update: %s", err->message);
+        error_report("Failed to call update: %s", gerr->message);
     }
 #endif
 
 #ifdef WIN32
     success = qemu_dbus_display1_listener_win32_d3d11_call_update_texture2d_finish(
-        ddl->d3d11_proxy, res, &err);
+        ddl->d3d11_proxy, res, &gerr);
     if (!success) {
-        error_report("Failed to call update: %s", err->message);
+        error_report("Failed to call update: %s", gerr->message);
     }
 
-    d3d_texture2d_acquire0(ddl->d3d_texture, &error_warn);
+    if (!d3d_texture2d_acquire0(ddl->d3d_texture, &err)) {
+        error_report_err(err);
+    }
 #endif
 
     graphic_hw_gl_block(ddl->dcl.con, false);
-- 
2.49.0

[PATCH v3 12/13] ivshmem-flat: Mark an instance of missing error handling FIXME

[Markus Armbruster]

ivshmem-flat's ivshmem_flat_add_vector() neglects to handle
qemu_set_blocking() failure.  It used to silently ignore errors there.
Recent commit 6f607941b1c (treewide: use qemu_set_blocking instead of
g_unix_set_fd_nonblocking) changed it to warn (without mentioning it
the commit message, tsk, tsk, tsk).

Note that ivshmem-pci's process_msg_connect() handles this error.

Add a FIXME comment to mark the missing error handling.

Cc: Gustavo Romero <gustavo.romero@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem-flat.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem-flat.c b/hw/misc/ivshmem-flat.c
index e83e6c6ee9..27ee8c9218 100644
--- a/hw/misc/ivshmem-flat.c
+++ b/hw/misc/ivshmem-flat.c
@@ -138,6 +138,8 @@ static void ivshmem_flat_remove_peer(IvshmemFTState *s, uint16_t peer_id)
 static void ivshmem_flat_add_vector(IvshmemFTState *s, IvshmemPeer *peer,
                                     int vector_fd)
 {
+    Error *err = NULL;
+
     if (peer->vector_counter >= IVSHMEM_MAX_VECTOR_NUM) {
         trace_ivshmem_flat_add_vector_failure(peer->vector_counter,
                                               vector_fd, peer->id);
@@ -154,8 +156,10 @@ static void ivshmem_flat_add_vector(IvshmemFTState *s, IvshmemPeer *peer,
      * peer.
      */
     peer->vector[peer->vector_counter].id = peer->vector_counter;
-    /* WARNING: qemu_socket_set_nonblock() return code ignored */
-    qemu_set_blocking(vector_fd, false, &error_warn);
+    if (!qemu_set_blocking(vector_fd, false, &err)) {
+        /* FIXME handle the error */
+        warn_report_err(err);
+    }
     event_notifier_init_fd(&peer->vector[peer->vector_counter].event_notifier,
                            vector_fd);
 
-- 
2.49.0

[PATCH v3 13/13] error: Kill @error_warn

[Markus Armbruster]

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 seven as unclean or
otherwise undesirable, adding back five elsewhere.  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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/qapi/error.h           |  6 ------
 include/system/os-win32.h      |  5 ++++-
 hw/display/virtio-gpu.c        |  8 ++++++--
 hw/net/virtio-net.c            |  8 +++++++-
 io/channel-socket.c            |  4 ++--
 io/channel-watch.c             |  6 +++---
 tests/unit/test-error-report.c | 17 -----------------
 ui/gtk.c                       |  6 +++++-
 util/aio-win32.c               |  2 +-
 util/error.c                   |  5 +----
 util/oslib-win32.c             | 21 ++++++++++++++++++++-
 11 files changed, 49 insertions(+), 39 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/include/system/os-win32.h b/include/system/os-win32.h
index 3aa6cee4c2..22d72babdf 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -168,11 +168,14 @@ static inline void qemu_funlockfile(FILE *f)
 #endif
 }
 
-/* Helper for WSAEventSelect, to report errors */
+/* Helpers for WSAEventSelect() */
 bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
                         long lNetworkEvents, Error **errp);
+void qemu_socket_select_nofail(int sockfd, WSAEVENT hEventObject,
+                               long lNetworkEvents);
 
 bool qemu_socket_unselect(int sockfd, Error **errp);
+void qemu_socket_unselect_nofail(int sockfd);
 
 /* We wrap all the sockets functions so that we can set errno based on
  * WSAGetLastError(), and use file-descriptors instead of SOCKET.
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/io/channel-socket.c b/io/channel-socket.c
index fdc7670867..712b793eaf 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -454,7 +454,7 @@ static void qio_channel_socket_finalize(Object *obj)
             }
         }
 #ifdef WIN32
-        qemu_socket_unselect(ioc->fd, &error_warn);
+        qemu_socket_unselect_nofail(ioc->fd);
 #endif
         close(ioc->fd);
         ioc->fd = -1;
@@ -929,7 +929,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 
     if (sioc->fd != -1) {
 #ifdef WIN32
-        qemu_socket_unselect(sioc->fd, &error_warn);
+        qemu_socket_unselect_nofail(sioc->fd);
 #endif
         if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
             socket_listen_cleanup(sioc->fd, errp);
diff --git a/io/channel-watch.c b/io/channel-watch.c
index ec76bd1ec6..018648b36b 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -281,9 +281,9 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
     GSource *source;
     QIOChannelSocketSource *ssource;
 
-    qemu_socket_select(sockfd, ioc->event,
-                       FD_READ | FD_ACCEPT | FD_CLOSE |
-                       FD_CONNECT | FD_WRITE | FD_OOB, &error_warn);
+    qemu_socket_select_nofail(sockfd, ioc->event,
+                              FD_READ | FD_ACCEPT | FD_CLOSE |
+                              FD_CONNECT | FD_WRITE | FD_OOB);
 
     source = g_source_new(&qio_channel_socket_source_funcs,
                           sizeof(QIOChannelSocketSource));
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/aio-win32.c b/util/aio-win32.c
index b125924433..c6fbce64c2 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
 
         QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
         event = event_notifier_get_handle(&ctx->notifier);
-        qemu_socket_select(fd, event, bitmask, &error_warn);
+        qemu_socket_select_nofail(fd, event, bitmask);
     }
     if (old_node) {
         aio_remove_fd_handler(ctx, old_node);
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);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 6a2367c89d..84bc65a765 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -182,7 +182,7 @@ bool qemu_set_blocking(int fd, bool block, Error **errp)
     unsigned long opt = block ? 0 : 1;
 
     if (block) {
-        qemu_socket_unselect(fd, &error_warn);
+        qemu_socket_unselect_nofail(fd);
     }
 
     if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) {
@@ -311,6 +311,25 @@ bool qemu_socket_unselect(int sockfd, Error **errp)
     return qemu_socket_select(sockfd, NULL, 0, errp);
 }
 
+void qemu_socket_select_nofail(int sockfd, WSAEVENT hEventObject,
+                               long lNetworkEvents)
+{
+    Error *err = NULL;
+
+    if (!qemu_socket_select(sockfd, hEventObject, lNetworkEvents, &err)) {
+        warn_report_err(err);
+    }
+}
+
+void qemu_socket_unselect_nofail(int sockfd)
+{
+    Error *err = NULL;
+
+    if (!qemu_socket_unselect(sockfd, &err)) {
+        warn_report_err(err);
+    }
+}
+
 int qemu_socketpair(int domain, int type, int protocol, int sv[2])
 {
     struct sockaddr_un addr = {
-- 
2.49.0

Re: [PATCH v3 08/13] util/oslib-win32: Do not treat null @errp as &error_warn

[Philippe Mathieu-Daudé]

On 23/9/25 11:09, Markus Armbruster wrote:
> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
> null @errp as &error_warn.  This is wildly inappropriate.  A caller
> passing null @errp specifies that errors are to be ignored.  If
> warnings are wanted, the caller must pass &error_warn.
> 
> Change callers to do that, and drop the inappropriate treatment of
> null @errp.
> 
> This assumes that warnings are wanted.  I'm not familiar with the
> calling code, so I can't say whether it will work when the socket is
> invalid, or WSAEventSelect() fails.  If it doesn't, then this should
> be an error instead of a warning.  Invalid socket might even be a
> programming error.
> 
> These warnings were introduced in commit f5fd677ae7cf (win32/socket:
> introduce qemu_socket_select() helper).  I considered reverting to
> silence, but Daniel Berrangé asked for the warnings to be preserved.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   io/channel-socket.c | 4 ++--
>   io/channel-watch.c  | 2 +-
>   util/aio-win32.c    | 2 +-
>   util/oslib-win32.c  | 6 +-----
>   4 files changed, 5 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 10/13] ui/dbus: Clean up dbus_update_gl_cb() error checking

[Philippe Mathieu-Daudé]

On 23/9/25 11:09, Markus Armbruster wrote:
>  From GLib "Rules for use of GError":
> 
>      A GError* must be initialized to NULL before passing its address
>      to a function that can report errors.
> 
> dbus_update_gl_cb() seemingly violates this rule: it passes &err to
> qemu_dbus_display1_listener_call_update_dmabuf_finish() and to
> qemu_dbus_display1_listener_win32_d3d11_call_update_texture2d_finish()
> without clearing it in between.  Harmless, because the first call is
> guarded by #ifdef CONFIG_GBM, the second by #ifdef WIN32, and the two
> are mutually exclusive.  I think.
> 
> Clean this up to be obviously correct.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus-listener.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Philippe Mathieu-Daudé]

On 23/9/25 11:09, Markus Armbruster wrote:
> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
> ...) when auto-shutdown is enabled, else with error_report().
> 
> Issues:
> 
> 1. The error is serious enough to warrant aborting the process when
> auto-shutdown is enabled, yet harmless enough to permit carrying on
> when it's disabled.  This makes no sense to me.
> 
> 2. Like assert(), &error_abort is strictly for programming errors.  Is
> this one?  Or should we exit(1) instead?
> 
> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
> assert()."
> 
> This patch addresses just 3.
> 
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/remote/vfio-user-obj.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 02/13] tcg: Fix error reporting on mprotect() failure in tcg_region_init()

[Philippe Mathieu-Daudé]

On 23/9/25 11:09, Markus Armbruster wrote:
> tcg_region_init() calls one of qemu_mprotect_rwx(),
> qemu_mprotect_rw(), and mprotect(), then reports failure with
> error_setg_errno(&error_fatal, errno, ...).
> 
> The use of &error_fatal is undesirable.  qapi/error.h advises:
> 
>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>   * exit(), because that's more obvious.
> 
> The use of errno is wrong.  qemu_mprotect_rwx() and qemu_mprotect_rw()
> wrap around qemu_mprotect__osdep().  qemu_mprotect__osdep() calls
> mprotect() on POSIX, VirtualProtect() on Windows, and reports failure
> with error_report().  VirtualProtect() doesn't set errno.  mprotect()
> does, but error_report() may clobber it.
> 
> Fix tcg_region_init() to report errors only when it calls mprotect(),
> and rely on qemu_mprotect_rwx()'s and qemu_mprotect_rw()'s error
> reporting otherwise.  Use error_report(), not error_setg().
> 
> Fixes: 22c6a9938f75 (tcg: Merge buffer protection and guard page protection)
> Fixes: 6bc144237a85 (tcg: Use Error with alloc_code_gen_buffer)
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/region.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tcg/region.c b/tcg/region.c
> index 7ea0b37a84..2181267e48 100644
> --- a/tcg/region.c
> +++ b/tcg/region.c
> @@ -832,13 +832,16 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_threads)
>               } else {
>   #ifdef CONFIG_POSIX
>                   rc = mprotect(start, end - start, need_prot);
> +                if (rc) {
> +                    error_report("mprotect of jit buffer: %s",
> +                                 strerror(errno));

I'm not keen on handling errors differently in the same function.

qemu_mprotect_rwx() and qemu_mprotect_rw() already print the error.

Why not add qemu_mprotect() as a simple qemu_mprotect__osdep() alias,
then call it here, also covering the non-POSIX case?
(Question for Richard, after looking at commits 22c6a9938f7 and more
  importantly 97a83753c9 -- wondering about WoA).

> +                }
>   #else
>                   g_assert_not_reached();
>   #endif
>               }
>               if (rc) {
> -                error_setg_errno(&error_fatal, errno,
> -                                 "mprotect of jit buffer");
> +                exit(1);
>               }>           }
>           if (have_prot != 0) {

Re: [PATCH v3 01/13] monitor: Clean up HMP gdbserver error reporting

[Philippe Mathieu-Daudé]

On 23/9/25 11:09, Markus Armbruster wrote:
> HMP command gdbserver used to emit two error messages for certain
> errors.  For instance, with -M none:
> 
>      (qemu) gdbserver
>      gdbstub: meaningless to attach gdb to a machine without any CPU.
>      Could not open gdbserver on device 'tcp::1234'

Orthogonal to this patch: interesting. This comes from commit
508b4ecc393 ("gdbstub.c: fix GDB connection segfault caused by empty
machines"). This feels wrong to me, as it is OK to use a jtag probe
to access memory or program a flash, even without any cpu online.

Yet another side-effect of use of the ill-first_cpu global variable.

Back to this patch description, probably not the best error example =)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> The first message is the specific error, and the second one a generic
> additional message that feels superfluous to me.
> 
> Commit c0e6b8b798b (system: propagate Error to gdbserver_start (and
> other device setups)) turned the first message into a warning:
> 
>      warning: gdbstub: meaningless to attach gdb to a machine without any CPU.
>      Could not open gdbserver on device 'tcp::1234'
> 
> This is arguably worse.
> 
> hmp_gdbserver() passes &error_warn to gdbserver_start(), so that
> failure gets reported as warning, and then additionally emits the
> generic error on failure.  This is a misuse of &error_warn.
> 
> Instead, receive the error in &err and report it, as usual.  With
> this, gdbserver reports just the error:
> 
>      gdbstub: meaningless to attach gdb to a machine without any CPU.
> 
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   include/exec/gdbstub.h | 3 ---
>   monitor/hmp-cmds.c     | 7 ++++---
>   2 files changed, 4 insertions(+), 6 deletions(-)

Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Vladimir Sementsov-Ogievskiy]

On 23.09.25 12:09, Markus Armbruster wrote:
> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
> ...) when auto-shutdown is enabled, else with error_report().
> 
> Issues:
> 
> 1. The error is serious enough to warrant aborting the process when
> auto-shutdown is enabled, yet harmless enough to permit carrying on
> when it's disabled.  This makes no sense to me.
> 
> 2. Like assert(), &error_abort is strictly for programming errors.  Is
> this one?

Brief look at the code make me think that, no it isn't.

>  Or should we exit(1) instead?
> 
> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
> assert()."
> 
> This patch addresses just 3.
> 
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/remote/vfio-user-obj.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index ea6165ebdc..eb96982a3a 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>    */
>   #define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
>       {                                                                     \
> -        if (vfu_object_auto_shutdown()) {                                 \
> -            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
> -        } else {                                                          \
> -            error_report((fmt), ## __VA_ARGS__);                          \
> -        }                                                                 \
> -    }                                                                     \
> +        error_report((fmt), ## __VA_ARGS__);                              \
> +        assert(!vfu_object_auto_shutdown());                              \

Probably, it's only my feeling, but for me, assert() is really strictly bound
to programming errors, more than abort(). Using abort() for errors which are
not programming, but we can't handle them looks less confusing, i.e.

if (vfu_object_auto_shutdown()) {
     abort();
}

Not really matter. Anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> +    }
>   
>   struct VfuObjectClass {
>       ObjectClass parent_class;


-- 
Best regards,
Vladimir

Re: [PATCH v3 12/13] ivshmem-flat: Mark an instance of missing error handling FIXME

[Vladimir Sementsov-Ogievskiy]

On 23.09.25 12:09, Markus Armbruster wrote:
> ivshmem-flat's ivshmem_flat_add_vector() neglects to handle
> qemu_set_blocking() failure.  It used to silently ignore errors there.
> Recent commit 6f607941b1c (treewide: use qemu_set_blocking instead of
> g_unix_set_fd_nonblocking) changed it to warn (without mentioning it
> the commit message, tsk, tsk, tsk).

Yes, my fault.
> 
> Note that ivshmem-pci's process_msg_connect() handles this error.
> 
> Add a FIXME comment to mark the missing error handling.
> 
> Cc: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   hw/misc/ivshmem-flat.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/ivshmem-flat.c b/hw/misc/ivshmem-flat.c
> index e83e6c6ee9..27ee8c9218 100644
> --- a/hw/misc/ivshmem-flat.c
> +++ b/hw/misc/ivshmem-flat.c
> @@ -138,6 +138,8 @@ static void ivshmem_flat_remove_peer(IvshmemFTState *s, uint16_t peer_id)
>   static void ivshmem_flat_add_vector(IvshmemFTState *s, IvshmemPeer *peer,
>                                       int vector_fd)
>   {
> +    Error *err = NULL;
> +
>       if (peer->vector_counter >= IVSHMEM_MAX_VECTOR_NUM) {
>           trace_ivshmem_flat_add_vector_failure(peer->vector_counter,
>                                                 vector_fd, peer->id);
> @@ -154,8 +156,10 @@ static void ivshmem_flat_add_vector(IvshmemFTState *s, IvshmemPeer *peer,
>        * peer.
>        */
>       peer->vector[peer->vector_counter].id = peer->vector_counter;
> -    /* WARNING: qemu_socket_set_nonblock() return code ignored */
> -    qemu_set_blocking(vector_fd, false, &error_warn);
> +    if (!qemu_set_blocking(vector_fd, false, &err)) {
> +        /* FIXME handle the error */
> +        warn_report_err(err);
> +    }
>       event_notifier_init_fd(&peer->vector[peer->vector_counter].event_notifier,
>                              vector_fd);
>   


-- 
Best regards,
Vladimir

Re: [PATCH v3 01/13] monitor: Clean up HMP gdbserver error reporting

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 23/9/25 11:09, Markus Armbruster wrote:
>> HMP command gdbserver used to emit two error messages for certain
>> errors.  For instance, with -M none:
>>      (qemu) gdbserver
>>      gdbstub: meaningless to attach gdb to a machine without any CPU.
>>      Could not open gdbserver on device 'tcp::1234'
>
> Orthogonal to this patch: interesting. This comes from commit
> 508b4ecc393 ("gdbstub.c: fix GDB connection segfault caused by empty
> machines"). This feels wrong to me, as it is OK to use a jtag probe
> to access memory or program a flash, even without any cpu online.
>
> Yet another side-effect of use of the ill-first_cpu global variable.
>
> Back to this patch description, probably not the best error example =)

I picked it because how to trigger it was obvious to me.

Happy to use another one if you show it to me :)

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Re: [PATCH v3 02/13] tcg: Fix error reporting on mprotect() failure in tcg_region_init()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 23/9/25 11:09, Markus Armbruster wrote:
>> tcg_region_init() calls one of qemu_mprotect_rwx(),
>> qemu_mprotect_rw(), and mprotect(), then reports failure with
>> error_setg_errno(&error_fatal, errno, ...).
>>
>> The use of &error_fatal is undesirable.  qapi/error.h advises:
>>
>>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>>   * exit(), because that's more obvious.
>>
>> The use of errno is wrong.  qemu_mprotect_rwx() and qemu_mprotect_rw()
>> wrap around qemu_mprotect__osdep().  qemu_mprotect__osdep() calls
>> mprotect() on POSIX, VirtualProtect() on Windows, and reports failure
>> with error_report().  VirtualProtect() doesn't set errno.  mprotect()
>> does, but error_report() may clobber it.
>>
>> Fix tcg_region_init() to report errors only when it calls mprotect(),
>> and rely on qemu_mprotect_rwx()'s and qemu_mprotect_rw()'s error
>> reporting otherwise.  Use error_report(), not error_setg().
>>
>> Fixes: 22c6a9938f75 (tcg: Merge buffer protection and guard page protection)
>> Fixes: 6bc144237a85 (tcg: Use Error with alloc_code_gen_buffer)
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   tcg/region.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>> diff --git a/tcg/region.c b/tcg/region.c
>> index 7ea0b37a84..2181267e48 100644
>> --- a/tcg/region.c
>> +++ b/tcg/region.c
>> @@ -832,13 +832,16 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_threads)
>>              } else {
>>  #ifdef CONFIG_POSIX
>>                  rc = mprotect(start, end - start, need_prot);
>> +                if (rc) {
>> +                    error_report("mprotect of jit buffer: %s",
>> +                                 strerror(errno));
>
> I'm not keen on handling errors differently in the same function.
>
> qemu_mprotect_rwx() and qemu_mprotect_rw() already print the error.

Yes: they call qemu_mprotect__osdep(), which uses error_report().

> Why not add qemu_mprotect() as a simple qemu_mprotect__osdep() alias,
> then call it here, also covering the non-POSIX case?
> (Question for Richard, after looking at commits 22c6a9938f7 and more
>  importantly 97a83753c9 -- wondering about WoA).

There is no commit 97a83753c9.  Do you mean a97a83753c9?

I'd like to merge this commit as is.  It's a minimal fix, and it's been
reviewed.  We can always improve on top.

>> +                }
>>  #else
>>                  g_assert_not_reached();
>>  #endif
>>              }
>>              if (rc) {
>> -                error_setg_errno(&error_fatal, errno,
>> -                                 "mprotect of jit buffer");
>> +                exit(1);
>>              }
>>           }
>>           if (have_prot != 0) {

Re: [PATCH v3 02/13] tcg: Fix error reporting on mprotect() failure in tcg_region_init()

[Philippe Mathieu-Daudé]

On 23/9/25 13:16, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 23/9/25 11:09, Markus Armbruster wrote:
>>> tcg_region_init() calls one of qemu_mprotect_rwx(),
>>> qemu_mprotect_rw(), and mprotect(), then reports failure with
>>> error_setg_errno(&error_fatal, errno, ...).
>>>
>>> The use of &error_fatal is undesirable.  qapi/error.h advises:
>>>
>>>    * Please don't error_setg(&error_fatal, ...), use error_report() and
>>>    * exit(), because that's more obvious.
>>>
>>> The use of errno is wrong.  qemu_mprotect_rwx() and qemu_mprotect_rw()
>>> wrap around qemu_mprotect__osdep().  qemu_mprotect__osdep() calls
>>> mprotect() on POSIX, VirtualProtect() on Windows, and reports failure
>>> with error_report().  VirtualProtect() doesn't set errno.  mprotect()
>>> does, but error_report() may clobber it.
>>>
>>> Fix tcg_region_init() to report errors only when it calls mprotect(),
>>> and rely on qemu_mprotect_rwx()'s and qemu_mprotect_rw()'s error
>>> reporting otherwise.  Use error_report(), not error_setg().
>>>
>>> Fixes: 22c6a9938f75 (tcg: Merge buffer protection and guard page protection)
>>> Fixes: 6bc144237a85 (tcg: Use Error with alloc_code_gen_buffer)
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    tcg/region.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>> diff --git a/tcg/region.c b/tcg/region.c
>>> index 7ea0b37a84..2181267e48 100644
>>> --- a/tcg/region.c
>>> +++ b/tcg/region.c
>>> @@ -832,13 +832,16 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned max_threads)
>>>               } else {
>>>   #ifdef CONFIG_POSIX
>>>                   rc = mprotect(start, end - start, need_prot);
>>> +                if (rc) {
>>> +                    error_report("mprotect of jit buffer: %s",
>>> +                                 strerror(errno));
>>
>> I'm not keen on handling errors differently in the same function.
>>
>> qemu_mprotect_rwx() and qemu_mprotect_rw() already print the error.
> 
> Yes: they call qemu_mprotect__osdep(), which uses error_report().
> 
>> Why not add qemu_mprotect() as a simple qemu_mprotect__osdep() alias,
>> then call it here, also covering the non-POSIX case?
>> (Question for Richard, after looking at commits 22c6a9938f7 and more
>>   importantly 97a83753c9 -- wondering about WoA).
> 
> There is no commit 97a83753c9.  Do you mean a97a83753c9?

Yes.

> I'd like to merge this commit as is.  It's a minimal fix, and it's been
> reviewed.  We can always improve on top.

Right (we'd need to map non-POSIX protection bits anyway).

No objection!

Re: [PATCH v3 00/13] Error reporting cleanup, a fix, and &error_warn removal

[Akihiko Odaki]

On 2025/09/23 18:09, Markus Armbruster wrote:
>>From qapi/error.h:
> 
>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>   * exit(), because that's more obvious.
>   * Likewise, don't error_setg(&error_abort, ...), use assert().
> 
> Not mentioned, but just as undesirable: error_setg(&error_warn, ...).
> 
> This series eliminates such uses, and gets rid of &error_warn.
> &error_warn has multiple issues and little use.  PATCH 12 has full
> rationale.
> 
> A note on warnings: we don't use warnings much, and when we use them,
> they're often pretty bad.  See my memo "Abuse of warnings for
> unhandled errors and programming errors"
> Message-ID: <87h5yijh3b.fsf@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87h5yijh3b.fsf@pond.sub.org/
> 
> v3:
> * PATCH 02: Whitespace cleanup [Akihiko Odaki]
> * PATCH 06+07: Memory leaks on error paths [Akihiko Odaki]
> * PATCH 08+13: Rebase conflicts
> * PATCH 12: New
> 
> v2:
> * PATCH 03: Mention change of cxl_fmws_link() return value in commit
>    message [Jonathan]
> * PATCH 04: Change exit(1) to g_assert_not_reached(), because it's a
>    programming error.
> * PATCH 06+07: Replace questions in commit message by answers from
>    review.
> * PATCH 06: Fix a format string
> * PATCH 08: Keep warnings instead of reverting to silence [Daniel]
> * PATCH 12: Adjusted for replaced PATCH 08
> 
> Issues raised in review I decided not to address in this series:
> * PATCH 03: messages could be improved further, in particular the
>    "gdbstub: " prefix could be dropped
> * ebpf_rss_load() can return false without setting an error
> * Capture the discussion on how to deal with undhandled errors in
>    cover letter and/or commit messages.
> 
> The first two could be done on top.
> 
> Markus Armbruster (13):
>    monitor: Clean up HMP gdbserver error reporting
>    tcg: Fix error reporting on mprotect() failure in  tcg_region_init()
>    hw/cxl: Convert cxl_fmws_link() to Error
>    migration/cpr: Clean up error reporting in cpr_resave_fd()
>    hw/remote/vfio-user: Clean up error reporting
>    net/slirp: Clean up error reporting
>    ui/spice-core: Clean up error reporting
>    util/oslib-win32: Do not treat null @errp as &error_warn
>    ui/pixman: Consistent error handling in qemu_pixman_shareable_free()
>    ui/dbus: Clean up dbus_update_gl_cb() error checking
>    ui/dbus: Consistent handling of texture mutex failure
>    ivshmem-flat: Mark an instance of missing error handling FIXME
>    error: Kill @error_warn
> 
>   include/exec/gdbstub.h         |  3 ---
>   include/qapi/error.h           |  6 ------
>   include/system/os-win32.h      |  5 ++++-
>   hw/cxl/cxl-host.c              |  7 ++++---
>   hw/display/virtio-gpu.c        |  8 ++++++--
>   hw/misc/ivshmem-flat.c         |  8 ++++++--
>   hw/net/virtio-net.c            |  8 +++++++-
>   hw/remote/vfio-user-obj.c      |  9 +++------
>   io/channel-socket.c            |  4 ++--
>   io/channel-watch.c             |  6 +++---
>   migration/cpr.c                |  9 +++++----
>   monitor/hmp-cmds.c             |  7 ++++---
>   net/slirp.c                    |  9 +++++++--
>   tcg/region.c                   |  7 +++++--
>   tests/unit/test-error-report.c | 17 -----------------
>   ui/dbus-listener.c             | 22 +++++++++++++++-------
>   ui/gtk.c                       |  6 +++++-
>   ui/qemu-pixman.c               |  5 ++++-
>   ui/spice-core.c                |  6 ++++--
>   util/aio-win32.c               |  2 +-
>   util/error.c                   |  5 +----
>   util/oslib-win32.c             | 25 ++++++++++++++++++++-----
>   22 files changed, 106 insertions(+), 78 deletions(-)
> 

For the whole series:

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 23.09.25 12:09, Markus Armbruster wrote:
>> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
>> ...) when auto-shutdown is enabled, else with error_report().
>>
>> Issues:
>>
>> 1. The error is serious enough to warrant aborting the process when
>> auto-shutdown is enabled, yet harmless enough to permit carrying on
>> when it's disabled.  This makes no sense to me.
>>
>> 2. Like assert(), &error_abort is strictly for programming errors.  Is
>> this one?
>
> Brief look at the code make me think that, no it isn't.

So the use of &error_abort is wrong.

>>  Or should we exit(1) instead?
>>
>> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>> assert()."
>>
>> This patch addresses just 3.
>>
>> Cc: Jagannathan Raman <jag.raman@oracle.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   hw/remote/vfio-user-obj.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> index ea6165ebdc..eb96982a3a 100644
>> --- a/hw/remote/vfio-user-obj.c
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>>    */
>>   #define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
>>       {                                                                     \
>> -        if (vfu_object_auto_shutdown()) {                                 \
>> -            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
>> -        } else {                                                          \
>> -            error_report((fmt), ## __VA_ARGS__);                          \
>> -        }                                                                 \
>> -    }                                                                     \
>> +        error_report((fmt), ## __VA_ARGS__);                              \
>> +        assert(!vfu_object_auto_shutdown());                              \
>
> Probably, it's only my feeling, but for me, assert() is really strictly bound
> to programming errors, more than abort(). Using abort() for errors which are
> not programming, but we can't handle them looks less confusing, i.e.
>
> if (vfu_object_auto_shutdown()) {
>     abort();
> }

assert(COND) is if (COND) abort() plus a message meant to help
developers.  Both are for programming errors.  If it isn't something
that needs debugging, why dump core?

But this particular error condition is *not* a programming error.  So

        assert(!vfu_object_auto_shutdown());

and

        if (vfu_object_auto_shutdown()) {
            abort();
        }

are both equally wrong.  However, the latter makes it easier to add a
FIXME comment:

        if (vfu_object_auto_shutdown()) {
            /*
             * FIXME This looks inappropriate.  The error is serious
             * enough programming error to warrant aborting the process
             * when auto-shutdown is enabled, yet harmless enough to
             * permit carrying on when it's disabled.  Makes no sense.
             */
            abort();
        }

The commit message would then need a tweak.  Perhaps

  Issues:

  1. The error is serious enough to warrant killing the process when
  auto-shutdown is enabled, yet harmless enough to permit carrying on
  when it's disabled.  This makes no sense to me.

  2. Like assert(), &error_abort is strictly for programming errors.  Is
  this one?  Vladimir Sementsov-Ogievskiy tells me it's not.

  3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
  assert()."

  This patch addresses just 3.  It adds a FIXME comment for the other
  two.

Thoughts?

> Not really matter. Anyway:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks!

>> +    }
>>     struct VfuObjectClass {
>>       ObjectClass parent_class;

Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Vladimir Sementsov-Ogievskiy]

On 26.09.25 09:51, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 23.09.25 12:09, Markus Armbruster wrote:
>>> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
>>> ...) when auto-shutdown is enabled, else with error_report().
>>>
>>> Issues:
>>>
>>> 1. The error is serious enough to warrant aborting the process when
>>> auto-shutdown is enabled, yet harmless enough to permit carrying on
>>> when it's disabled.  This makes no sense to me.
>>>
>>> 2. Like assert(), &error_abort is strictly for programming errors.  Is
>>> this one?
>>
>> Brief look at the code make me think that, no it isn't.
> 
> So the use of &error_abort is wrong.
> 
>>>   Or should we exit(1) instead?
>>>
>>> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>>> assert()."
>>>
>>> This patch addresses just 3.
>>>
>>> Cc: Jagannathan Raman <jag.raman@oracle.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    hw/remote/vfio-user-obj.c | 9 +++------
>>>    1 file changed, 3 insertions(+), 6 deletions(-)
>>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>>> index ea6165ebdc..eb96982a3a 100644
>>> --- a/hw/remote/vfio-user-obj.c
>>> +++ b/hw/remote/vfio-user-obj.c
>>> @@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>>>     */
>>>    #define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
>>>        {                                                                     \
>>> -        if (vfu_object_auto_shutdown()) {                                 \
>>> -            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
>>> -        } else {                                                          \
>>> -            error_report((fmt), ## __VA_ARGS__);                          \
>>> -        }                                                                 \
>>> -    }                                                                     \
>>> +        error_report((fmt), ## __VA_ARGS__);                              \
>>> +        assert(!vfu_object_auto_shutdown());                              \
>>
>> Probably, it's only my feeling, but for me, assert() is really strictly bound
>> to programming errors, more than abort(). Using abort() for errors which are
>> not programming, but we can't handle them looks less confusing, i.e.
>>
>> if (vfu_object_auto_shutdown()) {
>>      abort();
>> }
> 
> assert(COND) is if (COND) abort() plus a message meant to help
> developers.  Both are for programming errors.  If it isn't something
> that needs debugging, why dump core?
> 
> But this particular error condition is *not* a programming error.  So
> 
>          assert(!vfu_object_auto_shutdown());
> 
> and
> 
>          if (vfu_object_auto_shutdown()) {
>              abort();
>          }
> 
> are both equally wrong.  However, the latter makes it easier to add a
> FIXME comment:
> 
>          if (vfu_object_auto_shutdown()) {
>              /*
>               * FIXME This looks inappropriate.  The error is serious
>               * enough programming error to warrant aborting the process
>               * when auto-shutdown is enabled, yet harmless enough to
>               * permit carrying on when it's disabled.  Makes no sense.
>               */
>              abort();
>          }
> 

Looks more readable, yes.

> The commit message would then need a tweak.  Perhaps
> 
>    Issues:
> 
>    1. The error is serious enough to warrant killing the process when
>    auto-shutdown is enabled, yet harmless enough to permit carrying on
>    when it's disabled.  This makes no sense to me.
> 
>    2. Like assert(), &error_abort is strictly for programming errors.  Is
>    this one?  Vladimir Sementsov-Ogievskiy tells me it's not.

:)) I'm not an expert in vfio-user at all. But yes, I said it:)

> 
>    3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>    assert()."
> 
>    This patch addresses just 3.  It adds a FIXME comment for the other
>    two.
> 
> Thoughts?

Looks good.

> 
>> Not really matter. Anyway:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Thanks!
> 
>>> +    }
>>>      struct VfuObjectClass {
>>>        ObjectClass parent_class;
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 05/13] hw/remote/vfio-user: Clean up error reporting

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 26.09.25 09:51, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> On 23.09.25 12:09, Markus Armbruster wrote:
>>>> VFU_OBJECT_ERROR() reports the error with error_setg(&error_abort,
>>>> ...) when auto-shutdown is enabled, else with error_report().
>>>>
>>>> Issues:
>>>>
>>>> 1. The error is serious enough to warrant aborting the process when
>>>> auto-shutdown is enabled, yet harmless enough to permit carrying on
>>>> when it's disabled.  This makes no sense to me.
>>>>
>>>> 2. Like assert(), &error_abort is strictly for programming errors.  Is
>>>> this one?
>>>
>>> Brief look at the code make me think that, no it isn't.
>> So the use of &error_abort is wrong.
>> 
>>>>   Or should we exit(1) instead?
>>>>
>>>> 3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>>>> assert()."
>>>>
>>>> This patch addresses just 3.
>>>>
>>>> Cc: Jagannathan Raman <jag.raman@oracle.com>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    hw/remote/vfio-user-obj.c | 9 +++------
>>>>    1 file changed, 3 insertions(+), 6 deletions(-)
>>>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>>>> index ea6165ebdc..eb96982a3a 100644
>>>> --- a/hw/remote/vfio-user-obj.c
>>>> +++ b/hw/remote/vfio-user-obj.c
>>>> @@ -75,12 +75,9 @@ OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>>>>     */
>>>>    #define VFU_OBJECT_ERROR(o, fmt, ...)                                     \
>>>>        {                                                                     \
>>>> -        if (vfu_object_auto_shutdown()) {                                 \
>>>> -            error_setg(&error_abort, (fmt), ## __VA_ARGS__);              \
>>>> -        } else {                                                          \
>>>> -            error_report((fmt), ## __VA_ARGS__);                          \
>>>> -        }                                                                 \
>>>> -    }                                                                     \
>>>> +        error_report((fmt), ## __VA_ARGS__);                              \
>>>> +        assert(!vfu_object_auto_shutdown());                              \
>>>
>>> Probably, it's only my feeling, but for me, assert() is really strictly bound
>>> to programming errors, more than abort(). Using abort() for errors which are
>>> not programming, but we can't handle them looks less confusing, i.e.
>>>
>>> if (vfu_object_auto_shutdown()) {
>>>      abort();
>>> }
>>
>> assert(COND) is if (COND) abort() plus a message meant to help
>> developers.  Both are for programming errors.  If it isn't something
>> that needs debugging, why dump core?
>>
>> But this particular error condition is *not* a programming error.  So
>>          assert(!vfu_object_auto_shutdown());
>>
>> and
>>
>>          if (vfu_object_auto_shutdown()) {
>>              abort();
>>          }
>>
>> are both equally wrong.  However, the latter makes it easier to add a
>> FIXME comment:
>>
>>          if (vfu_object_auto_shutdown()) {
>>              /*
>>               * FIXME This looks inappropriate.  The error is serious
>>               * enough programming error to warrant aborting the process
>>               * when auto-shutdown is enabled, yet harmless enough to
>>               * permit carrying on when it's disabled.  Makes no sense.
>>               */
>>              abort();
>>          }
>> 
>
> Looks more readable, yes.

Sold!

>> The commit message would then need a tweak.  Perhaps
>>
>>    Issues:
>>
>>    1. The error is serious enough to warrant killing the process when
>>    auto-shutdown is enabled, yet harmless enough to permit carrying on
>>    when it's disabled.  This makes no sense to me.
>>
>>    2. Like assert(), &error_abort is strictly for programming errors.  Is
>>    this one?  Vladimir Sementsov-Ogievskiy tells me it's not.
>
> :)) I'm not an expert in vfio-user at all. But yes, I said it:)

I'll soften it to "believes it's not."

>>    3. qapi/error.h advises "don't error_setg(&error_abort, ...), use
>>    assert()."
>>
>>    This patch addresses just 3.  It adds a FIXME comment for the other
>>    two.
>>
>> Thoughts?
>
> Looks good.

Thanks again!

>> 
>>> Not really matter. Anyway:
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Thanks!
>> 
>>>> +    }
>>>>      struct VfuObjectClass {
>>>>        ObjectClass parent_class;
>>