[PATCH-for-8.2?] hw/acpi/erst: Do not ignore Error* in realize handler

[PATCH-for-8.2?] hw/acpi/erst: Do not ignore Error* in realize handler

[Philippe Mathieu-Daudé]

erst_realizefn() calls functions which could update the 'errp'
argument, but then ignores it. Use the ERRP_GUARD() macro and
check *errp, as suggested in commit ae7c80a7bd ("error: New macro
ERRP_GUARD()").

Cc: qemu-stable@nongnu.org
Fixes: f7e26ffa59 ("ACPI ERST: support for ACPI ERST feature")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/acpi/erst.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
index 35007d8017..ba751dc60e 100644
--- a/hw/acpi/erst.c
+++ b/hw/acpi/erst.c
@@ -947,6 +947,7 @@ static const VMStateDescription erst_vmstate  = {
 
 static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
 {
+    ERRP_GUARD();
     ERSTDeviceState *s = ACPIERST(pci_dev);
 
     trace_acpi_erst_realizefn_in();
@@ -964,9 +965,15 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
 
     /* HostMemoryBackend size will be multiple of PAGE_SIZE */
     s->storage_size = object_property_get_int(OBJECT(s->hostmem), "size", errp);
+    if (*errp) {
+        return;
+    }
 
     /* Initialize backend storage and record_count */
     check_erst_backend_storage(s, errp);
+    if (*errp) {
+        return;
+    }
 
     /* BAR 0: Programming registers */
     memory_region_init_io(&s->iomem_mr, OBJECT(pci_dev), &erst_reg_ops, s,
@@ -977,6 +984,9 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
     memory_region_init_ram(&s->exchange_mr, OBJECT(pci_dev),
                             "erst.exchange",
                             le32_to_cpu(s->header->record_size), errp);
+    if (*errp) {
+        return;
+    }
     pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
                         &s->exchange_mr);
 
-- 
2.41.0

Re: [PATCH-for-8.2?] hw/acpi/erst: Do not ignore Error* in realize handler

[Ani Sinha]

> On 20-Nov-2023, at 6:30 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> erst_realizefn() calls functions which could update the 'errp'
> argument, but then ignores it. Use the ERRP_GUARD() macro and
> check *errp, as suggested in commit ae7c80a7bd ("error: New macro
> ERRP_GUARD()").
> 
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa59 ("ACPI ERST: support for ACPI ERST feature")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
> hw/acpi/erst.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index 35007d8017..ba751dc60e 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -947,6 +947,7 @@ static const VMStateDescription erst_vmstate  = {
> 
> static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
> {
> +    ERRP_GUARD();
>     ERSTDeviceState *s = ACPIERST(pci_dev);
> 
>     trace_acpi_erst_realizefn_in();
> @@ -964,9 +965,15 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
> 
>     /* HostMemoryBackend size will be multiple of PAGE_SIZE */
>     s->storage_size = object_property_get_int(OBJECT(s->hostmem), "size", errp);
> +    if (*errp) {
> +        return;
> +    }
> 
>     /* Initialize backend storage and record_count */
>     check_erst_backend_storage(s, errp);
> +    if (*errp) {
> +        return;
> +    }
> 
>     /* BAR 0: Programming registers */
>     memory_region_init_io(&s->iomem_mr, OBJECT(pci_dev), &erst_reg_ops, s,
> @@ -977,6 +984,9 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
>     memory_region_init_ram(&s->exchange_mr, OBJECT(pci_dev),
>                             "erst.exchange",
>                             le32_to_cpu(s->header->record_size), errp);
> +    if (*errp) {
> +        return;
> +    }
>     pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                         &s->exchange_mr);
> 
> -- 
> 2.41.0
> 

Re: [PATCH-for-8.2?] hw/acpi/erst: Do not ignore Error* in realize handler

[Markus Armbruster]

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

> erst_realizefn() calls functions which could update the 'errp'
> argument, but then ignores it.

To be precise: it ignores failure.  Suggest to clarify the commit
message like this:

  erst_realizefn() passes @errp to functions without checking for
  failure.  If it runs into another failure, it trips error_setv()'s
  assertion.

>                                Use the ERRP_GUARD() macro and
> check *errp, as suggested in commit ae7c80a7bd ("error: New macro
> ERRP_GUARD()").
>
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa59 ("ACPI ERST: support for ACPI ERST feature")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/acpi/erst.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index 35007d8017..ba751dc60e 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -947,6 +947,7 @@ static const VMStateDescription erst_vmstate  = {
>  
>  static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      ERSTDeviceState *s = ACPIERST(pci_dev);
>  
>      trace_acpi_erst_realizefn_in();
> @@ -964,9 +965,15 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
>  
>      /* HostMemoryBackend size will be multiple of PAGE_SIZE */
>      s->storage_size = object_property_get_int(OBJECT(s->hostmem), "size", errp);
> +    if (*errp) {
> +        return;
> +    }
>  
>      /* Initialize backend storage and record_count */
>      check_erst_backend_storage(s, errp);
> +    if (*errp) {
> +        return;
> +    }

If you change check_erst_backend_storage() to return bool, you can use

       if (!check_erst_backend_storage(s, errp) {
           return;
       }

Not a demand.

>  
>      /* BAR 0: Programming registers */
>      memory_region_init_io(&s->iomem_mr, OBJECT(pci_dev), &erst_reg_ops, s,
> @@ -977,6 +984,9 @@ static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
>      memory_region_init_ram(&s->exchange_mr, OBJECT(pci_dev),
>                              "erst.exchange",
>                              le32_to_cpu(s->header->record_size), errp);
> +    if (*errp) {
> +        return;
> +    }

Likewise, with more callers to simplify.  Again, not a demand.

>      pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                          &s->exchange_mr);

With the commit message clarified:
Reviewed-by: Markus Armbruster <armbru@redhat.com>