Re: [Qemu-devel] [PATCH v4 2/4] qobject: use a QObjectBase_ struct

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

On Mon, Apr 16, 2018 at 10:31 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>>> By moving the base fields to a QObjectBase_, QObject can be a type
>>> which also has a 'base' field. This allows to write a generic
>>> QOBJECT() macro that will work with any QObject type, including
>>> QObject itself. The container_of() macro ensures that the object to
>>> cast has a QObjectBase_ base field, giving some type safety
>>> guarantees. However, for it to work properly, all QObject types must
>>> have 'base' at offset 0 (which is ensured by static checking from
>>> the previous patch)
>>
>> I'm afraid this condition is neither sufficient nor necessary.
>>
>> QOBJECT() maps a pointer to some subtype to the base type QObject.  For
>> this to work, the subtype must contain a QObject.
>>
>> Before the patch, this is trivially the case: the subtypes have a member
>> QObject base, and QOBJECT() returns its address.
>>
>> Afterwards, the subtypes have a member QObjectBase_ base, and QOBJECT()
>> returns the address of a notional QObject wrapped around this member.
>> Works because QObject has no other members.
>>
>> The condition 'base is at offset 0' does not ensure this, and is
>> therefore not sufficient.
>>
>> It's not necessary, either: putting base elsewhere would work just fine
>> as long as we put *all* of QObject there.
>>
>> Please document the real condition "QObject must have no members but
>> QObjectBase_ base, or else QOBJECT() breaks".  Feel free to check their
>> sizes are the same (I wouldn't bother).
>>
>> If requiring base to be at offset zero for all subtypes materially
>> simplifies code, then go ahead and do that in a separate patch.  My gut
>> feeling is it doesn't, but I could be wrong.
>
> Uh, there's another reason: existing type casts from QObject * to
> subtypes.  I just spotted one in tests/check-qdict.c:
>
>     dst = (QDict *)qdict_crumple(src, &error_abort);
>
> There may well be more.
>

So we better have checks from patch 1, don't we?