[libvirt] [PATCH 8/9] virobject: Check if @parent is the first member in class

[Erik Skultety]

On Fri, Apr 13, 2018 at 04:47:15PM +0200, Michal Privoznik wrote:
> Our virObject code relies heavily on the fact that the first
> member of the class struct is type of virObject (or some
> derivation of if). Let's check for that.

If a class is missing 'parent' memeber, it's a bug in the definition of the
struct/class, therefore there should be a static assertion rather than a
runtime check.

>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virobject.c | 31 +++++++++++++++++++++----------
>  src/util/virobject.h |  5 ++++-
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index c5a98d21cc..e184f5349e 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -77,6 +77,7 @@ virObjectOnceInit(void)
>  {
>      if (!(virObjectClass = virClassNew(NULL,
>                                         "virObject",
> +                                       0,
>                                         sizeof(virObject),
>                                         NULL)))

Also, I don't like this extra parameter, which really shouldn't be needed; you
created a macro which hides this parameter, but that doesn't mean that
design-wise it makes sense to have it there, think of it as a constructor, you
don't pass a constructor an offset of the class' member, because it shouldn't
have need for it, but you do, solely for the purpose of checking whether we have
a particular member in place.
So, to start a discussion about this (I also think Dan posted something related
to this recently, but I don't seem to be able to find it in the archives - do I
even archive?!!!), I came up with my first compile-time hack ever, it seems to
work like expected, but I'd like to hear your opinions both the macro itself
and the approach we're going to take, so here's my replacement patch:

diff --git a/src/util/virobject.h b/src/util/virobject.h
index 92dd51239..2a973d401 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -75,8 +75,12 @@ virClassPtr virClassForObjectRWLockable(void);
 #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
 # endif

+# define VIR_CLASS_HAS_PARENT(name) \
+        !sizeof(char[0-offsetof(name, parent)])
+
 # define VIR_CLASS_NEW(prnt, name) \
-    virClassNew(prnt, #name, sizeof(name), name##Dispose)
+    VIR_CLASS_HAS_PARENT(name) ? \
+        virClassNew(prnt, #name, sizeof(name), name##Dispose) : NULL

Notes:
 - I suppose mingw would handle this hack the same way it handles
   VIR_TYPEMATCH, IOW it works...

 - it also doesn't need to be a ternary, I suggested extending VIR_CLASS_NEW to
   do the complete assignment in [Patch 7/9], like this:

# define VIR_CLASS_NEW(prnt, name) \
    if (!(name##Class) = virClassNew(prnt, #name, sizeof(name), name##Dispose))
        return -1;

Regards,
Erik