[PATCH 0/5] A bit of cleanup around Error

[PATCH 0/5] A bit of cleanup around Error

[Markus Armbruster]

Note: the last patch adds a drive-by FIXME.  I asked for advice on how
to fix it:

    Subject: Incorrect error handling in xen_enable_tpm()
    To: qemu-devel@nongnu.org
    Cc: Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, Paul
     Durrant <paul@xen.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
     xen-devel@lists.xenproject.org, Vikram Garhwal <vikram.garhwal@amd.com> 
    Date: Fri, 14 Nov 2025 10:16:58 +0100
    Message-ID: <87jyzt0y9h.fsf@pond.sub.org>

No reply so far.

Markus Armbruster (5):
  hw/core/loader: Make load_elf_hdr() return bool, simplify caller
  hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
  nbd/client-connection: Replace error_propagate() by assignment
  error: error_free(NULL) is safe, drop unnecessary conditionals
  error: Consistently name Error * objects err, and not errp

 include/hw/loader.h         |  4 +++-
 block/crypto.c              |  8 ++++----
 hw/acpi/ghes.c              |  8 ++++----
 hw/acpi/pcihp.c             |  4 +---
 hw/arm/boot.c               |  6 +-----
 hw/core/loader.c            |  8 ++++++--
 hw/nvram/xlnx-bbram.c       | 18 ++++--------------
 hw/ppc/spapr.c              | 16 ++++++++--------
 hw/riscv/spike.c            | 10 +---------
 hw/xen/xen-pvh-common.c     | 13 ++++++++++---
 io/channel-websock.c        |  4 +---
 io/task.c                   |  4 +---
 migration/migration.c       |  6 ++----
 nbd/client-connection.c     |  3 +--
 nbd/common.c                |  6 +++---
 tests/unit/test-smp-parse.c |  5 +----
 16 files changed, 51 insertions(+), 72 deletions(-)

-- 
2.49.0

[PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/loader.h |  4 +++-
 hw/arm/boot.c       |  6 +-----
 hw/core/loader.c    |  8 ++++++--
 hw/riscv/spike.c    | 10 +---------
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index d035e72748..6f91703503 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
  *
  * Inspect an ELF file's header. Read its full header contents into a
  * buffer and/or determine if the ELF is 64bit.
+ *
+ * Returns true on success, false on failure.
  */
-void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
+bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
 
 ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
                   bool big_endian, hwaddr target_page_size);
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b91660208f..06b303aab8 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -766,16 +766,12 @@ static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
     int data_swab = 0;
     int elf_data_order;
     ssize_t ret;
-    Error *err = NULL;
 
-
-    load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
-    if (err) {
+    if (!load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, NULL)) {
         /*
          * If the file is not an ELF file we silently return.
          * The caller will fall back to try other formats.
          */
-        error_free(err);
         return -1;
     }
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 590c5b02aa..10687fe1c8 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -364,8 +364,9 @@ const char *load_elf_strerror(ssize_t error)
     }
 }
 
-void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
+bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
 {
+    bool ok = false;
     int fd;
     uint8_t e_ident_local[EI_NIDENT];
     uint8_t *e_ident;
@@ -380,7 +381,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {
         error_setg_errno(errp, errno, "Failed to open file: %s", filename);
-        return;
+        return false;
     }
     if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
         error_setg_errno(errp, errno, "Failed to read file: %s", filename);
@@ -415,8 +416,11 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
         off += br;
     }
 
+    ok = true;
+
 fail:
     close(fd);
+    return ok;
 }
 
 /* return < 0 if error, otherwise the number of bytes loaded in memory */
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index b0bab3fe00..8531e1d121 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -180,15 +180,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
 
 static bool spike_test_elf_image(char *filename)
 {
-    Error *err = NULL;
-
-    load_elf_hdr(filename, NULL, NULL, &err);
-    if (err) {
-        error_free(err);
-        return false;
-    } else {
-        return true;
-    }
+    return load_elf_hdr(filename, NULL, NULL, NULL);
 }
 
 static void spike_board_init(MachineState *machine)
-- 
2.49.0

[PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Markus Armbruster]

bbram_bdrv_error() interpolates a "detail" string into a template with
error_setg_errno(), then reports the result with error_report().
Produces error messages with an unwanted '.':

    BLK-NAME: BBRAM backstore DETAIL failed.: STERROR

Replace both calls of bbram_bdrv_error() by straightforward
error_report(), and drop the function.  This is less code, easier to
read, and the error message is more greppable.

Also delete the unwanted '.'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/nvram/xlnx-bbram.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/hw/nvram/xlnx-bbram.c b/hw/nvram/xlnx-bbram.c
index 22aefbc240..fe289bad9d 100644
--- a/hw/nvram/xlnx-bbram.c
+++ b/hw/nvram/xlnx-bbram.c
@@ -88,18 +88,6 @@ static bool bbram_pgm_enabled(XlnxBBRam *s)
     return ARRAY_FIELD_EX32(s->regs, BBRAM_STATUS, PGM_MODE) != 0;
 }
 
-static void bbram_bdrv_error(XlnxBBRam *s, int rc, gchar *detail)
-{
-    Error *errp = NULL;
-
-    error_setg_errno(&errp, -rc, "%s: BBRAM backstore %s failed.",
-                     blk_name(s->blk), detail);
-    error_report("%s", error_get_pretty(errp));
-    error_free(errp);
-
-    g_free(detail);
-}
-
 static void bbram_bdrv_read(XlnxBBRam *s, Error **errp)
 {
     uint32_t *ram = &s->regs[R_BBRAM_0];
@@ -162,7 +150,8 @@ static void bbram_bdrv_sync(XlnxBBRam *s, uint64_t hwaddr)
     offset = hwaddr - A_BBRAM_0;
     rc = blk_pwrite(s->blk, offset, 4, &le32, 0);
     if (rc < 0) {
-        bbram_bdrv_error(s, rc, g_strdup_printf("write to offset %u", offset));
+        error_report("%s: BBRAM backstore write to offset %u failed: %s",
+                     blk_name(s->blk), offset, strerror(-rc));
     }
 }
 
@@ -178,7 +167,8 @@ static void bbram_bdrv_zero(XlnxBBRam *s)
 
     rc = blk_make_zero(s->blk, 0);
     if (rc < 0) {
-        bbram_bdrv_error(s, rc, g_strdup("zeroizing"));
+        error_report("%s: BBRAM backstore zeroizing failed: %s",
+                     blk_name(s->blk), strerror(-rc));
     }
 
     /* Restore bbram8 if it is non-zero */
-- 
2.49.0

[PATCH 3/5] nbd/client-connection: Replace error_propagate() by assignment

[Markus Armbruster]

connect_thread_func() sets a variable to null, then error_propagate()s
an Error * to it.  This is a roundabout way to assign the Error * to
it, so replace it by just that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 nbd/client-connection.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 79ea97e4cc..6a4f080717 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -207,8 +207,7 @@ static void *connect_thread_func(void *opaque)
         qemu_mutex_lock(&conn->mutex);
 
         error_free(conn->err);
-        conn->err = NULL;
-        error_propagate(&conn->err, local_err);
+        conn->err = local_err;
 
         if (ret < 0) {
             object_unref(OBJECT(conn->sioc));
-- 
2.49.0

[PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary conditionals

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/acpi/pcihp.c             | 4 +---
 io/channel-websock.c        | 4 +---
 io/task.c                   | 4 +---
 migration/migration.c       | 6 ++----
 tests/unit/test-smp-parse.c | 5 +----
 5 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 4922bbc778..87162ff2c0 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -62,9 +62,7 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
                                              &local_err);
 
     if (local_err || bsel >= ACPI_PCIHP_MAX_HOTPLUG_BUS) {
-        if (local_err) {
-            error_free(local_err);
-        }
+        error_free(local_err);
         return -1;
     } else {
         return bsel;
diff --git a/io/channel-websock.c b/io/channel-websock.c
index cb4dafdebb..d0929ba232 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -932,9 +932,7 @@ static void qio_channel_websock_finalize(Object *obj)
     if (ioc->io_tag) {
         g_source_remove(ioc->io_tag);
     }
-    if (ioc->io_err) {
-        error_free(ioc->io_err);
-    }
+    error_free(ioc->io_err);
     object_unref(OBJECT(ioc->master));
 }
 
diff --git a/io/task.c b/io/task.c
index 451f26f8b4..da79d31782 100644
--- a/io/task.c
+++ b/io/task.c
@@ -91,9 +91,7 @@ static void qio_task_free(QIOTask *task)
     if (task->destroyResult) {
         task->destroyResult(task->result);
     }
-    if (task->err) {
-        error_free(task->err);
-    }
+    error_free(task->err);
     object_unref(task->source);
 
     qemu_mutex_unlock(&task->thread_lock);
diff --git a/migration/migration.c b/migration/migration.c
index c2daab6bdd..3160e24220 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1569,10 +1569,8 @@ bool migrate_has_error(MigrationState *s)
 static void migrate_error_free(MigrationState *s)
 {
     QEMU_LOCK_GUARD(&s->error_mutex);
-    if (s->error) {
-        error_free(s->error);
-        s->error = NULL;
-    }
+    error_free(s->error);
+    s->error = NULL;
 }
 
 static void migration_connect_set_error(MigrationState *s, const Error *error)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 326045ecbb..34082c1465 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -875,10 +875,7 @@ static void check_parse(MachineState *ms, const SMPConfiguration *config,
                config_str, expect_err, output_topo_str);
 
 end:
-    if (err != NULL) {
-        error_free(err);
-    }
-
+    error_free(err);
     abort();
 }
 
-- 
2.49.0

[PATCH 5/5] error: Consistently name Error * objects err, and not errp

[Markus Armbruster]

This touches code in xen_enable_tpm() that is obviously wrong.  Since
I don't know how to fix it properly, I'm adding a FIXME there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/crypto.c          |  8 ++++----
 hw/acpi/ghes.c          |  8 ++++----
 hw/ppc/spapr.c          | 16 ++++++++--------
 hw/xen/xen-pvh-common.c | 13 ++++++++++---
 nbd/common.c            |  6 +++---
 5 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index b97d027444..36abb7af46 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -938,14 +938,14 @@ static void GRAPH_RDLOCK
 block_crypto_amend_cleanup(BlockDriverState *bs)
 {
     BlockCrypto *crypto = bs->opaque;
-    Error *errp = NULL;
+    Error *err = NULL;
 
     /* release exclusive read/write permissions to the underlying file */
     crypto->updating_keys = false;
-    bdrv_child_refresh_perms(bs, bs->file, &errp);
+    bdrv_child_refresh_perms(bs, bs->file, &err);
 
-    if (errp) {
-        error_report_err(errp);
+    if (err) {
+        error_report_err(err);
     }
 }
 
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 06555905ce..841a36e370 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -563,7 +563,7 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,
     const uint8_t guid[] =
           UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
                   0xED, 0x7C, 0x83, 0xB1);
-    Error *errp = NULL;
+    Error *err = NULL;
     int data_length;
     GArray *block;
 
@@ -583,12 +583,12 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,
     acpi_ghes_build_append_mem_cper(block, physical_address);
 
     /* Report the error */
-    ghes_record_cper_errors(ags, block->data, block->len, source_id, &errp);
+    ghes_record_cper_errors(ags, block->data, block->len, source_id, &err);
 
     g_array_free(block, true);
 
-    if (errp) {
-        error_report_err(errp);
+    if (err) {
+        error_report_err(err);
         return -1;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 99b843ba2f..db5e98e458 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2847,7 +2847,7 @@ static void spapr_machine_init(MachineState *machine)
     int i;
     MemoryRegion *sysmem = get_system_memory();
     long load_limit, fw_size;
-    Error *errp = NULL;
+    Error *err = NULL;
     NICInfo *nd;
 
     if (!filename) {
@@ -2871,7 +2871,7 @@ static void spapr_machine_init(MachineState *machine)
     /* Determine capabilities to run with */
     spapr_caps_init(spapr);
 
-    kvmppc_check_papr_resize_hpt(&errp);
+    kvmppc_check_papr_resize_hpt(&err);
     if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
         /*
          * If the user explicitly requested a mode we should either
@@ -2879,10 +2879,10 @@ static void spapr_machine_init(MachineState *machine)
          * it's not set explicitly, we reset our mode to something
          * that works
          */
-        if (errp) {
+        if (err) {
             spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
-            error_free(errp);
-            errp = NULL;
+            error_free(err);
+            err = NULL;
         } else {
             spapr->resize_hpt = smc->resize_hpt_default;
         }
@@ -2890,14 +2890,14 @@ static void spapr_machine_init(MachineState *machine)
 
     assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
 
-    if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && errp) {
+    if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && err) {
         /*
          * User requested HPT resize, but this host can't supply it.  Bail out
          */
-        error_report_err(errp);
+        error_report_err(err);
         exit(1);
     }
-    error_free(errp);
+    error_free(err);
 
     spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
 
diff --git a/hw/xen/xen-pvh-common.c b/hw/xen/xen-pvh-common.c
index b93ff80c85..3e62ec09d0 100644
--- a/hw/xen/xen-pvh-common.c
+++ b/hw/xen/xen-pvh-common.c
@@ -101,7 +101,7 @@ static void xen_create_virtio_mmio_devices(XenPVHMachineState *s)
 #ifdef CONFIG_TPM
 static void xen_enable_tpm(XenPVHMachineState *s)
 {
-    Error *errp = NULL;
+    Error *err = NULL;
     DeviceState *dev;
     SysBusDevice *busdev;
 
@@ -111,8 +111,15 @@ static void xen_enable_tpm(XenPVHMachineState *s)
         return;
     }
     dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
-    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
-    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
+    /*
+     * FIXME This use of &err is is wrong.  If both calls fail, the
+     * second will trip error_setv()'s assertion.  If just one call
+     * fails, we leak an Error object.  Setting the same property
+     * twice (first to a QOM path, then to an ID string) is almost
+     * certainly wrong, too.
+     */
+    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &err);
+    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &err);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, s->cfg.tpm.base);
diff --git a/nbd/common.c b/nbd/common.c
index 2a133a66c3..f43cbaa15b 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -282,10 +282,10 @@ void nbd_set_socket_send_buffer(QIOChannelSocket *sioc)
 #ifdef UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE
     if (sioc->localAddr.ss_family == AF_UNIX) {
         size_t size = UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE;
-        Error *errp = NULL;
+        Error *err = NULL;
 
-        if (qio_channel_socket_set_send_buffer(sioc, size, &errp) < 0) {
-            warn_report_err(errp);
+        if (qio_channel_socket_set_send_buffer(sioc, size, &err) < 0) {
+            warn_report_err(err);
         }
     }
 #endif /* UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE */
-- 
2.49.0

Re: [PATCH 5/5] error: Consistently name Error * objects err, and not errp

[Andrew Cooper]

On 19/11/2025 1:08 pm, Markus Armbruster wrote:
> diff --git a/hw/xen/xen-pvh-common.c b/hw/xen/xen-pvh-common.c
> index b93ff80c85..3e62ec09d0 100644
> --- a/hw/xen/xen-pvh-common.c
> +++ b/hw/xen/xen-pvh-common.c
> @@ -101,7 +101,7 @@ static void xen_create_virtio_mmio_devices(XenPVHMachineState *s)
>  #ifdef CONFIG_TPM
>  static void xen_enable_tpm(XenPVHMachineState *s)
>  {
> -    Error *errp = NULL;
> +    Error *err = NULL;
>      DeviceState *dev;
>      SysBusDevice *busdev;
>  
> @@ -111,8 +111,15 @@ static void xen_enable_tpm(XenPVHMachineState *s)
>          return;
>      }
>      dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> -    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> -    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> +    /*
> +     * FIXME This use of &err is is wrong.  If both calls fail, the
> +     * second will trip error_setv()'s assertion.  If just one call
> +     * fails, we leak an Error object.  Setting the same property
> +     * twice (first to a QOM path, then to an ID string) is almost
> +     * certainly wrong, too.
> +     */
> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &err);
> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &err);

To your question, I don't know the answer, but I think it's far more
likely that the original author didn't grok the proper use of &errp,
than for this behaviour to be intentional.

Surely we just want a failure path and abort the construction if this
goes wrong?

~Andrew

Re: [PATCH 5/5] error: Consistently name Error * objects err, and not errp

[Daniel P. Berrangé]

On Wed, Nov 19, 2025 at 01:22:06PM +0000, Andrew Cooper wrote:
> On 19/11/2025 1:08 pm, Markus Armbruster wrote:
> > diff --git a/hw/xen/xen-pvh-common.c b/hw/xen/xen-pvh-common.c
> > index b93ff80c85..3e62ec09d0 100644
> > --- a/hw/xen/xen-pvh-common.c
> > +++ b/hw/xen/xen-pvh-common.c
> > @@ -101,7 +101,7 @@ static void xen_create_virtio_mmio_devices(XenPVHMachineState *s)
> >  #ifdef CONFIG_TPM
> >  static void xen_enable_tpm(XenPVHMachineState *s)
> >  {
> > -    Error *errp = NULL;
> > +    Error *err = NULL;
> >      DeviceState *dev;
> >      SysBusDevice *busdev;
> >  
> > @@ -111,8 +111,15 @@ static void xen_enable_tpm(XenPVHMachineState *s)
> >          return;
> >      }
> >      dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> > -    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> > -    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> > +    /*
> > +     * FIXME This use of &err is is wrong.  If both calls fail, the
> > +     * second will trip error_setv()'s assertion.  If just one call
> > +     * fails, we leak an Error object.  Setting the same property
> > +     * twice (first to a QOM path, then to an ID string) is almost
> > +     * certainly wrong, too.
> > +     */
> > +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &err);
> > +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &err);
> 
> To your question, I don't know the answer, but I think it's far more
> likely that the original author didn't grok the proper use of &errp,
> than for this behaviour to be intentional.
> 
> Surely we just want a failure path and abort the construction if this
> goes wrong?

In the caller of xen_enable_tpm, we just have error_report+exit calls,
so there's no error propagation ability in the call chain.

The caller will also skip  xen_enable_tpm unless a TPM was explicitly
requested in the config.

Given that, I'm inclined to say that the object_property_set_* calls
in xen_enable_tpm should be using &error_abort, as a failure to setup
the explicitly requested TPM should be fatal.

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 5/5] error: Consistently name Error * objects err, and not errp

[Markus Armbruster]

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

> On Wed, Nov 19, 2025 at 01:22:06PM +0000, Andrew Cooper wrote:
>> On 19/11/2025 1:08 pm, Markus Armbruster wrote:
>> > diff --git a/hw/xen/xen-pvh-common.c b/hw/xen/xen-pvh-common.c
>> > index b93ff80c85..3e62ec09d0 100644
>> > --- a/hw/xen/xen-pvh-common.c
>> > +++ b/hw/xen/xen-pvh-common.c
>> > @@ -101,7 +101,7 @@ static void xen_create_virtio_mmio_devices(XenPVHMachineState *s)
>> >  #ifdef CONFIG_TPM
>> >  static void xen_enable_tpm(XenPVHMachineState *s)
>> >  {
>> > -    Error *errp = NULL;
>> > +    Error *err = NULL;
>> >      DeviceState *dev;
>> >      SysBusDevice *busdev;
>> >  
>> > @@ -111,8 +111,15 @@ static void xen_enable_tpm(XenPVHMachineState *s)
>> >          return;
>> >      }
>> >      dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
>> > -    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
>> > -    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
>> > +    /*
>> > +     * FIXME This use of &err is is wrong.  If both calls fail, the
>> > +     * second will trip error_setv()'s assertion.  If just one call
>> > +     * fails, we leak an Error object.  Setting the same property
>> > +     * twice (first to a QOM path, then to an ID string) is almost
>> > +     * certainly wrong, too.
>> > +     */
>> > +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &err);
>> > +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &err);
>> 
>> To your question, I don't know the answer, but I think it's far more
>> likely that the original author didn't grok the proper use of &errp,
>> than for this behaviour to be intentional.
>> 
>> Surely we just want a failure path and abort the construction if this
>> goes wrong?
>
> In the caller of xen_enable_tpm, we just have error_report+exit calls,
> so there's no error propagation ability in the call chain.
>
> The caller will also skip  xen_enable_tpm unless a TPM was explicitly
> requested in the config.
>
> Given that, I'm inclined to say that the object_property_set_* calls
> in xen_enable_tpm should be using &error_abort, as a failure to setup
> the explicitly requested TPM should be fatal.

I *suspect* that the first call always fails, and the second one always
works.  If that's the case, the fix is to delete the first call, and
pass &error_abort to the second.

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Richard Henderson]

On 11/19/25 14:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   include/hw/loader.h |  4 +++-
>   hw/arm/boot.c       |  6 +-----
>   hw/core/loader.c    |  8 ++++++--
>   hw/riscv/spike.c    | 10 +---------
>   4 files changed, 11 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary conditionals

[Richard Henderson]

On 11/19/25 14:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   hw/acpi/pcihp.c             | 4 +---
>   io/channel-websock.c        | 4 +---
>   io/task.c                   | 4 +---
>   migration/migration.c       | 6 ++----
>   tests/unit/test-smp-parse.c | 5 +----
>   5 files changed, 6 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Vladimir Sementsov-Ogievskiy]

On 19.11.25 16:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>   include/hw/loader.h |  4 +++-
>   hw/arm/boot.c       |  6 +-----
>   hw/core/loader.c    |  8 ++++++--
>   hw/riscv/spike.c    | 10 +---------
>   4 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index d035e72748..6f91703503 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
>    *
>    * Inspect an ELF file's header. Read its full header contents into a
>    * buffer and/or determine if the ELF is 64bit.
> + *
> + * Returns true on success, false on failure.

I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.

>    */
> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>   
>   ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>                     bool big_endian, hwaddr target_page_size);

-- 
Best regards,
Vladimir

Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Vladimir Sementsov-Ogievskiy]

On 19.11.25 16:08, Markus Armbruster wrote:
> bbram_bdrv_error() interpolates a "detail" string into a template with
> error_setg_errno(), then reports the result with error_report().
> Produces error messages with an unwanted '.':
> 
>      BLK-NAME: BBRAM backstore DETAIL failed.: STERROR
> 
> Replace both calls of bbram_bdrv_error() by straightforward
> error_report(), and drop the function.  This is less code, easier to
> read, and the error message is more greppable.
> 
> Also delete the unwanted '.'.

Also, using "errp" name for local "Error *" (one star) variable is a bit misleading.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>   hw/nvram/xlnx-bbram.c | 18 ++++--------------
>   1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/xlnx-bbram.c b/hw/nvram/xlnx-bbram.c
> index 22aefbc240..fe289bad9d 100644
> --- a/hw/nvram/xlnx-bbram.c
> +++ b/hw/nvram/xlnx-bbram.c
> @@ -88,18 +88,6 @@ static bool bbram_pgm_enabled(XlnxBBRam *s)
>       return ARRAY_FIELD_EX32(s->regs, BBRAM_STATUS, PGM_MODE) != 0;
>   }
>   
> -static void bbram_bdrv_error(XlnxBBRam *s, int rc, gchar *detail)
> -{
> -    Error *errp = NULL;
> -
> -    error_setg_errno(&errp, -rc, "%s: BBRAM backstore %s failed.",
> -                     blk_name(s->blk), detail);
> -    error_report("%s", error_get_pretty(errp));
> -    error_free(errp);
> -
> -    g_free(detail);
> -}
> -
>   static void bbram_bdrv_read(XlnxBBRam *s, Error **errp)
>   {
>       uint32_t *ram = &s->regs[R_BBRAM_0];
> @@ -162,7 +150,8 @@ static void bbram_bdrv_sync(XlnxBBRam *s, uint64_t hwaddr)
>       offset = hwaddr - A_BBRAM_0;
>       rc = blk_pwrite(s->blk, offset, 4, &le32, 0);
>       if (rc < 0) {
> -        bbram_bdrv_error(s, rc, g_strdup_printf("write to offset %u", offset));
> +        error_report("%s: BBRAM backstore write to offset %u failed: %s",
> +                     blk_name(s->blk), offset, strerror(-rc));
>       }
>   }
>   
> @@ -178,7 +167,8 @@ static void bbram_bdrv_zero(XlnxBBRam *s)
>   
>       rc = blk_make_zero(s->blk, 0);
>       if (rc < 0) {
> -        bbram_bdrv_error(s, rc, g_strdup("zeroizing"));
> +        error_report("%s: BBRAM backstore zeroizing failed: %s",
> +                     blk_name(s->blk), strerror(-rc));
>       }
>   
>       /* Restore bbram8 if it is non-zero */


-- 
Best regards,
Vladimir

Re: [PATCH 3/5] nbd/client-connection: Replace error_propagate() by assignment

[Vladimir Sementsov-Ogievskiy]

On 19.11.25 16:08, Markus Armbruster wrote:
> connect_thread_func() sets a variable to null, then error_propagate()s
> an Error * to it.  This is a roundabout way to assign the Error * to
> it, so replace it by just that.
> 
> Signed-off-by: Markus Armbruster<armbru@redhat.com>


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

-- 
Best regards,
Vladimir

Re: [PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary conditionals

[Vladimir Sementsov-Ogievskiy]

On 19.11.25 16:08, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary conditionals

[Peter Xu]

On Wed, Nov 19, 2025 at 02:08:54PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Peter Xu]

On Wed, Nov 19, 2025 at 02:08:51PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Peter Xu]

On Wed, Nov 19, 2025 at 02:08:52PM +0100, Markus Armbruster wrote:
> bbram_bdrv_error() interpolates a "detail" string into a template with
> error_setg_errno(), then reports the result with error_report().
> Produces error messages with an unwanted '.':
> 
>     BLK-NAME: BBRAM backstore DETAIL failed.: STERROR
> 
> Replace both calls of bbram_bdrv_error() by straightforward
> error_report(), and drop the function.  This is less code, easier to
> read, and the error message is more greppable.
> 
> Also delete the unwanted '.'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 3/5] nbd/client-connection: Replace error_propagate() by assignment

[Peter Xu]

On Wed, Nov 19, 2025 at 02:08:53PM +0100, Markus Armbruster wrote:
> connect_thread_func() sets a variable to null, then error_propagate()s
> an Error * to it.  This is a roundabout way to assign the Error * to
> it, so replace it by just that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Markus Armbruster]

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

> On 19.11.25 16:08, Markus Armbruster wrote:
>> bbram_bdrv_error() interpolates a "detail" string into a template with
>> error_setg_errno(), then reports the result with error_report().
>> Produces error messages with an unwanted '.':
>>
>>      BLK-NAME: BBRAM backstore DETAIL failed.: STERROR
>>
>> Replace both calls of bbram_bdrv_error() by straightforward
>> error_report(), and drop the function.  This is less code, easier to
>> read, and the error message is more greppable.
>>
>> Also delete the unwanted '.'.
>
> Also, using "errp" name for local "Error *" (one star) variable is a bit misleading.

True.  I don't mention it, because I delete the variable.

My search for misleading uses of @errp led me here.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks!

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Markus Armbruster]

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

> On 19.11.25 16:08, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>> ---
>>   include/hw/loader.h |  4 +++-
>>   hw/arm/boot.c       |  6 +-----
>>   hw/core/loader.c    |  8 ++++++--
>>   hw/riscv/spike.c    | 10 +---------
>>   4 files changed, 11 insertions(+), 17 deletions(-)
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index d035e72748..6f91703503 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
>>    *
>>    * Inspect an ELF file's header. Read its full header contents into a
>>    * buffer and/or determine if the ELF is 64bit.
>> + *
>> + * Returns true on success, false on failure.
>
> I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.

Nearby function comments all have a "Returns" sentence.  I try to blend
in :)

>>    */
>> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>>     ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>>                     bool big_endian, hwaddr target_page_size);

Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Luc Michel]

On 14:08 Wed 19 Nov     , Markus Armbruster wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> bbram_bdrv_error() interpolates a "detail" string into a template with
> error_setg_errno(), then reports the result with error_report().
> Produces error messages with an unwanted '.':
> 
>     BLK-NAME: BBRAM backstore DETAIL failed.: STERROR
> 
> Replace both calls of bbram_bdrv_error() by straightforward
> error_report(), and drop the function.  This is less code, easier to
> read, and the error message is more greppable.
> 
> Also delete the unwanted '.'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Luc Michel <luc.michel@amd.com>

> ---
>  hw/nvram/xlnx-bbram.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/xlnx-bbram.c b/hw/nvram/xlnx-bbram.c
> index 22aefbc240..fe289bad9d 100644
> --- a/hw/nvram/xlnx-bbram.c
> +++ b/hw/nvram/xlnx-bbram.c
> @@ -88,18 +88,6 @@ static bool bbram_pgm_enabled(XlnxBBRam *s)
>      return ARRAY_FIELD_EX32(s->regs, BBRAM_STATUS, PGM_MODE) != 0;
>  }
> 
> -static void bbram_bdrv_error(XlnxBBRam *s, int rc, gchar *detail)
> -{
> -    Error *errp = NULL;
> -
> -    error_setg_errno(&errp, -rc, "%s: BBRAM backstore %s failed.",
> -                     blk_name(s->blk), detail);
> -    error_report("%s", error_get_pretty(errp));
> -    error_free(errp);
> -
> -    g_free(detail);
> -}
> -
>  static void bbram_bdrv_read(XlnxBBRam *s, Error **errp)
>  {
>      uint32_t *ram = &s->regs[R_BBRAM_0];
> @@ -162,7 +150,8 @@ static void bbram_bdrv_sync(XlnxBBRam *s, uint64_t hwaddr)
>      offset = hwaddr - A_BBRAM_0;
>      rc = blk_pwrite(s->blk, offset, 4, &le32, 0);
>      if (rc < 0) {
> -        bbram_bdrv_error(s, rc, g_strdup_printf("write to offset %u", offset));
> +        error_report("%s: BBRAM backstore write to offset %u failed: %s",
> +                     blk_name(s->blk), offset, strerror(-rc));
>      }
>  }
> 
> @@ -178,7 +167,8 @@ static void bbram_bdrv_zero(XlnxBBRam *s)
> 
>      rc = blk_make_zero(s->blk, 0);
>      if (rc < 0) {
> -        bbram_bdrv_error(s, rc, g_strdup("zeroizing"));
> +        error_report("%s: BBRAM backstore zeroizing failed: %s",
> +                     blk_name(s->blk), strerror(-rc));
>      }
> 
>      /* Restore bbram8 if it is non-zero */
> --
> 2.49.0
> 
> 

-- 

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Philippe Mathieu-Daudé]

On 19/11/25 20:12, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 19.11.25 16:08, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>>> ---
>>>    include/hw/loader.h |  4 +++-
>>>    hw/arm/boot.c       |  6 +-----
>>>    hw/core/loader.c    |  8 ++++++--
>>>    hw/riscv/spike.c    | 10 +---------
>>>    4 files changed, 11 insertions(+), 17 deletions(-)
>>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>>> index d035e72748..6f91703503 100644
>>> --- a/include/hw/loader.h
>>> +++ b/include/hw/loader.h
>>> @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
>>>     *
>>>     * Inspect an ELF file's header. Read its full header contents into a
>>>     * buffer and/or determine if the ELF is 64bit.
>>> + *
>>> + * Returns true on success, false on failure.
>>
>> I don't really care, but IMO, it's obvious contract for bool+errp functions, not worth a comment.
> 
> Nearby function comments all have a "Returns" sentence.  I try to blend
> in :)

New developers might just have to look at a particular API such this
loader one, without knowing the global errp contract. With that in
mind, I'll documents @return everywhere.

> 
>>>     */
>>> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>>> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>>>      ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>>>                      bool big_endian, hwaddr target_page_size);
> 

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Daniel Henrique Barboza]

On 11/19/25 10:08 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Nice cleanup


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   include/hw/loader.h |  4 +++-
>   hw/arm/boot.c       |  6 +-----
>   hw/core/loader.c    |  8 ++++++--
>   hw/riscv/spike.c    | 10 +---------
>   4 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index d035e72748..6f91703503 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
>    *
>    * Inspect an ELF file's header. Read its full header contents into a
>    * buffer and/or determine if the ELF is 64bit.
> + *
> + * Returns true on success, false on failure.
>    */
> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>   
>   ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>                     bool big_endian, hwaddr target_page_size);
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b91660208f..06b303aab8 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -766,16 +766,12 @@ static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
>       int data_swab = 0;
>       int elf_data_order;
>       ssize_t ret;
> -    Error *err = NULL;
>   
> -
> -    load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
> -    if (err) {
> +    if (!load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, NULL)) {
>           /*
>            * If the file is not an ELF file we silently return.
>            * The caller will fall back to try other formats.
>            */
> -        error_free(err);
>           return -1;
>       }
>   
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 590c5b02aa..10687fe1c8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -364,8 +364,9 @@ const char *load_elf_strerror(ssize_t error)
>       }
>   }
>   
> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
>   {
> +    bool ok = false;
>       int fd;
>       uint8_t e_ident_local[EI_NIDENT];
>       uint8_t *e_ident;
> @@ -380,7 +381,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
>       fd = open(filename, O_RDONLY | O_BINARY);
>       if (fd < 0) {
>           error_setg_errno(errp, errno, "Failed to open file: %s", filename);
> -        return;
> +        return false;
>       }
>       if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
>           error_setg_errno(errp, errno, "Failed to read file: %s", filename);
> @@ -415,8 +416,11 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
>           off += br;
>       }
>   
> +    ok = true;
> +
>   fail:
>       close(fd);
> +    return ok;
>   }
>   
>   /* return < 0 if error, otherwise the number of bytes loaded in memory */
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index b0bab3fe00..8531e1d121 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -180,15 +180,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>   
>   static bool spike_test_elf_image(char *filename)
>   {
> -    Error *err = NULL;
> -
> -    load_elf_hdr(filename, NULL, NULL, &err);
> -    if (err) {
> -        error_free(err);
> -        return false;
> -    } else {
> -        return true;
> -    }
> +    return load_elf_hdr(filename, NULL, NULL, NULL);
>   }
>   
>   static void spike_board_init(MachineState *machine)

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[BALATON Zoltan]

On Thu, 20 Nov 2025, Daniel Henrique Barboza wrote:
> On 11/19/25 10:08 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Nice cleanup
>
>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
>>   include/hw/loader.h |  4 +++-
>>   hw/arm/boot.c       |  6 +-----
>>   hw/core/loader.c    |  8 ++++++--
>>   hw/riscv/spike.c    | 10 +---------
>>   4 files changed, 11 insertions(+), 17 deletions(-)
>> 
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index d035e72748..6f91703503 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -188,8 +188,10 @@ ssize_t load_elf(const char *filename,
>>    *
>>    * Inspect an ELF file's header. Read its full header contents into a
>>    * buffer and/or determine if the ELF is 64bit.
>> + *
>> + * Returns true on success, false on failure.
>>    */
>> -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp);
>> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp);
>>     ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>>                     bool big_endian, hwaddr target_page_size);
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index b91660208f..06b303aab8 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -766,16 +766,12 @@ static ssize_t arm_load_elf(struct arm_boot_info 
>> *info, uint64_t *pentry,
>>       int data_swab = 0;
>>       int elf_data_order;
>>       ssize_t ret;
>> -    Error *err = NULL;
>>   -
>> -    load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
>> -    if (err) {
>> +    if (!load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, 
>> NULL)) {
>>           /*
>>            * If the file is not an ELF file we silently return.
>>            * The caller will fall back to try other formats.
>>            */
>> -        error_free(err);
>>           return -1;
>>       }
>>   diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 590c5b02aa..10687fe1c8 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -364,8 +364,9 @@ const char *load_elf_strerror(ssize_t error)
>>       }
>>   }
>>   -void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp)
>> +bool load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp)
>>   {
>> +    bool ok = false;
>>       int fd;
>>       uint8_t e_ident_local[EI_NIDENT];
>>       uint8_t *e_ident;
>> @@ -380,7 +381,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool 
>> *is64, Error **errp)
>>       fd = open(filename, O_RDONLY | O_BINARY);
>>       if (fd < 0) {
>>           error_setg_errno(errp, errno, "Failed to open file: %s", 
>> filename);
>> -        return;
>> +        return false;
>>       }
>>       if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
>>           error_setg_errno(errp, errno, "Failed to read file: %s", 
>> filename);
>> @@ -415,8 +416,11 @@ void load_elf_hdr(const char *filename, void *hdr, 
>> bool *is64, Error **errp)
>>           off += br;
>>       }
>>   +    ok = true;
>> +
>>   fail:
>>       close(fd);
>> +    return ok;
>>   }
>>     /* return < 0 if error, otherwise the number of bytes loaded in memory 
>> */
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index b0bab3fe00..8531e1d121 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -180,15 +180,7 @@ static void create_fdt(SpikeState *s, const 
>> MemMapEntry *memmap,
>>     static bool spike_test_elf_image(char *filename)
>>   {
>> -    Error *err = NULL;
>> -
>> -    load_elf_hdr(filename, NULL, NULL, &err);
>> -    if (err) {
>> -        error_free(err);
>> -        return false;
>> -    } else {
>> -        return true;
>> -    }
>> +    return load_elf_hdr(filename, NULL, NULL, NULL);

Does it worth to keep this function or could just be inlined at the two 
callers now that it's equivalent with load_elf_hdr?

Regards,
BALATON Zoltan

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Zhao Liu]

On Wed, Nov 19, 2025 at 02:08:51PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:51 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool,
>  simplify caller
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/loader.h |  4 +++-
>  hw/arm/boot.c       |  6 +-----
>  hw/core/loader.c    |  8 ++++++--
>  hw/riscv/spike.c    | 10 +---------
>  4 files changed, 11 insertions(+), 17 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting

[Zhao Liu]

On Wed, Nov 19, 2025 at 02:08:52PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:52 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error
>  reporting
> 
> bbram_bdrv_error() interpolates a "detail" string into a template with
> error_setg_errno(), then reports the result with error_report().
> Produces error messages with an unwanted '.':
> 
>     BLK-NAME: BBRAM backstore DETAIL failed.: STERROR
> 
> Replace both calls of bbram_bdrv_error() by straightforward
> error_report(), and drop the function.  This is less code, easier to
> read, and the error message is more greppable.
> 
> Also delete the unwanted '.'.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/nvram/xlnx-bbram.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 3/5] nbd/client-connection: Replace error_propagate() by assignment

[Zhao Liu]

On Wed, Nov 19, 2025 at 02:08:53PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:53 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 3/5] nbd/client-connection: Replace error_propagate() by
>  assignment
> 
> connect_thread_func() sets a variable to null, then error_propagate()s
> an Error * to it.  This is a roundabout way to assign the Error * to
> it, so replace it by just that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  nbd/client-connection.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary conditionals

[Zhao Liu]

On Wed, Nov 19, 2025 at 02:08:54PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:54 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 4/5] error: error_free(NULL) is safe, drop unnecessary
>  conditionals
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/acpi/pcihp.c             | 4 +---
>  io/channel-websock.c        | 4 +---
>  io/task.c                   | 4 +---
>  migration/migration.c       | 6 ++----
>  tests/unit/test-smp-parse.c | 5 +----
>  5 files changed, 6 insertions(+), 17 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 5/5] error: Consistently name Error * objects err, and not errp

[Zhao Liu]

On Wed, Nov 19, 2025 at 02:08:55PM +0100, Markus Armbruster wrote:
> Date: Wed, 19 Nov 2025 14:08:55 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: [PATCH 5/5] error: Consistently name Error * objects err, and not
>  errp
> 
> This touches code in xen_enable_tpm() that is obviously wrong.  Since
> I don't know how to fix it properly, I'm adding a FIXME there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/crypto.c          |  8 ++++----
>  hw/acpi/ghes.c          |  8 ++++----
>  hw/ppc/spapr.c          | 16 ++++++++--------
>  hw/xen/xen-pvh-common.c | 13 ++++++++++---
>  nbd/common.c            |  6 +++---
>  5 files changed, 29 insertions(+), 22 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 1/5] hw/core/loader: Make load_elf_hdr() return bool, simplify caller

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 20 Nov 2025, Daniel Henrique Barboza wrote:
>> On 11/19/25 10:08 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>> Nice cleanup
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

[...]

>>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>>> index b0bab3fe00..8531e1d121 100644
>>> --- a/hw/riscv/spike.c
>>> +++ b/hw/riscv/spike.c
>>> @@ -180,15 +180,7 @@ static void create_fdt(SpikeState *s, const 
>>> MemMapEntry *memmap,
>>>     static bool spike_test_elf_image(char *filename)
>>>   {
>>> -    Error *err = NULL;
>>> -
>>> -    load_elf_hdr(filename, NULL, NULL, &err);
>>> -    if (err) {
>>> -        error_free(err);
>>> -        return false;
>>> -    } else {
>>> -        return true;
>>> -    }
>>> +    return load_elf_hdr(filename, NULL, NULL, NULL);
>
> Does it worth to keep this function or could just be inlined at the two 
> callers now that it's equivalent with load_elf_hdr?

Palmer, Alistair, Daniel, got a preference?