| Message ID | 20231120130017.81286-1-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCH-for-8.2?] hw/acpi/erst: Do not ignore Error* in realize handler | expand |
> 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 >
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>
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);
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(+)