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

[Markus Armbruster]

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

> On Tue, 20 Oct 2020, Markus Armbruster wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>>
>>> One thing I have thought about is being able to mark a link property
>>> as mandatory so if a value hasn't been set before realize then you
>>
>> A non-null value, I presume.
>
> Do you mean something like distinguish between NULL and INVALID_VALUE
> where setting the latter as initial value means property is mandatory?

I doubt "somebody must have set some value (which could be null)" is
useful here.  I believe Mark was thinking about "somebody must have
connected the link (i.e. set a non-null value)".

>>> receive a fatal error. This would be for cases like this where 2
>>> internal devices are connected together without any formal interface,
>>> i.e. in cases where -device wouldn't work anyway.
>>
>> Moves the check from code one step closer to data: from the realize
>> method to the object_property_add_link() call.
>>
>> I like doing things in data, because data is easier to reason about than
>> code.
>
> Except when object initialisation is scattered around in boiler plate
> code as in QEMU where it may not be obvious why a realize method fails
> due to something set/documented in a class_init method elsewhere,
> whereas an assert in the realize method is quite self evident and
> would document the same requirements.

Two aspects.

One, making sense of an assertion failure.  Whether some ad hoc

    assert(s->some_other_object);

in the device realize method crashes, or a generic

    if (!object_property_get_link(s, prop->name, &error_abort)) {
        error_setg(errp, "link %s must be connected", prop->name);
    }

crashes in the error_setg() is all the same to me.  Some developers may
find the latter's crash message superior (I don't care).

Two, making sense of the code.  You have to consider property definition
(which might be code in .class_init(), code in .instance_init(), or data
passed to .device_class_set_props()), property use (in the code that
sets up the device), and property value checking (generic checks in
qom/object.c, specific checks in .realize()).  Moving one check from
"specific" to "generic" won't make much of a difference.