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

[BALATON Zoltan via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2937 bytes --]

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

Regards,
BALATON Zoltan