On 2018-03-19 20:36, Eric Blake wrote: > On 02/26/2018 05:58 AM, Max Reitz wrote: >> On 2018-02-24 21:57, Eric Blake wrote: >>> On 02/24/2018 09:40 AM, Max Reitz wrote: >>>> This is a dynamic casting macro that, given a QObject type, returns an >>>> object as that type or NULL if the object is of a different type (or >>>> NULL itself). >>>> > >>>> +#define qobject_to(obj, type) \ >>>> + container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) >>>> ?: \ >>>> + QOBJECT((type *)NULL), \ >>> >>> I guess the third (second?) branch of the ternary is written this way, >>> rather than the simpler 'NULL', to ensure that 'type' is still something >>> that can have the QOBJECT() macro applied to it? Should be okay. >> >> It's written this way because of the container_of() around it. We want >> the whole expression to return NULL then, and without the QOBJECT() >> around it, it would only return NULL if offsetof(type, base) == 0 (which >> it is not necessarily). >> >> OTOH, container_of(&((type *)NULL)->base, type, base) is by definition >> NULL. >> >> (QOBJECT(x) is &(x)->base) > > Well, clang's ubsan griped: > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html > > Practically, all of our qtypes have 'base' at offset 0, which means > (QObject*)addr and (QString*)addr are the same address, even when addr > is NULL. But neither QOBJECT() nor container_of() are currently fit to > run on a NULL pointer, since the 'base' member need not be at offset 0, > at which point, we'd be converting away from the NULL pointer on the > &(x)->base conversion, and then back to the NULL pointer on the > container_of() conversion. So at the end of the day, we get the right > results, but we relied on undefined behavior in the interim. > > So here's what I'm squashing in, if you like it (and remembering that I > already swapped argument order to be qobject_to(type, obj) in my pending > pull requests): > > diff --git i/include/qapi/qmp/qobject.h w/include/qapi/qmp/qobject.h > index ea9702270e7..e6ce9347ab8 100644 > --- i/include/qapi/qmp/qobject.h > +++ w/include/qapi/qmp/qobject.h > @@ -62,9 +62,8 @@ QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7, > "The QTYPE_CAST_TO_* list needs to be extended"); > > #define qobject_to(type, obj) \ > - container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \ > - QOBJECT((type *)NULL), \ > - type, base) > + ({ QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, > type)); \ > + _tmp ? container_of(_tmp, type, base) : (type *)NULL; }) > > /* Initialize an object to default values */ > static inline void qobject_init(QObject *obj, QType type)
Yes, that looks good. Thanks! Max
signature.asc
Description: OpenPGP digital signature
