Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

[Ani Sinha <ani@anisinha.ca>]

On Wed, 28 Jul 2021, Michael S. Tsirkin wrote:

> On Mon, Jul 26, 2021 at 10:27:43PM +0530, Ani Sinha wrote:
> > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > return value from this function call. Instead of returning early when the value
> > returned is NULL, assert instead. Since there are only two possible host buses
> > for i386 - q35 and i440fx, a null value return from the function does not make
> > sense in most cases and is likely an error situation.
>
> add "on i386"?
>
> > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>
> This that seems inappropriate, this is not a bugfix.
>
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
>
> Frankly I don't see this as a useful cleanup.
> assert is generally a last resort thing.
>

Igor pushed in the direction of assertion. Otherwise, see my v2.

> > ---
> >  hw/acpi/pcihp.c      |  8 ++++++++
> >  hw/i386/acpi-build.c | 15 ++++++---------
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > changelog:
> > v1: initial patch
> > v2: removed comment addition - that can be sent as a separate patch.
> > v3: added assertion for null host values for all cases except one.
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f4d706e47d..054ee8cbc5 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >      bsel_is_set = true;
> >
> >      if (!host) {
> > +        /*
> > +         * This function can be eventually called from
> > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > +         * for architectures other than i386. Hence, we need
>
> why call out i386 here? well because currently host
> is only non-null for q35 and i440fx which are both i386.
> all the above is not a given and we won't remember to update
> the comment if we change it. Generally graceful failure
> is the default or should be.

Hmm. there is much debate to be had about graceful and unfraceful
failures :-) Some might say ungraceful failures helps to catch issues
earlier before the state is messed up.