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

[Markus Armbruster <armbru@redhat.com>]

BALATON Zoltan via <qemu-devel@nongnu.org> writes:

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

They can't: "info qdm" shows

    name "grackle-pcihost", bus System, no-user
                                        ~~~~~~~

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

As long as the device is no-user, asserting that properties have sane
values feels reasonable enough to me.

Setting an error instead is not wrong, of course.