Re: [Qemu-devel] [PATCH v6 1/5] qobject: ensure base is at offset 0

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> On Thu, Apr 19, 2018 at 5:20 PM, Eric Blake <address@hidden> wrote:
>> On 04/19/2018 10:01 AM, Marc-André Lureau wrote:
>>> All QObject types have the base QObject as their first field. This
>>> allows the simplification of qobject_to().
>>>
>>> This explicitly guarantees that existing casts work correctly (even
>>> though we'd prefer to get rid of such casts in any location except the
>>> qobject.h macros)
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>
>> My R-b stands that this is correct from a coding point of view.  But if
>> I read Markus' review correctly, we could omit this patch, fix the one
>> broken client in tests/check-qdict.c to use qobject_to() (why didn't you
>> fix that in v6)?, and then just apply patches 2-5 without this patch,

Is that safe?

>> with no change in behavior and where we are no longer dependent on using
>> offset 0 (even though all current instances do).  So, I'll leave that to
>> maintainer discretion.
>
> I don't think we have a good reason to allow offset different than 0.
> The fact that we have code that rely on that behaviour already is a
> sign that this could easily happen again,

Maybe.  We found one sloppy type cast, which should be cleaned up no
matter what we do with this patch (happy to post the obvious patch
myself).

>                                           because it's the common
> pattern in C for inheritance, and static casting is allowed, for
> better or worse.

"Common way to do single inheritance in C" is a stronger argument.  More
so since QOM does it that way.

We define hundreds of QOM types without ever bothering to check the
super type comes first.  We don't even bother to clearly document it has
to come first.

The fact that we nevertheless haven't seen misuse looks like fairly
strong evidence of a non-problem to me.