Re: [Qemu-block] [PATCH v3 2/7] qapi: Add qobject_to()

[Max Reitz]

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