Message ID | 20180419150145.24795-2-marcandre.lureau@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 04/19/2018 10:01 AM, Marc-André Lureau wrote: > All QObject types have the base QObject as their first field. This > allows the simplification of qobject_to(). > > 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) > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> My R-b stands that this is correct from a coding point of view. But if I read Markus' review correctly, we could omit this patch, fix the one broken client in tests/check-qdict.c to use qobject_to() (why didn't you fix that in v6)?, and then just apply patches 2-5 without this patch, with no change in behavior and where we are no longer dependent on using offset 0 (even though all current instances do). So, I'll leave that to maintainer discretion.
On Thu, Apr 19, 2018 at 5:20 PM, Eric Blake <eblake@redhat.com> wrote: > On 04/19/2018 10:01 AM, Marc-André Lureau wrote: >> All QObject types have the base QObject as their first field. This >> allows the simplification of qobject_to(). >> >> 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) >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > My R-b stands that this is correct from a coding point of view. But if > I read Markus' review correctly, we could omit this patch, fix the one > broken client in tests/check-qdict.c to use qobject_to() (why didn't you > fix that in v6)?, and then just apply patches 2-5 without this patch, > with no change in behavior and where we are no longer dependent on using > offset 0 (even though all current instances do). So, I'll leave that to > maintainer discretion. > I don't think we have a good reason to allow offset different than 0. The fact that we have code that rely on that behaviour already is a sign that this could easily happen again, because it's the common pattern in C for inheritance, and static casting is allowed, for better or worse.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > On Thu, Apr 19, 2018 at 5:20 PM, Eric Blake <eblake@redhat.com> wrote: >> On 04/19/2018 10:01 AM, Marc-André Lureau wrote: >>> All QObject types have the base QObject as their first field. This >>> allows the simplification of qobject_to(). >>> >>> 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) >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> My R-b stands that this is correct from a coding point of view. But if >> I read Markus' review correctly, we could omit this patch, fix the one >> broken client in tests/check-qdict.c to use qobject_to() (why didn't you >> fix that in v6)?, and then just apply patches 2-5 without this patch, Is that safe? >> with no change in behavior and where we are no longer dependent on using >> offset 0 (even though all current instances do). So, I'll leave that to >> maintainer discretion. > > I don't think we have a good reason to allow offset different than 0. > The fact that we have code that rely on that behaviour already is a > sign that this could easily happen again, Maybe. We found one sloppy type cast, which should be cleaned up no matter what we do with this patch (happy to post the obvious patch myself). > because it's the common > pattern in C for inheritance, and static casting is allowed, for > better or worse. "Common way to do single inheritance in C" is a stronger argument. More so since QOM does it that way. We define hundreds of QOM types without ever bothering to check the super type comes first. We don't even bother to clearly document it has to come first. The fact that we nevertheless haven't seen misuse looks like fairly strong evidence of a non-problem to me.
On 24 April 2018 at 13:18, Markus Armbruster <armbru@redhat.com> wrote: > We define hundreds of QOM types without ever bothering to check the > super type comes first. We don't even bother to clearly document it has > to come first. If you don't put the super type first then the first time you do MyParent *p = MY_PARENT(me); then the QOM cast will assert on you, so you'll figure it out pretty quickly... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 24 April 2018 at 13:18, Markus Armbruster <armbru@redhat.com> wrote: >> We define hundreds of QOM types without ever bothering to check the >> super type comes first. We don't even bother to clearly document it has >> to come first. > > If you don't put the super type first then the first time you > do MyParent *p = MY_PARENT(me); then the QOM cast will assert > on you, so you'll figure it out pretty quickly... Same for QObject & friends, where qobject_to() asserts. Marc-André's argument for adding more checks was the possibility of someone doing MyParent *p = (MyParent *)me without running into MY_PARENT(me).
Markus Armbruster <armbru@redhat.com> writes: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > >> On Thu, Apr 19, 2018 at 5:20 PM, Eric Blake <eblake@redhat.com> wrote: >>> On 04/19/2018 10:01 AM, Marc-André Lureau wrote: >>>> All QObject types have the base QObject as their first field. This >>>> allows the simplification of qobject_to(). >>>> >>>> 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) >>>> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>> My R-b stands that this is correct from a coding point of view. But if >>> I read Markus' review correctly, we could omit this patch, fix the one >>> broken client in tests/check-qdict.c to use qobject_to() (why didn't you >>> fix that in v6)?, and then just apply patches 2-5 without this patch, > > Is that safe? Yes, with a tweak to PATCH 2. However, requiring base to come first is totally fine. There's really no reason to put it anywhere else, and fuzzing around with container_of() is just complicating things. Sunk cost (and thus not worth changing) until this series, where keeping it would complicate the next patch a bit, justifying this one. I wouldn't have bothered with QEMU_BUILD_BUG(), let alone QEMU_BUILD_BUG_MSG(); experience with QOM strongly indicates this mistake is vanishingly unlikely in practice. But I also can't be bothered to rip it out. [...]
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > All QObject types have the base QObject as their first field. This > allows the simplification of qobject_to(). > > 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) In my opinion, type casts between QObject * and subtypes are plain wrong. I intend to apply my "[PATCH] qobject: Use qobject_to() instead of type cast" first, and drop the paragraph, if you don't mind. > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > include/qapi/qmp/qobject.h | 5 ++--- > qobject/qobject.c | 9 +++++++++ > 2 files changed, 11 insertions(+), 3 deletions(-) > > 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 */ As mentioned elsewhere in this thread, the coding errors this assertion can catch are vanishingly unlikely. I could flip a coin to decide whether to keep it. Instead I'm asking you :) With the commit message rephrased not to give anyone ideas about type casts, and with or without the assertion: Reviewed-by: Markus Armbruster <armbru@redhat.com>
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 */