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

From: Ani Sinha <ani@anisinha.ca>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	jusual@redhat.com, qemu-devel@nongnu.org,
	Paolo Bonzini <pbonzini@redhat.com>, Ani Sinha <ani@anisinha.ca>,
	Igor Mammedov <imammedo@redhat.com>
Subject: Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host
Date: Thu, 29 Jul 2021 10:08:18 +0530 (IST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2107291003380.387306@anisinha-lenovo> (raw)
In-Reply-To: <20210728085437-mutt-send-email-mst@kernel.org>

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.
>

Forgot to answer this. I started this patch because I saw a gap that was
introduced with the above patch. In acpi_pcihp_disable_root_bus(), Julia's
code did not check for null return value from acpi_get_i386_pci_host().
See v2. Hence, I added the fixes tag. Then Igor suggested that I assert
instead and I also thought perhaps assertion is a better idea. Hence v3. I
am not conflicted after reading your argument. We should assert only when
a certain invariant is always respected. Otherwise we should not assert.
If you think acpi_get_i386_pci_host() can be called from non-i386 path as
well, maybe v2 approach is better.