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

Max Reitz posted 7 patches 7 years, 11 months ago
[Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Max Reitz 7 years, 11 months ago
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).

The macro uses lower-case letters because:
1. There does not seem to be a hard rule on whether qemu macros have to
   be upper-cased,
2. The current situation in qapi/qmp is inconsistent (compare e.g.
   QINCREF() vs. qdict_put()),
3. qobject_to() will evaluate its @obj parameter only once, thus it is
   generally not important to the caller whether it is a macro or not,
4. I prefer it aesthetically.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qobject.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 38ac68845c..a448576b46 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -50,6 +50,22 @@ struct QObject {
 #define QDECREF(obj)              \
     qobject_decref(obj ? QOBJECT(obj) : NULL)
 
+/* Required for qobject_to() */
+#define QTYPE_CAST_TO_QNull     QTYPE_QNULL
+#define QTYPE_CAST_TO_QNum      QTYPE_QNUM
+#define QTYPE_CAST_TO_QString   QTYPE_QSTRING
+#define QTYPE_CAST_TO_QDict     QTYPE_QDICT
+#define QTYPE_CAST_TO_QList     QTYPE_QLIST
+#define QTYPE_CAST_TO_QBool     QTYPE_QBOOL
+
+QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
+                   "The QTYPE_CAST_TO_* list needs to be extended");
+
+#define qobject_to(obj, type) \
+    container_of(qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)) ?: \
+                     QOBJECT((type *)NULL), \
+                 type, base)
+
 /* Initialize an object to default values */
 static inline void qobject_init(QObject *obj, QType type)
 {
@@ -102,4 +118,18 @@ static inline QType qobject_type(const QObject *obj)
     return obj->type;
 }
 
+/**
+ * qobject_check_type(): Helper function for the qobject_to() macro.
+ * Return @obj, but only if @obj is not NULL and @type is equal to
+ * @obj's type.  Return NULL otherwise.
+ */
+static inline QObject *qobject_check_type(const QObject *obj, QType type)
+{
+    if (obj && qobject_type(obj) == type) {
+        return (QObject *)obj;
+    } else {
+        return NULL;
+    }
+}
+
 #endif /* QOBJECT_H */
-- 
2.14.3


Re: [Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Eric Blake 7 years, 11 months ago
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).
> 
> The macro uses lower-case letters because:
> 1. There does not seem to be a hard rule on whether qemu macros have to
>     be upper-cased,
> 2. The current situation in qapi/qmp is inconsistent (compare e.g.
>     QINCREF() vs. qdict_put()),
> 3. qobject_to() will evaluate its @obj parameter only once, thus it is
>     generally not important to the caller whether it is a macro or not,
> 4. I prefer it aesthetically.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/qapi/qmp/qobject.h | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 

> +++ b/include/qapi/qmp/qobject.h
> @@ -50,6 +50,22 @@ struct QObject {
>   #define QDECREF(obj)              \
>       qobject_decref(obj ? QOBJECT(obj) : NULL)
>   
> +/* Required for qobject_to() */
> +#define QTYPE_CAST_TO_QNull     QTYPE_QNULL
> +#define QTYPE_CAST_TO_QNum      QTYPE_QNUM
> +#define QTYPE_CAST_TO_QString   QTYPE_QSTRING
> +#define QTYPE_CAST_TO_QDict     QTYPE_QDICT
> +#define QTYPE_CAST_TO_QList     QTYPE_QLIST
> +#define QTYPE_CAST_TO_QBool     QTYPE_QBOOL
> +
> +QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
> +                   "The QTYPE_CAST_TO_* list needs to be extended");
> +
> +#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.

> +                 type, base)
> +

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Max Reitz 7 years, 11 months ago
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).
>>
>> The macro uses lower-case letters because:
>> 1. There does not seem to be a hard rule on whether qemu macros have to
>>     be upper-cased,
>> 2. The current situation in qapi/qmp is inconsistent (compare e.g.
>>     QINCREF() vs. qdict_put()),
>> 3. qobject_to() will evaluate its @obj parameter only once, thus it is
>>     generally not important to the caller whether it is a macro or not,
>> 4. I prefer it aesthetically.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   include/qapi/qmp/qobject.h | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
> 
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -50,6 +50,22 @@ struct QObject {
>>   #define QDECREF(obj)              \
>>       qobject_decref(obj ? QOBJECT(obj) : NULL)
>>   +/* Required for qobject_to() */
>> +#define QTYPE_CAST_TO_QNull     QTYPE_QNULL
>> +#define QTYPE_CAST_TO_QNum      QTYPE_QNUM
>> +#define QTYPE_CAST_TO_QString   QTYPE_QSTRING
>> +#define QTYPE_CAST_TO_QDict     QTYPE_QDICT
>> +#define QTYPE_CAST_TO_QList     QTYPE_QLIST
>> +#define QTYPE_CAST_TO_QBool     QTYPE_QBOOL
>> +
>> +QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
>> +                   "The QTYPE_CAST_TO_* list needs to be extended");
>> +
>> +#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)

Max

> 
>> +                 type, base)
>> +
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


Re: [Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Eric Blake 7 years, 10 months ago
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)


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Max Reitz 7 years, 10 months ago
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

Re: [Qemu-devel] [PATCH v3 2/7] qapi: Add qobject_to()
Posted by Alberto Garcia 7 years, 11 months ago
On Sat 24 Feb 2018 04:40:28 PM CET, 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).
>
> The macro uses lower-case letters because:
> 1. There does not seem to be a hard rule on whether qemu macros have to
>    be upper-cased,
> 2. The current situation in qapi/qmp is inconsistent (compare e.g.
>    QINCREF() vs. qdict_put()),
> 3. qobject_to() will evaluate its @obj parameter only once, thus it is
>    generally not important to the caller whether it is a macro or not,
> 4. I prefer it aesthetically.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto