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.

Signed-off-by: Michal Privoznik <mprivozn@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)))
         return -1;
@@ -159,21 +160,31 @@ virClassForObjectRWLockable(void)
 virClassPtr
 virClassNew(virClassPtr parent,
             const char *name,
+            size_t off,
             size_t objectSize,
             virObjectDisposeCallback dispose)
 {
     virClassPtr klass;
 
-    if (parent == NULL &&
-        STRNEQ(name, "virObject")) {
-        virReportInvalidNonNullArg(parent);
-        return NULL;
-    } else if (parent &&
-               objectSize <= parent->objectSize) {
-        virReportInvalidArg(objectSize,
-                            _("object size %zu of %s is smaller than parent class %zu"),
-                            objectSize, name, parent->objectSize);
-        return NULL;
+    if (parent == NULL) {
+        if (STRNEQ(name, "virObject")) {
+            virReportInvalidNonNullArg(parent);
+            return NULL;
+        }
+    } else {
+        if (objectSize <= parent->objectSize) {
+            virReportInvalidArg(objectSize,
+                                _("object size %zu of %s is smaller than parent class %zu"),
+                                objectSize, name, parent->objectSize);
+            return NULL;
+        }
+
+        if (off) {
+            virReportInvalidArg(off,
+                                _("struct %s doesn't have 'parent' as its first member"),
+                                name);
+            return NULL;
+        }
     }
 
     if (VIR_ALLOC(klass) < 0)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 92dd512399..25db3a0c22 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -76,11 +76,14 @@ virClassPtr virClassForObjectRWLockable(void);
 # endif
 
 # define VIR_CLASS_NEW(prnt, name) \
-    virClassNew(prnt, #name, sizeof(name), name##Dispose)
+    virClassNew(prnt, #name, \
+                offsetof(name, parent), \
+                sizeof(name), name##Dispose)
 
 virClassPtr
 virClassNew(virClassPtr parent,
             const char *name,
+            size_t off,
             size_t objectSize,
             virObjectDisposeCallback dispose)
     VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(2);
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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)])

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)

We couldn't do that if VIR_CLASS_NEW() is still a function-like macro
( if (!(nameClass = VIR_CLASS_NEW(...))) return -1; ).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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)])
>
> 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@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list