Re: [Qemu-devel] [PATCH v6 1/5] qobject: ensure base is at offset 0

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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 <address@hidden>