[v6,1/5] qobject: ensure base is at offset 0

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>
---
 include/qapi/qmp/qobject.h | 5 ++---
 qobject/qobject.c          | 9 +++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

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>