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

Markus Armbruster posted 5 patches 23 hours ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Dongjiu Geng <gengdongjiu1@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Palmer Dabbelt <palmer@dabbelt.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, Paul Durrant <paul@xen.org>, "Daniel P. Berrangé" <berrange@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>
[PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
Posted by Markus Armbruster 23 hours ago
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
Re: [PATCH 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
Posted by Luc Michel 4 hours ago
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 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
Posted by Peter Xu 18 hours ago
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 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
Posted by Vladimir Sementsov-Ogievskiy 19 hours ago
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 2/5] hw/nvram/xlnx-bbram: More idiomatic and simpler error reporting
Posted by Markus Armbruster 17 hours ago
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!