On Mon, Apr 16, 2018 at 06:27:45PM +0200, Michal Privoznik wrote:
> 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.
Sigh, poor choice of words, you're right, I meant the scenario where you put it
somewhere else...
>
> >
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> >> ---
> >> 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:
Well, this was to accommodate the macro to the original form of having it
behave function-like. But as I said, if we adjust 7/9, we have other options as
well and frankly, I like the usage of verify below, which I didn't know gnulib
had.
>
> #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)
I did and it works. With the above change:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
