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

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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.

> QObjectBase_ is not typedef and use a trailing underscore to make it
> obvious it is not for normal use and to avoid potential abuse.

Trailing underscore I like, lack of typedef I don't mind (but I'm firmly
in the "eschew typedef for structs" camp).  It does violate the common
QEMU coding style, though.

A comment /* Not for use outside include/qapi/qmp/ */ next to
QObjectBase_ wouldn't hurt.

> Signed-off-by: Marc-André Lureau <address@hidden>