[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

From: BALATON Zoltan
Subject: Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set
Date: Mon, 19 Oct 2020 13:11:42 +0200 (CEST)

On Mon, 19 Oct 2020, Markus Armbruster wrote:
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
On 10/12/20 1:50 PM, BALATON Zoltan via wrote:
On Mon, 12 Oct 2020, David Gibson wrote:
On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe
Mathieu-Daudé wrote:
On 10/12/20 12:34 AM, David Gibson wrote:
On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe
Mathieu-Daudé wrote:
The Grackle PCI host model expects the interrupt controller
being set, but does not verify it is present. Add a check to
help developers using this model.

I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2

Do you want I correct the description as:
"The Grackle PCI host model expects the interrupt controller
being set, but does not verify it is present.
A developer basing its implementation on the Grackle model
might hit this problem. Add a check to help future developers
using this model as reference."?

No, it's fine.  All I was saying is that the chances of anyone using
Grackle in future seem very low to me.
So maybe an assert instead of a user visible error would be enough?

My understanding is realize() shouldn't abort()
(the caller might choose to by using &error_abort).

assert() is for checking invariants.  A violated invariant is a
programming error: developers screwed up, safe recovery is impossible.

Abusing assert() to catch errors that are not programming errors is

You may check invariants with assert() anywhere in the code.

You should not misuse assert() anywhere in the code.

Sometimes, an error condition that is *not* a programming error in the
function where it is detected *is* a programming error for certain
calls.  Having these calls pass &error_abort is a common solution for
this problem.

Hope this helps.

Helps just a bit but after reading this I'm still confused if this particular case should be assert or ser error.

I was suggesting assert and I think it's a programming error to use the grackle model without setting PIC link but not sure if users can also create this instance via command line (even if it does not make much sense) in which case it's probably better to return error. Having all devices user creatable via -device without a way to describe their dependencies is a nice way to make all sorts of errors possible. But maybe aborting with assert during creation of the machine is still OK. If people device_add a model later and that crashes then it's their problem. Unless we want to avoid that being used as DoS in enterprise setting. So maybe we should never abort then if there's a way to fail with an error instead.

I can look at this problem from different angles and all seem to be plausible resulting in different solutions.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]