diff mbox

[v2,2/3] qobject: introduce QObjectCommon

Message ID 20180328130723.20831-3-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc-André Lureau March 28, 2018, 1:07 p.m. UTC 
By moving the common fields to a QObjectCommon, QObject can be a type
which also has a 'base' QObjectCommon 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 QObjectCommon base field, give me 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
previous patch)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/qmp/qbool.h   |  2 +-
 include/qapi/qmp/qdict.h   |  2 +-
 include/qapi/qmp/qlist.h   |  2 +-
 include/qapi/qmp/qnull.h   |  2 +-
 include/qapi/qmp/qnum.h    |  2 +-
 include/qapi/qmp/qobject.h | 27 ++++++++++++++++-----------
 include/qapi/qmp/qstring.h |  2 +-
 qobject/qobject.c          | 12 ++++++------
 tests/check-qdict.c        |  6 +++---
 9 files changed, 31 insertions(+), 26 deletions(-)

Comments

Eric Blake March 28, 2018, 1:45 p.m. UTC | #1 
On 03/28/2018 08:07 AM, Marc-André Lureau wrote:
> By moving the common fields to a QObjectCommon, QObject can be a type
> which also has a 'base' QObjectCommon 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 QObjectCommon base field, give me 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
> previous patch)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

> +++ b/include/qapi/qmp/qbool.h
> @@ -17,7 +17,7 @@
>   #include "qapi/qmp/qobject.h"
>   
>   struct QBool {
> -    QObject base;
> +    struct QObjectCommon base;

Why no 'typedef struct QObjectCommon QObjectCommon' in scope to make 
this simpler?

> -/* Get the 'base' part of an object */
> -#define QOBJECT(obj) (&(obj)->base)
> +struct QObject {
> +    struct QObjectCommon base;
> +};
> +
> +#define QOBJECT(x) \
> +   container_of(&(x)->base, QObject, base)

If I understand correctly, this still causes clang complaints when 
called as QOBJECT(NULL).  As long as we are touching this, should we 
improve this macro to be friendly to NULL conversion?
Marc-André Lureau March 28, 2018, 1:48 p.m. UTC | #2 
Hi

On Wed, Mar 28, 2018 at 3:45 PM, Eric Blake <eblake@redhat.com> wrote:
> On 03/28/2018 08:07 AM, Marc-André Lureau wrote:
>>
>> By moving the common fields to a QObjectCommon, QObject can be a type
>> which also has a 'base' QObjectCommon 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 QObjectCommon base field, give me 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
>> previous patch)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>
>
>> +++ b/include/qapi/qmp/qbool.h
>> @@ -17,7 +17,7 @@
>>   #include "qapi/qmp/qobject.h"
>>     struct QBool {
>> -    QObject base;
>> +    struct QObjectCommon base;
>
>
> Why no 'typedef struct QObjectCommon QObjectCommon' in scope to make this
> simpler?

We could (not having the typedef enforces the feeling that
QObjectCommon is internal in my mind, but ymmv)

>> -/* Get the 'base' part of an object */
>> -#define QOBJECT(obj) (&(obj)->base)
>> +struct QObject {
>> +    struct QObjectCommon base;
>> +};
>> +
>> +#define QOBJECT(x) \
>> +   container_of(&(x)->base, QObject, base)
>
>
> If I understand correctly, this still causes clang complaints when called as
> QOBJECT(NULL).  As long as we are touching this, should we improve this
> macro to be friendly to NULL conversion?

I don't see much need for allowing NULL (literally) to be passed to
QOBJECT(). If it's a null pointer, as long as it has the right type,
it should be fine, shouldn't it?

> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
Eric Blake March 28, 2018, 1:55 p.m. UTC | #3 
On 03/28/2018 08:48 AM, Marc-André Lureau wrote:

>>> +#define QOBJECT(x) \
>>> +   container_of(&(x)->base, QObject, base)
>>
>>
>> If I understand correctly, this still causes clang complaints when called as
>> QOBJECT(NULL).  As long as we are touching this, should we improve this
>> macro to be friendly to NULL conversion?
> 
> I don't see much need for allowing NULL (literally) to be passed to
> QOBJECT(). If it's a null pointer, as long as it has the right type,
> it should be fine, shouldn't it?

Not with clang ubsan (okay, the failure is at runtime, not compile time):

https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05148.html

Even when the offset is 0, the mere fact that you are computing an 
offset relative to a NULL pointer is undefined behavior.
Paolo Bonzini March 28, 2018, 2:11 p.m. UTC | #4 
On 28/03/2018 15:55, Eric Blake wrote:
> On 03/28/2018 08:48 AM, Marc-André Lureau wrote:
> 
>>>> +#define QOBJECT(x) \
>>>> +   container_of(&(x)->base, QObject, base)
>>>
>>>
>>> If I understand correctly, this still causes clang complaints when
>>> called as
>>> QOBJECT(NULL).  As long as we are touching this, should we improve this
>>> macro to be friendly to NULL conversion?
>>
>> I don't see much need for allowing NULL (literally) to be passed to
>> QOBJECT(). If it's a null pointer, as long as it has the right type,
>> it should be fine, shouldn't it?
> 
> Not with clang ubsan (okay, the failure is at runtime, not compile time):
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05148.html
> 
> Even when the offset is 0, the mere fact that you are computing an
> offset relative to a NULL pointer is undefined behavior.

Are there cases where we are passing NULL to qobject_{inc,dec}ref?  They
currently have an "if" in they're body, but my opinion is they ought to
crash and burn...

Paolo
Marc-André Lureau March 28, 2018, 2:19 p.m. UTC | #5 
Hi

On Wed, Mar 28, 2018 at 4:11 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 28/03/2018 15:55, Eric Blake wrote:
>> On 03/28/2018 08:48 AM, Marc-André Lureau wrote:
>>
>>>>> +#define QOBJECT(x) \
>>>>> +   container_of(&(x)->base, QObject, base)
>>>>
>>>>
>>>> If I understand correctly, this still causes clang complaints when
>>>> called as
>>>> QOBJECT(NULL).  As long as we are touching this, should we improve this
>>>> macro to be friendly to NULL conversion?
>>>
>>> I don't see much need for allowing NULL (literally) to be passed to
>>> QOBJECT(). If it's a null pointer, as long as it has the right type,
>>> it should be fine, shouldn't it?
>>
>> Not with clang ubsan (okay, the failure is at runtime, not compile time):
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05143.html
>> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05148.html
>>
>> Even when the offset is 0, the mere fact that you are computing an
>> offset relative to a NULL pointer is undefined behavior.
>
> Are there cases where we are passing NULL to qobject_{inc,dec}ref?  They
> currently have an "if" in they're body, but my opinion is they ought to
> crash and burn...

I find it convenient that unref() accepts NULL, just like free().

However, I agree than ref() should crash if given a NULL pointer.

While at it, I'd also prefer ref() to return a pointer to the same object...
diff mbox

Patch

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index b9a44a1bfe..38e6287973 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -17,7 +17,7 @@ 
 #include "qapi/qmp/qobject.h"
 
 struct QBool {
-    QObject base;
+    struct QObjectCommon base;
     bool value;
 };
 
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 2cc3e906f7..3e54df2291 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -25,7 +25,7 @@  typedef struct QDictEntry {
 } QDictEntry;
 
 struct QDict {
-    QObject base;
+    struct QObjectCommon base;
     size_t size;
     QLIST_HEAD(,QDictEntry) table[QDICT_BUCKET_MAX];
 };
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 5c673acb06..5cf26554ae 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -22,7 +22,7 @@  typedef struct QListEntry {
 } QListEntry;
 
 struct QList {
-    QObject base;
+    struct QObjectCommon base;
     QTAILQ_HEAD(,QListEntry) head;
 };
 
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index c992ee2ae1..32d4ce98af 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -16,7 +16,7 @@ 
 #include "qapi/qmp/qobject.h"
 
 struct QNull {
-    QObject base;
+    struct QObjectCommon base;
 };
 
 extern QNull qnull_;
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 3e47475b2c..3326547f2c 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -45,7 +45,7 @@  typedef enum {
  * convert under the hood.
  */
 struct QNum {
-    QObject base;
+    struct QObjectCommon base;
     QNumKind kind;
     union {
         int64_t i64;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 5206ff9ee1..4fc59b54d3 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -34,13 +34,17 @@ 
 
 #include "qapi/qapi-builtin-types.h"
 
-struct QObject {
+struct QObjectCommon {
     QType type;
     size_t refcnt;
 };
 
-/* Get the 'base' part of an object */
-#define QOBJECT(obj) (&(obj)->base)
+struct QObject {
+    struct QObjectCommon base;
+};
+
+#define QOBJECT(x) \
+   container_of(&(x)->base, QObject, base)
 
 /* High-level interface for qobject_incref() */
 #define QINCREF(obj)      \
@@ -68,8 +72,8 @@  QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
 static inline void qobject_init(QObject *obj, QType type)
 {
     assert(QTYPE_NONE < type && type < QTYPE__MAX);
-    obj->refcnt = 1;
-    obj->type = type;
+    obj->base.refcnt = 1;
+    obj->base.type = type;
 }
 
 /**
@@ -77,8 +81,9 @@  static inline void qobject_init(QObject *obj, QType type)
  */
 static inline void qobject_incref(QObject *obj)
 {
-    if (obj)
-        obj->refcnt++;
+    if (obj) {
+        obj->base.refcnt++;
+    }
 }
 
 /**
@@ -101,8 +106,8 @@  void qobject_destroy(QObject *obj);
  */
 static inline void qobject_decref(QObject *obj)
 {
-    assert(!obj || obj->refcnt);
-    if (obj && --obj->refcnt == 0) {
+    assert(!obj || obj->base.refcnt);
+    if (obj && --obj->base.refcnt == 0) {
         qobject_destroy(obj);
     }
 }
@@ -112,8 +117,8 @@  static inline void qobject_decref(QObject *obj)
  */
 static inline QType qobject_type(const QObject *obj)
 {
-    assert(QTYPE_NONE < obj->type && obj->type < QTYPE__MAX);
-    return obj->type;
+    assert(QTYPE_NONE < obj->base.type && obj->base.type < QTYPE__MAX);
+    return obj->base.type;
 }
 
 /**
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 30ae260a7f..dc50f6a0c7 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -16,7 +16,7 @@ 
 #include "qapi/qmp/qobject.h"
 
 struct QString {
-    QObject base;
+    struct QObjectCommon base;
     char *string;
     size_t length;
     size_t capacity;
diff --git a/qobject/qobject.c b/qobject/qobject.c
index 87649c5be5..cf4b7e229e 100644
--- a/qobject/qobject.c
+++ b/qobject/qobject.c
@@ -37,9 +37,9 @@  static void (*qdestroy[QTYPE__MAX])(QObject *) = {
 
 void qobject_destroy(QObject *obj)
 {
-    assert(!obj->refcnt);
-    assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
-    qdestroy[obj->type](obj);
+    assert(!obj->base.refcnt);
+    assert(QTYPE_QNULL < obj->base.type && obj->base.type < QTYPE__MAX);
+    qdestroy[obj->base.type](obj);
 }
 
 
@@ -62,11 +62,11 @@  bool qobject_is_equal(const QObject *x, const QObject *y)
         return true;
     }
 
-    if (!x || !y || x->type != y->type) {
+    if (!x || !y || x->base.type != y->base.type) {
         return false;
     }
 
-    assert(QTYPE_NONE < x->type && x->type < QTYPE__MAX);
+    assert(QTYPE_NONE < x->base.type && x->base.type < QTYPE__MAX);
 
-    return qis_equal[x->type](x, y);
+    return qis_equal[x->base.type](x, y);
 }
diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index 2e73c2f86e..029b6b15b9 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -570,11 +570,11 @@  static void qdict_join_test(void)
         }
 
         /* Check the references */
-        g_assert(qdict_get(dict1, "foo")->refcnt == 1);
-        g_assert(qdict_get(dict1, "bar")->refcnt == 1);
+        g_assert(qdict_get(dict1, "foo")->base.refcnt == 1);
+        g_assert(qdict_get(dict1, "bar")->base.refcnt == 1);
 
         if (!overwrite) {
-            g_assert(qdict_get(dict2, "foo")->refcnt == 1);
+            g_assert(qdict_get(dict2, "foo")->base.refcnt == 1);
         }
 
         /* Clean up */