| Message ID | 20180417133602.23832-2-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/17/2018 08:35 AM, Marc-André Lureau wrote: > All QObject types have the base QObject as first field. This allows to s/as/as their/ > simplify qobject_to() and will allow further simplification in s/allows to simplify/allows the simplification of/ s/in/in the/ > following patch. Might also be worth mentioning that this explicitly guarantees that existing casts work correctly (even though we'd prefer to get rid of such casts in any location except the qobject.h macros); Markus pointed out: >> 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); > +++ b/qobject/qobject.c > @@ -16,6 +16,15 @@ > #include "qapi/qmp/qlist.h" > #include "qapi/qmp/qstring.h" > > +QEMU_BUILD_BUG_MSG( > + offsetof(QNull, base) != 0 || Is the ' != 0' portion necessary? Style-wise, I'd have avoided it, but HACKING doesn't forbid it, so I'm fine if you leave it. With the commit message further improved, Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 04/17/2018 08:35 AM, Marc-André Lureau wrote: >> All QObject types have the base QObject as first field. This allows to > > s/as/as their/ > >> simplify qobject_to() and will allow further simplification in > > s/allows to simplify/allows the simplification of/ > s/in/in the/ > >> following patch. > > Might also be worth mentioning that this explicitly guarantees that > existing casts work correctly (even though we'd prefer to get rid of > such casts in any location except the qobject.h macros); Markus pointed out: > >>> 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); As far as I'm concerned, that's the real reason. The simplification plus the check to make it safe seems like a wash. The cast I spotted appears to be the only one, though: $ git-grep '(Q[A-Z][a-z]* \*)' hmp.c: qmp_device_add((QDict *)qdict, NULL, &err); include/qapi/qmp/qobject.h: return (QObject *)obj; qobject/qobject.c:static void (*qdestroy[QTYPE__MAX])(QObject *) = { tests/check-qdict.c: dst = (QDict *)qdict_crumple(src, &error_abort); The first two cast away const, the third isn't a type cast. The fourth one should use qobject_to() instead, regardless of this patch. Do we want to force base to come first anyway? Where does PATCH 2 exploit "base first"? [...]
Hi On Wed, Apr 18, 2018 at 6:45 PM, Markus Armbruster <armbru@redhat.com> wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 04/17/2018 08:35 AM, Marc-André Lureau wrote: >>> All QObject types have the base QObject as first field. This allows to >> >> s/as/as their/ >> >>> simplify qobject_to() and will allow further simplification in >> >> s/allows to simplify/allows the simplification of/ >> s/in/in the/ >> >>> following patch. >> >> Might also be worth mentioning that this explicitly guarantees that >> existing casts work correctly (even though we'd prefer to get rid of >> such casts in any location except the qobject.h macros); Markus pointed out: >> >>>> 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); > > As far as I'm concerned, that's the real reason. The simplification > plus the check to make it safe seems like a wash. > > The cast I spotted appears to be the only one, though: > > $ git-grep '(Q[A-Z][a-z]* \*)' > hmp.c: qmp_device_add((QDict *)qdict, NULL, &err); > include/qapi/qmp/qobject.h: return (QObject *)obj; > qobject/qobject.c:static void (*qdestroy[QTYPE__MAX])(QObject *) = { > tests/check-qdict.c: dst = (QDict *)qdict_crumple(src, &error_abort); > > The first two cast away const, the third isn't a type cast. The fourth > one should use qobject_to() instead, regardless of this patch. > > Do we want to force base to come first anyway? to simplify qobject_to and allow static casting? > Where does PATCH 2 exploit "base first"? patch 2 no longer needs that afaict
On 04/18/2018 11:45 AM, Markus Armbruster wrote: >> Might also be worth mentioning that this explicitly guarantees that >> existing casts work correctly (even though we'd prefer to get rid of >> such casts in any location except the qobject.h macros); Markus pointed out: >> >>>> 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); > > As far as I'm concerned, that's the real reason. The simplification > plus the check to make it safe seems like a wash. > > The cast I spotted appears to be the only one, though: > > $ git-grep '(Q[A-Z][a-z]* \*)' > hmp.c: qmp_device_add((QDict *)qdict, NULL, &err); > include/qapi/qmp/qobject.h: return (QObject *)obj; > qobject/qobject.c:static void (*qdestroy[QTYPE__MAX])(QObject *) = { > tests/check-qdict.c: dst = (QDict *)qdict_crumple(src, &error_abort); > > The first two cast away const, the third isn't a type cast. The fourth > one should use qobject_to() instead, regardless of this patch. > > Do we want to force base to come first anyway? > > Where does PATCH 2 exploit "base first"? It doesn't, but PATCH 4 does: > /** > * qobject_ref(): Increment QObject's reference count > + * > + * Returns: the same @obj. The type of @obj will be propagated to the > + * return type. > */ > -#define qobject_ref(obj) qobject_ref_impl(QOBJECT(obj)) > +#define qobject_ref(obj) ((typeof(obj)) qobject_ref_impl(QOBJECT(obj))) >
Eric Blake <eblake@redhat.com> writes: > On 04/18/2018 11:45 AM, Markus Armbruster wrote: > >>> Might also be worth mentioning that this explicitly guarantees that >>> existing casts work correctly (even though we'd prefer to get rid of >>> such casts in any location except the qobject.h macros); Markus pointed out: >>> >>>>> 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); >> >> As far as I'm concerned, that's the real reason. The simplification >> plus the check to make it safe seems like a wash. >> >> The cast I spotted appears to be the only one, though: >> >> $ git-grep '(Q[A-Z][a-z]* \*)' >> hmp.c: qmp_device_add((QDict *)qdict, NULL, &err); >> include/qapi/qmp/qobject.h: return (QObject *)obj; >> qobject/qobject.c:static void (*qdestroy[QTYPE__MAX])(QObject *) = { >> tests/check-qdict.c: dst = (QDict *)qdict_crumple(src, &error_abort); >> >> The first two cast away const, the third isn't a type cast. The fourth >> one should use qobject_to() instead, regardless of this patch. >> >> Do we want to force base to come first anyway? >> >> Where does PATCH 2 exploit "base first"? > > It doesn't, but PATCH 4 does: > >> /** >> * qobject_ref(): Increment QObject's reference count >> + * >> + * Returns: the same @obj. The type of @obj will be propagated to the >> + * return type. >> */ >> -#define qobject_ref(obj) qobject_ref_impl(QOBJECT(obj)) >> +#define qobject_ref(obj) ((typeof(obj)) qobject_ref_impl(QOBJECT(obj))) Easy enough to fix: #define qobject_ref(obj) ({ \ typeof(obj) _obj = obj; \ qobject_ref_impl(QOBJECT(_obj)); \ _obj; \ }) Look ma, no type casts!
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h index e022707578..5206ff9ee1 100644 --- a/include/qapi/qmp/qobject.h +++ b/include/qapi/qmp/qobject.h @@ -61,9 +61,8 @@ struct QObject { QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7, "The QTYPE_CAST_TO_* list needs to be extended"); -#define qobject_to(type, obj) ({ \ - QObject *_tmp = qobject_check_type(obj, glue(QTYPE_CAST_TO_, type)); \ - _tmp ? container_of(_tmp, type, base) : (type *)NULL; }) +#define qobject_to(type, obj) \ + ((type *)qobject_check_type(obj, glue(QTYPE_CAST_TO_, type))) /* Initialize an object to default values */ static inline void qobject_init(QObject *obj, QType type) diff --git a/qobject/qobject.c b/qobject/qobject.c index 23600aa1c1..87649c5be5 100644 --- a/qobject/qobject.c +++ b/qobject/qobject.c @@ -16,6 +16,15 @@ #include "qapi/qmp/qlist.h" #include "qapi/qmp/qstring.h" +QEMU_BUILD_BUG_MSG( + offsetof(QNull, base) != 0 || + offsetof(QNum, base) != 0 || + offsetof(QString, base) != 0 || + offsetof(QDict, base) != 0 || + offsetof(QList, base) != 0 || + offsetof(QBool, base) != 0, + "base qobject must be at offset 0"); + static void (*qdestroy[QTYPE__MAX])(QObject *) = { [QTYPE_NONE] = NULL, /* No such object exists */ [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */
All QObject types have the base QObject as first field. This allows to simplify qobject_to() and will allow further simplification in following patch. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- include/qapi/qmp/qobject.h | 5 ++--- qobject/qobject.c | 9 +++++++++ 2 files changed, 11 insertions(+), 3 deletions(-)