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)
QObjectBase_ is not typedef and use a trailing underscore to make it
obvious it is not for normal use and to avoid potential abuse.
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 | 29 ++++++++++++++++++-----------
include/qapi/qmp/qstring.h | 2 +-
qobject/qobject.c | 12 ++++++------
tests/check-qdict.c | 6 +++---
9 files changed, 33 insertions(+), 26 deletions(-)
diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index b9a44a1bfe..5f61e38e64 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 QObjectBase_ base;
bool value;
};
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 2cc3e906f7..921a28d2d3 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 QObjectBase_ 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..8d2c32ca28 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 QObjectBase_ base;
QTAILQ_HEAD(,QListEntry) head;
};
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index c992ee2ae1..e8ea2c315a 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 QObjectBase_ base;
};
extern QNull qnull_;
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index 3e47475b2c..45bf02a036 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 QObjectBase_ base;
QNumKind kind;
union {
int64_t i64;
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 5206ff9ee1..7e6fed242e 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -34,13 +34,19 @@
#include "qapi/qapi-builtin-types.h"
-struct QObject {
+struct QObjectBase_ {
QType type;
size_t refcnt;
};
-/* Get the 'base' part of an object */
-#define QOBJECT(obj) (&(obj)->base)
+struct QObject {
+ struct QObjectBase_ base;
+};
+
+#define QOBJECT(x) ({ \
+ typeof(x) __x = (x); \
+ __x ? container_of(&(__x)->base, QObject, base) : NULL; \
+})
/* High-level interface for qobject_incref() */
#define QINCREF(obj) \
@@ -68,8 +74,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 +83,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 +108,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 +119,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..b3b3d444d2 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 QObjectBase_ 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 */
--
2.17.0.rc1.36.gcedb63ea2f
Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
Markus Armbruster <armbru@redhat.com> writes:
> Marc-André Lureau <marcandre.lureau@redhat.com> 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.
Uh, there's another reason: existing type casts from QObject * to
subtypes. I just spotted one in tests/check-qdict.c:
dst = (QDict *)qdict_crumple(src, &error_abort);
There may well be more.
>> 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 <marcandre.lureau@redhat.com>
On Mon, Apr 16, 2018 at 10:31 AM, Markus Armbruster <armbru@redhat.com> wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Marc-André Lureau <marcandre.lureau@redhat.com> 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. > > Uh, there's another reason: existing type casts from QObject * to > subtypes. I just spotted one in tests/check-qdict.c: > > dst = (QDict *)qdict_crumple(src, &error_abort); > > There may well be more. > So we better have checks from patch 1, don't we?
Hi On Mon, Apr 16, 2018 at 10:12 AM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> 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). ok > > 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. what is missing from patch 1? > >> 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. > ok
© 2016 - 2026 Red Hat, Inc.