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.0On 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>
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
>
>
--
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
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
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!
© 2016 - 2026 Red Hat, Inc.