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

[Daniel P. Berrangé]

On Mon, Apr 16, 2018 at 02:30:40PM +0200, Erik Skultety wrote:
> 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.

Agreed, my suggestion was for a static assert too.

> 
> >
> > 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:

https://www.redhat.com/archives/libvir-list/2018-April/msg00984.html

I had suggested something in the virObjectNew function:

  #define virObjectNew(typ) \
      (typ *)(&((typ *)virObjectNewImpl(typ # Class)).parent)

catching it with virClassNew works fine too, as it would be a compile
time check too

> 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

So we're relying on the fact the the ': NULL" will never execute
because VIR_CLASS_HAS_PARENT will trigger a compile time error.

> 
> 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;

This has the added benefit of enforcing class variable naming scheme
which removes another source of developer error, and is in keeping
with VIR_ALLOC() style.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|