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

On 04/16/2018 02:30 PM, 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. If a class is missing parent then you'd hit compile time error because offsetof() is trying to get offset of a non-existent member.

> >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> 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)]) I don't quite understand why this is so obfuscated. Anyway, since VIR_CLASS_NEW() is going to be a stand alone macro (like VIR_ENUM_DECL for instance) we can do plain:

#define VIR_CLASS_NEW(prt, name) \ verify(offsetof(name, parent) == 0); \ if (!(name##Class = virClassNew(prt, #name, sizeof(name), name##Dispose))) \ return -1; (written from the top of my head, not tested, not compiled, don't take it too much literally)