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>