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

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

[Markus Armbruster]

Markus Armbruster (12):
  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: Revert warning on WSAEventSelect() failure
  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
  error: Kill @error_warn

 include/exec/gdbstub.h         |  3 ---
 include/qapi/error.h           |  6 ------
 hw/cxl/cxl-host.c              |  7 ++++---
 hw/display/virtio-gpu.c        |  8 ++++++--
 hw/net/virtio-net.c            |  8 +++++++-
 hw/remote/vfio-user-obj.c      |  9 +++------
 migration/cpr.c                |  9 +++++----
 monitor/hmp-cmds.c             |  7 ++++---
 net/slirp.c                    |  6 ++++--
 tcg/region.c                   |  8 ++++++--
 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                |  3 ++-
 util/error.c                   |  5 +----
 util/oslib-win32.c             |  4 ----
 17 files changed, 66 insertions(+), 67 deletions(-)

-- 
2.49.0

[PATCH 01/12] 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>
---
 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

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

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 10:08:12AM +0200, 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'
> 
> 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.

What do you think about an alternative of removing "gdbstub: "
from all the errors raised in  'gdbserver_start' & similar, and
then using

  error_prepend(err, "Could not open gdbserver on device '%s'", device);

in hmp_gdbserver ?

I don't feel too strongly, so in any case for the patch as is,

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@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
> 
> 

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 :|

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 10:08:12AM +0200, 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'
>> 
>> 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.
>
> What do you think about an alternative of removing "gdbstub: "
> from all the errors raised in  'gdbserver_start' & similar, and
> then using
>
>   error_prepend(err, "Could not open gdbserver on device '%s'", device);
>
> in hmp_gdbserver ?

This would change the error message from

    (qemu) gdbserver 
    gdbstub: meaningless to attach gdb to a machine without any CPU.

to

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

Longer, but doesn't tell me anything new beyond reminding me that
gdbserver's argument defaults to "tcp::1234".

This instance of gdbserver_start() is additionally called by
qemu_machine_creation_done() in system/vl.c on behalf of command line
option gdb.  The error message would change from

    $ qemu-system-x86_64 -M none -gdb ""
    qemu-system-x86_64: -gdb : gdbstub: meaningless to attach gdb to a machine without any CPU.

to

    $ qemu-system-x86_64 -M none -gdb ""
    qemu-system-x86_64: -gdb : meaningless to attach gdb to a machine without any CPU.

Improvement.  Howver, we better drop the prefix from all error messages
emitted here, not just this one.

There's another instance of gdbserver_start() in gdbstub/user.c, called
by main() in bsduser/main.c and in linux-user/main.c on behalf of
command line option -g / -gdb.  Same silly prefix:

    $ qemu-x86_64 -g x,y /bin/ls
    qemu-x86_64: gdbstub: unknown option "y"
    Usage: -g {port|path}[,suspend={y|n}]

> I don't feel too strongly, so in any case for the patch as is,

Separate patch to strip "gdbstub: " from all error messages?

> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!

[PATCH 02/12] 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>
---
 tcg/region.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tcg/region.c b/tcg/region.c
index 7ea0b37a84..74e3b4b774 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -832,13 +832,17 @@ 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

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

[Philippe Mathieu-Daudé]

Hi Markus,

On 8/8/25 10:08, 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>
> ---
>   tcg/region.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tcg/region.c b/tcg/region.c
> index 7ea0b37a84..74e3b4b774 100644
> --- a/tcg/region.c
> +++ b/tcg/region.c
> @@ -832,13 +832,17 @@ 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);

- Before:

Error displayed when qemu_mprotect_rwx/qemu_mprotect_rw/mprotect fail,
then exit.

- After:

Error only displayed when mprotect() fails, then exit.
Nothing displayed when qemu_mprotect_rwx() or qemu_mprotect_rw() failed,
and exit.

Can we keep the "mprotect of jit buffer" string displayed before
exiting, but using error_report()?

>               }
>           }
>           if (have_prot != 0) {

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

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 04:00:42PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> On 8/8/25 10:08, 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>
> > ---
> >   tcg/region.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tcg/region.c b/tcg/region.c
> > index 7ea0b37a84..74e3b4b774 100644
> > --- a/tcg/region.c
> > +++ b/tcg/region.c
> > @@ -832,13 +832,17 @@ 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);
> 
> - Before:
> 
> Error displayed when qemu_mprotect_rwx/qemu_mprotect_rw/mprotect fail,
> then exit.
> 
> - After:
> 
> Error only displayed when mprotect() fails, then exit.
> Nothing displayed when qemu_mprotect_rwx() or qemu_mprotect_rw() failed,
> and exit.

Check the impl in qemu_mprotect__osdep - it calls error_report already,
so those code paths didn't need to be changedin this patch.


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 :|

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

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 10:08:13AM +0200, 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>
> ---
>  tcg/region.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH 03/12] 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.

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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

Re: [PATCH 03/12] hw/cxl: Convert cxl_fmws_link() to Error

[Jonathan Cameron via]

On Fri,  8 Aug 2025 10:08:14 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> 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.

Patch is definitely an improvement though I'm no sure how
it is really a violation of the above principle given
it has no effect on being called twice for example.

> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

The -1 return is perhaps unrelated to the main thing here,
but does make more sense than return 1 so fair enough.

None of the above comments I've raised are that important to me though.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>


> ---
>  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,

Re: [PATCH 03/12] hw/cxl: Convert cxl_fmws_link() to Error

[Markus Armbruster]

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> On Fri,  8 Aug 2025 10:08:14 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>
> Patch is definitely an improvement though I'm no sure how
> it is really a violation of the above principle given
> it has no effect on being called twice for example.

Note I wrote "Clean this up", not "fix this" :)

This is actually a canned commit message I've been using with suitable
adjustments for similar patches: commit b765d21e4ab, 35b1561e3ec,
e6696d3ee9b, 07d5b946539, ...

>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> The -1 return is perhaps unrelated to the main thing here,
> but does make more sense than return 1 so fair enough.

Accident, will back it out.

> None of the above comments I've raised are that important to me though.
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks!

>> ---
>>  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;

This line is the accident.

>>          }
>>          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,

Re: [PATCH 03/12] hw/cxl: Convert cxl_fmws_link() to Error

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
>
>> On Fri,  8 Aug 2025 10:08:14 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> 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.
>>
>> Patch is definitely an improvement though I'm no sure how
>> it is really a violation of the above principle given
>> it has no effect on being called twice for example.
>
> Note I wrote "Clean this up", not "fix this" :)
>
> This is actually a canned commit message I've been using with suitable
> adjustments for similar patches: commit b765d21e4ab, 35b1561e3ec,
> e6696d3ee9b, 07d5b946539, ...
>
>>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> The -1 return is perhaps unrelated to the main thing here,
>> but does make more sense than return 1 so fair enough.
>
> Accident, will back it out.

Changed my mind: I'm keeping it, with rationale in the commit message.

>> None of the above comments I've raised are that important to me though.
>>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> Thanks!

Re: [PATCH 03/12] hw/cxl: Convert cxl_fmws_link() to Error

[Philippe Mathieu-Daudé]

On 8/8/25 10:08, Markus Armbruster wrote:
> 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.
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/cxl/cxl-host.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

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

[PATCH 04/12] 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.

The error message starts with "internal error: ", so maybe this should
assert() instead.

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

diff --git a/migration/cpr.c b/migration/cpr.c
index 42ad0b0d50..908bcf83b2 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);
+        exit(1);
     }
 }
 
-- 
2.49.0

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

[Steven Sistare]

On 8/8/2025 4:08 AM, Markus Armbruster wrote:
> qapi/error.h advises:
> 
>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>   * exit(), because that's more obvious.
> 
> Do that.
> 
> The error message starts with "internal error: ", so maybe this should
> assert() instead.
> 
> 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..908bcf83b2 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);
> +        exit(1);
>       }
>   }
>   

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

[Philippe Mathieu-Daudé]

On 8/8/25 10:08, Markus Armbruster wrote:
> qapi/error.h advises:
> 
>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>   * exit(), because that's more obvious.
> 
> Do that.
> 
> The error message starts with "internal error: ", so maybe this should
> assert() instead.
> 
> Cc: Steve Sistare <steven.sistare@oracle.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   migration/cpr.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/cpr.c b/migration/cpr.c
> index 42ad0b0d50..908bcf83b2 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);
> +        exit(1);

My 2 cents, I'm not sure this information is more helpful than a plain
assertion (at least for users). No objection for this change.

>       }
>   }
>   

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

[Steven Sistare]

On 8/8/2025 9:55 AM, Philippe Mathieu-Daudé wrote:
> On 8/8/25 10:08, Markus Armbruster wrote:
>> qapi/error.h advises:
>>
>>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>>   * exit(), because that's more obvious.
>>
>> Do that.
>>
>> The error message starts with "internal error: ", so maybe this should
>> assert() instead.
>>
>> Cc: Steve Sistare <steven.sistare@oracle.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   migration/cpr.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/cpr.c b/migration/cpr.c
>> index 42ad0b0d50..908bcf83b2 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);
>> +        exit(1);
> 
> My 2 cents, I'm not sure this information is more helpful than a plain
> assertion (at least for users). No objection for this change.

The message gives more information.  It has helped me debug
problems in the past, in concert with enabling cpr traces.

- Steve

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

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 8/8/2025 9:55 AM, Philippe Mathieu-Daudé wrote:
>> On 8/8/25 10:08, Markus Armbruster wrote:
>>> qapi/error.h advises:
>>> 
>>>  * Please don't error_setg(&error_fatal, ...), use error_report() and
>>>  * exit(), because that's more obvious.
>>> 
>>> Do that.
>>> 
>>> The error message starts with "internal error: ", so maybe this should
>>> assert() instead.
>>> 
>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  migration/cpr.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/migration/cpr.c b/migration/cpr.c
>>> index 42ad0b0d50..908bcf83b2 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);
>>> +        exit(1);
>>
>> My 2 cents, I'm not sure this information is more helpful than a plain
>> assertion (at least for users). No objection for this change.
>
> The message gives more information.  It has helped me debug
> problems in the past, in concert with enabling cpr traces.

Is it a programming error?

If no, then "internal error: " is wrong.

If yes, then exit(1) is wrong.  I'd use assert() myself, but you're the
maintainer here, and if you want this message rather than the one
assert() gives you for free, we just replace exit(1) by abort() or
assert(0) or, if we're feeling particularly fancy
g_assert_not_reached().

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

[Steven Sistare]

On 8/8/2025 10:43 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
> 
>> On 8/8/2025 9:55 AM, Philippe Mathieu-Daudé wrote:
>>> On 8/8/25 10:08, Markus Armbruster wrote:
>>>> qapi/error.h advises:
>>>>
>>>>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>>>>   * exit(), because that's more obvious.
>>>>
>>>> Do that.
>>>>
>>>> The error message starts with "internal error: ", so maybe this should
>>>> assert() instead.
>>>>
>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>   migration/cpr.c | 9 +++++----
>>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/migration/cpr.c b/migration/cpr.c
>>>> index 42ad0b0d50..908bcf83b2 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);
>>>> +        exit(1);
>>>
>>> My 2 cents, I'm not sure this information is more helpful than a plain
>>> assertion (at least for users). No objection for this change.
>>
>> The message gives more information.  It has helped me debug
>> problems in the past, in concert with enabling cpr traces.
> 
> Is it a programming error?

Yes.

> If no, then "internal error: " is wrong.
> 
> If yes, then exit(1) is wrong.  I'd use assert() myself, but you're the
> maintainer here, and if you want this message rather than the one
> assert() gives you for free, we just replace exit(1) by abort() or
> assert(0) or, if we're feeling particularly fancy
> g_assert_not_reached().

I would like the full message.
I have no preference on how to exit.

- Steve

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

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 8/8/2025 10:43 AM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:
>> 
>>> On 8/8/2025 9:55 AM, Philippe Mathieu-Daudé wrote:
>>>> On 8/8/25 10:08, Markus Armbruster wrote:
>>>>> qapi/error.h advises:
>>>>>
>>>>>   * Please don't error_setg(&error_fatal, ...), use error_report() and
>>>>>   * exit(), because that's more obvious.
>>>>>
>>>>> Do that.
>>>>>
>>>>> The error message starts with "internal error: ", so maybe this should
>>>>> assert() instead.
>>>>>
>>>>> Cc: Steve Sistare <steven.sistare@oracle.com>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

>>>> My 2 cents, I'm not sure this information is more helpful than a plain
>>>> assertion (at least for users). No objection for this change.
>>>
>>> The message gives more information.  It has helped me debug
>>> problems in the past, in concert with enabling cpr traces.
>>
>> Is it a programming error?
>
> Yes.
>
>> If no, then "internal error: " is wrong.
>>
>> If yes, then exit(1) is wrong.  I'd use assert() myself, but you're the
>> maintainer here, and if you want this message rather than the one
>> assert() gives you for free, we just replace exit(1) by abort() or
>> assert(0) or, if we're feeling particularly fancy
>> g_assert_not_reached().
>
> I would like the full message.
> I have no preference on how to exit.

Will adjust v2 accordingly.

Thanks!

[PATCH 05/12] 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 06/12] 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, ...).

I'm not familiar with liblirp, so I can't say whether the network
backend will work after such a failure.  If it doesn't, then this
should be an error.  If it does, then why bother the user with a
warning that isn't actionable, and likely confusing?

Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
...)  are.  Replace by warn_report().

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

diff --git a/net/slirp.c b/net/slirp.c
index 9657e86a84..d75b09f16b 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -262,7 +262,8 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
     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()");
+        warn_report("failed to WSAEventSelect(): %s",
+                    g_win32_error_message(WSAGetLastError()));
     }
 #endif
 }
@@ -271,7 +272,8 @@ static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
 {
 #ifdef WIN32
     if (WSAEventSelect(fd, NULL, 0) != 0) {
-        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
+        warn_report("failed to WSAEventSelect()",
+                    g_win32_error_message(WSAGetLastError()));
     }
 #endif
 }
-- 
2.49.0

Re: [PATCH 06/12] net/slirp: Clean up error reporting

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:

> net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
> report WSAEventSelect() failure with error_setg(&error_warn, ...).
>
> I'm not familiar with liblirp, so I can't say whether the network
> backend will work after such a failure.  If it doesn't, then this
> should be an error.  If it does, then why bother the user with a
> warning that isn't actionable, and likely confusing?
>

I don't know if this is recoverable either. It's probably not if the handle
is invalid. I guess you could turn it into a non-fatal error.


>
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...)  are.  Replace by warn_report().
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>

lgtm, thanks

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> ---
>  net/slirp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index 9657e86a84..d75b09f16b 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -262,7 +262,8 @@ static void
> net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
>      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()");
> +        warn_report("failed to WSAEventSelect(): %s",
> +                    g_win32_error_message(WSAGetLastError()));
>      }
>  #endif
>  }
> @@ -271,7 +272,8 @@ static void
> net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
>  {
>  #ifdef WIN32
>      if (WSAEventSelect(fd, NULL, 0) != 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to
> WSAEventSelect()");
> +        warn_report("failed to WSAEventSelect()",
> +                    g_win32_error_message(WSAGetLastError()));
>      }
>  #endif
>  }
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 3359 bytes --]

Re: [PATCH 06/12] net/slirp: Clean up error reporting

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 10:08:17AM +0200, Markus Armbruster wrote:
> net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
> report WSAEventSelect() failure with error_setg(&error_warn, ...).
> 
> I'm not familiar with liblirp, so I can't say whether the network

                       ^^^^^^^^^ 'libslirp'


> backend will work after such a failure.  If it doesn't, then this
> should be an error.  If it does, then why bother the user with a
> warning that isn't actionable, and likely confusing?
> 
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...)  are.  Replace by warn_report().
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> --->
  net/slirp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> diff --git a/net/slirp.c b/net/slirp.c
> index 9657e86a84..d75b09f16b 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -262,7 +262,8 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
>      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()");
> +        warn_report("failed to WSAEventSelect(): %s",
> +                    g_win32_error_message(WSAGetLastError()));
>      }
>  #endif

IMHO this one ought to be considered fatal. If we can't select the
right events on the socket, then we're not going to have a good time
trying to poll on events. The libslirp callback API doesn't allow
us to return a success/failure code from this function, and IMHO it
is not appropriate to use error_fatal here because a fault with slirp
should not take down the whole of QEMU. So warn_report is the least
worst option I guess. At least it is a hint to the user that all is
not well - even if they can't action it, it might alert them if they
see network problems in their guest.


>  }
> @@ -271,7 +272,8 @@ static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
>  {
>  #ifdef WIN32
>      if (WSAEventSelect(fd, NULL, 0) != 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
> +        warn_report("failed to WSAEventSelect()",
> +                    g_win32_error_message(WSAGetLastError()));
>      }

This one is reasonable to treat as non-fatal, since once we've
unregistered the socket for polling

>  #endif
>  }
> -- 
> 2.49.0
> 
> 

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 :|

Re: [PATCH 06/12] net/slirp: Clean up error reporting

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 10:08:17AM +0200, Markus Armbruster wrote:
>> net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
>> report WSAEventSelect() failure with error_setg(&error_warn, ...).
>> 
>> I'm not familiar with liblirp, so I can't say whether the network
>
>                        ^^^^^^^^^ 'libslirp'

Will fix, thanks!

>> backend will work after such a failure.  If it doesn't, then this
>> should be an error.  If it does, then why bother the user with a
>> warning that isn't actionable, and likely confusing?
>> 
>> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
>> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
>> ...)  are.  Replace by warn_report().
>> 
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/slirp.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>> diff --git a/net/slirp.c b/net/slirp.c
>> index 9657e86a84..d75b09f16b 100644
>> --- a/net/slirp.c
>> +++ b/net/slirp.c
>> @@ -262,7 +262,8 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
>>      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()");
>> +        warn_report("failed to WSAEventSelect(): %s",
>> +                    g_win32_error_message(WSAGetLastError()));
>>      }
>>  #endif
>
> IMHO this one ought to be considered fatal. If we can't select the
> right events on the socket, then we're not going to have a good time
> trying to poll on events. The libslirp callback API doesn't allow
> us to return a success/failure code from this function, and IMHO it
> is not appropriate to use error_fatal here because a fault with slirp
> should not take down the whole of QEMU. So warn_report is the least
> worst option I guess. At least it is a hint to the user that all is
> not well - even if they can't action it, it might alert them if they
> see network problems in their guest.

So, we'd make this an error if we could.  But we can't: the function is
a callback that cannot fail, and outright exit(1) is too harsh.

That leaves silence or warning.  Warning is less bad.

Correct?

>
>>  }
>> @@ -271,7 +272,8 @@ static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
>>  {
>>  #ifdef WIN32
>>      if (WSAEventSelect(fd, NULL, 0) != 0) {
>> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
>> +        warn_report("failed to WSAEventSelect()",
>> +                    g_win32_error_message(WSAGetLastError()));
>>      }
>
> This one is reasonable to treat as non-fatal, since once we've
> unregistered the socket for polling

Whether clearing the event associated with the socket can fail is
unclear.  Whether it should be treated as an error is also unclear.

If yes, then same argument as for net_slirp_register_poll_sock() above.

If no, silence or warning.  Warning is less bad.

Correct?

>>  #endif
>>  }

[...]

Re: [PATCH 06/12] net/slirp: Clean up error reporting

[Daniel P. Berrangé]

On Tue, Sep 09, 2025 at 01:40:59PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:17AM +0200, Markus Armbruster wrote:
> >> net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
> >> report WSAEventSelect() failure with error_setg(&error_warn, ...).
> >> 
> >> I'm not familiar with liblirp, so I can't say whether the network
> >
> >                        ^^^^^^^^^ 'libslirp'
> 
> Will fix, thanks!
> 
> >> backend will work after such a failure.  If it doesn't, then this
> >> should be an error.  If it does, then why bother the user with a
> >> warning that isn't actionable, and likely confusing?
> >> 
> >> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> >> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> >> ...)  are.  Replace by warn_report().
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  net/slirp.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> >> diff --git a/net/slirp.c b/net/slirp.c
> >> index 9657e86a84..d75b09f16b 100644
> >> --- a/net/slirp.c
> >> +++ b/net/slirp.c
> >> @@ -262,7 +262,8 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
> >>      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()");
> >> +        warn_report("failed to WSAEventSelect(): %s",
> >> +                    g_win32_error_message(WSAGetLastError()));
> >>      }
> >>  #endif
> >
> > IMHO this one ought to be considered fatal. If we can't select the
> > right events on the socket, then we're not going to have a good time
> > trying to poll on events. The libslirp callback API doesn't allow
> > us to return a success/failure code from this function, and IMHO it
> > is not appropriate to use error_fatal here because a fault with slirp
> > should not take down the whole of QEMU. So warn_report is the least
> > worst option I guess. At least it is a hint to the user that all is
> > not well - even if they can't action it, it might alert them if they
> > see network problems in their guest.
> 
> So, we'd make this an error if we could.  But we can't: the function is
> a callback that cannot fail, and outright exit(1) is too harsh.
> 
> That leaves silence or warning.  Warning is less bad.
> 
> Correct?

Yes.

> >> @@ -271,7 +272,8 @@ static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
> >>  {
> >>  #ifdef WIN32
> >>      if (WSAEventSelect(fd, NULL, 0) != 0) {
> >> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
> >> +        warn_report("failed to WSAEventSelect()",
> >> +                    g_win32_error_message(WSAGetLastError()));
> >>      }
> >
> > This one is reasonable to treat as non-fatal, since once we've
> > unregistered the socket for polling
> 
> Whether clearing the event associated with the socket can fail is
> unclear.  Whether it should be treated as an error is also unclear.
> 
> If yes, then same argument as for net_slirp_register_poll_sock() above.

If the callback allowed returning an error, we probably would return
an error, but then I'm confident slirp would do nothing more than
print a warning and continue to close the file handle as normal. It
doesn't make sense to turn file descriptor cleanup into fatal error.

> 
> If no, silence or warning.  Warning is less bad.
> 
> Correct?

Yes.

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 :|

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

[Markus Armbruster]

watch_add() reports _open_osfhandle() failure with
error_setg(&error_warn, ...).

I'm not familiar with Spice, so I can't say whether it will work after
such a failure.  If it doesn't, then this should be an error.  If it
does, then why bother the user with a warning that isn't actionable,
and likely confusing?

Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
...) are.  Replace by warn_report().

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/spice-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:

> watch_add() reports _open_osfhandle() failure with
> error_setg(&error_warn, ...).
>
> I'm not familiar with Spice, so I can't say whether it will work after
> such a failure.  If it doesn't, then this should be an error.  If it
> does, then why bother the user with a warning that isn't actionable,
> and likely confusing?
>
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...) are.  Replace by warn_report().
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>


lgtm, thanks
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  ui/spice-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 5992f9daec..97bdd171cd 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -132,7 +132,8 @@ static SpiceWatch *watch_add(int fd, int event_mask,
> SpiceWatchFunc func, void *
>  #ifdef WIN32
>      fd = _open_osfhandle(fd, _O_BINARY);
>      if (fd < 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't
> associate a FD with the SOCKET");
> +        warn_report("Couldn't associate a FD with the SOCKET: %s"
> +                    g_win32_error_message(WSAGetLastError()));
>          return NULL;
>      }
>  #endif
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 2394 bytes --]

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
> watch_add() reports _open_osfhandle() failure with
> error_setg(&error_warn, ...).
> 
> I'm not familiar with Spice, so I can't say whether it will work after
> such a failure.  If it doesn't, then this should be an error.  If it
> does, then why bother the user with a warning that isn't actionable,
> and likely confusing?

If watch_add fails, spice is dead. Eventually the NULL returned from
watch_add will bubble up to the spice_server_init function which will
return non-zero and QEMU will do

        error_report("failed to initialize spice server");
        exit(1);

The warning in watch_add gives a better chance of understanding
what failed than this generic error_report() does.

> 
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...) are.  Replace by warn_report().
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/spice-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 5992f9daec..97bdd171cd 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -132,7 +132,8 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
>  #ifdef WIN32
>      fd = _open_osfhandle(fd, _O_BINARY);
>      if (fd < 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET");
> +        warn_report("Couldn't associate a FD with the SOCKET: %s"
> +                    g_win32_error_message(WSAGetLastError()));
>          return NULL;
>      }
>  #endif


> -- 
> 2.49.0
> 
> 

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 :|

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
>> watch_add() reports _open_osfhandle() failure with
>> error_setg(&error_warn, ...).
>> 
>> I'm not familiar with Spice, so I can't say whether it will work after
>> such a failure.  If it doesn't, then this should be an error.  If it
>> does, then why bother the user with a warning that isn't actionable,
>> and likely confusing?
>
> If watch_add fails, spice is dead. Eventually the NULL returned from
> watch_add will bubble up to the spice_server_init function which will
> return non-zero and QEMU will do
>
>         error_report("failed to initialize spice server");
>         exit(1);
>
> The warning in watch_add gives a better chance of understanding
> what failed than this generic error_report() does.

Would you like me to work this into the commit message?

>> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
>> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
>> ...) are.  Replace by warn_report().
>> 
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  ui/spice-core.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!

[...]

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

[Daniel P. Berrangé]

On Tue, Sep 09, 2025 at 01:41:51PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
> >> watch_add() reports _open_osfhandle() failure with
> >> error_setg(&error_warn, ...).
> >> 
> >> I'm not familiar with Spice, so I can't say whether it will work after
> >> such a failure.  If it doesn't, then this should be an error.  If it
> >> does, then why bother the user with a warning that isn't actionable,
> >> and likely confusing?
> >
> > If watch_add fails, spice is dead. Eventually the NULL returned from
> > watch_add will bubble up to the spice_server_init function which will
> > return non-zero and QEMU will do
> >
> >         error_report("failed to initialize spice server");
> >         exit(1);
> >
> > The warning in watch_add gives a better chance of understanding
> > what failed than this generic error_report() does.
> 
> Would you like me to work this into the commit message?

Yes, if you're re-spinning might as well add the context info.

> 
> >> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> >> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> >> ...) are.  Replace by warn_report().
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  ui/spice-core.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Thanks!
> 
> [...]
> 

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 :|

[PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[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 specifies that errors are to be ignored.  If warnings are
wanted, the caller must pass &error_warn.

I'm not familiar with the calling code, so I can't say whether it will
work after WSAEventSelect() failure.  If it doesn't, then this should
be an error.  If it does, then why bother the user with a warning that
isn't actionable, and likely confusing?

The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
qemu_socket_select() helper).  Before that commit, the error was
ignored, as indicated by passing a null @errp.  Revert to that
behavior.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/oslib-win32.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b7351634ec..136a8fe118 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -296,10 +296,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

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> 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 specifies that errors are to be ignored.  If warnings are
> wanted, the caller must pass &error_warn.
>
> I'm not familiar with the calling code, so I can't say whether it will
> work after WSAEventSelect() failure.  If it doesn't, then this should
> be an error.  If it does, then why bother the user with a warning that
> isn't actionable, and likely confusing?
>
> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> qemu_socket_select() helper).  Before that commit, the error was
> ignored, as indicated by passing a null @errp.  Revert to that
> behavior.
>

Yes, the potential errors before introducing the wrapper were simply
ignored. I think we should fix the users or maybe just report the warning
and drop errp from the wrapper function. wdyt?


>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

---
>  util/oslib-win32.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ec..136a8fe118 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -296,10 +296,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
>
>

[-- Attachment #2: Type: text/html, Size: 2634 bytes --]

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> 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 specifies that errors are to be ignored.  If warnings are
>> wanted, the caller must pass &error_warn.
>>
>> I'm not familiar with the calling code, so I can't say whether it will
>> work after WSAEventSelect() failure.  If it doesn't, then this should
>> be an error.  If it does, then why bother the user with a warning that
>> isn't actionable, and likely confusing?
>>
>> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
>> qemu_socket_select() helper).  Before that commit, the error was
>> ignored, as indicated by passing a null @errp.  Revert to that
>> behavior.
>>
>
> Yes, the potential errors before introducing the wrapper were simply
> ignored. I think we should fix the users or maybe just report the warning
> and drop errp from the wrapper function. wdyt?

Phil's "[RFC PATCH 0/2] system/win32: Remove unused Error argument in
qemu_socket_[un]select()" does the latter.

I doubt warnings are the right tool here.  I just posted

    Subject: Abuse of warnings for unhandled errors and programming errors
    Message-ID: <87h5yijh3b.fsf@pond.sub.org>

[...]

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Daniel P. Berrangé]

On Fri, Aug 08, 2025 at 10:08:19AM +0200, 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 specifies that errors are to be ignored.  If warnings are
> wanted, the caller must pass &error_warn.
> 
> I'm not familiar with the calling code, so I can't say whether it will
> work after WSAEventSelect() failure.  If it doesn't, then this should
> be an error.  If it does, then why bother the user with a warning that
> isn't actionable, and likely confusing?
> 
> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> qemu_socket_select() helper).  Before that commit, the error was
> ignored, as indicated by passing a null @errp.  Revert to that
> behavior.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/oslib-win32.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ec..136a8fe118 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>  {
>      SOCKET s = _get_osfhandle(sockfd);
>  
> -    if (errp == NULL) {
> -        errp = &error_warn;
> -    }

This makes sense, but I'd want the callers to be using warn_report
instead. Ideally some (but not all) of the callers would propagate
the error, but this isn't practical with the QIOChannel create
watch function usage. I'd want to keep Error *errp on this function
though, and have warn_report as a sign to our future selves that
this is still not ideal.

> -
>      if (s == INVALID_SOCKET) {
>          error_setg(errp, "invalid socket fd=%d", sockfd);
>          return false;
> -- 
> 2.49.0
> 
> 

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 :|

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 10:08:19AM +0200, 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 specifies that errors are to be ignored.  If warnings are
>> wanted, the caller must pass &error_warn.
>> 
>> I'm not familiar with the calling code, so I can't say whether it will
>> work after WSAEventSelect() failure.  If it doesn't, then this should
>> be an error.  If it does, then why bother the user with a warning that
>> isn't actionable, and likely confusing?
>> 
>> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
>> qemu_socket_select() helper).  Before that commit, the error was
>> ignored, as indicated by passing a null @errp.  Revert to that
>> behavior.
>> 
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/oslib-win32.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index b7351634ec..136a8fe118 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>>  {
>>      SOCKET s = _get_osfhandle(sockfd);
>>  
>> -    if (errp == NULL) {
>> -        errp = &error_warn;
>> -    }
>
> This makes sense, but I'd want the callers to be using warn_report
> instead. Ideally some (but not all) of the callers would propagate
> the error, but this isn't practical with the QIOChannel create
> watch function usage. I'd want to keep Error *errp on this function
> though, and have warn_report as a sign to our future selves that
> this is still not ideal.

The direct callers are qio_channel_create_socket_watch(),
aio_set_fd_handler().  Callers via qemu_socket_unselect() are
qio_channel_socket_finalize(), qio_channel_socket_close(),
qemu_socket_set_block().

All but qio_channel_socket_close() cannot fail.  Would you like me to
make them pass &error_warn, because warning is less bad than silence
there?

qio_channel_socket_close() can fail, but it ignores
qemu_socket_unselect() failure.  What do you want me to do there?

>> -
>>      if (s == INVALID_SOCKET) {
>>          error_setg(errp, "invalid socket fd=%d", sockfd);
>>          return false;
>> -- 
>> 2.49.0
>> 
>> 
>
> With regards,
> Daniel

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Daniel P. Berrangé]

On Tue, Sep 09, 2025 at 01:50:56PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:19AM +0200, 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 specifies that errors are to be ignored.  If warnings are
> >> wanted, the caller must pass &error_warn.
> >> 
> >> I'm not familiar with the calling code, so I can't say whether it will
> >> work after WSAEventSelect() failure.  If it doesn't, then this should
> >> be an error.  If it does, then why bother the user with a warning that
> >> isn't actionable, and likely confusing?
> >> 
> >> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> >> qemu_socket_select() helper).  Before that commit, the error was
> >> ignored, as indicated by passing a null @errp.  Revert to that
> >> behavior.
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  util/oslib-win32.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >> 
> >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> >> index b7351634ec..136a8fe118 100644
> >> --- a/util/oslib-win32.c
> >> +++ b/util/oslib-win32.c
> >> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> >>  {
> >>      SOCKET s = _get_osfhandle(sockfd);
> >>  
> >> -    if (errp == NULL) {
> >> -        errp = &error_warn;
> >> -    }
> >
> > This makes sense, but I'd want the callers to be using warn_report
> > instead. Ideally some (but not all) of the callers would propagate
> > the error, but this isn't practical with the QIOChannel create
> > watch function usage. I'd want to keep Error *errp on this function
> > though, and have warn_report as a sign to our future selves that
> > this is still not ideal.
> 
> The direct callers are qio_channel_create_socket_watch(),
> aio_set_fd_handler().  Callers via qemu_socket_unselect() are
> qio_channel_socket_finalize(), qio_channel_socket_close(),
> qemu_socket_set_block().
> 
> All but qio_channel_socket_close() cannot fail.  Would you like me to
> make them pass &error_warn, because warning is less bad than silence
> there?
> 
> qio_channel_socket_close() can fail, but it ignores
> qemu_socket_unselect() failure.  What do you want me to do there?

I think the overriding important thing is that we /must/ try to
close(), and if close() succeeds claim the whole qio_channel_socket_close
was successful. So I guess I'd say that &error_warn should be passed
from all callers.

> 
> >> -
> >>      if (s == INVALID_SOCKET) {
> >>          error_setg(errp, "invalid socket fd=%d", sockfd);
> >>          return false;
> >> -- 
> >> 2.49.0
> >> 
> >> 
> >
> > With regards,
> > Daniel
> 

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 :|

[PATCH 09/12] 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>
---
 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

Re: [PATCH 09/12] ui/pixman: Consistent error handling in qemu_pixman_shareable_free()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:

> 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>
>

lgtm, thanks

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
>
>

[-- Attachment #2: Type: text/html, Size: 2054 bytes --]

[PATCH 10/12] 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>
---
 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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> 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>
>

lgtm, thanks

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
>
>

[-- Attachment #2: Type: text/html, Size: 2902 bytes --]

[PATCH 11/12] 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>
---
 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

Re: [PATCH 11/12] ui/dbus: Consistent handling of texture mutex failure

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:

> 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>
>

lgtm, thanks

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
>
>

[-- Attachment #2: Type: text/html, Size: 3132 bytes --]

[PATCH 12/12] 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 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.0

Re: [PATCH 12/12] error: Kill @error_warn

[Philippe Mathieu-Daudé]

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(-)

Re: [PATCH 12/12] error: Kill @error_warn

[Markus Armbruster]

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(-)

Re: [PATCH 12/12] error: Kill @error_warn

[Akihiko Odaki]

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);

Re: [PATCH 12/12] error: Kill @error_warn

[Markus Armbruster]

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.

Re: [PATCH 12/12] error: Kill @error_warn

[Akihiko Odaki]

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.

Re: [PATCH 12/12] error: Kill @error_warn

[Markus Armbruster]

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!

Re: [PATCH 12/12] error: Kill @error_warn

[Daniel P. Berrangé]

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

> +    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 :|

Re: [PATCH 12/12] error: Kill @error_warn

[Markus Armbruster]

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