[libvirt] [PATCH] conf: Actually make virDomainChrSourceDef an object

[Daniel P. Berrangé]

On Thu, Apr 12, 2018 at 01:27:41PM +0200, Michal Privoznik wrote:
> On 04/12/2018 01:08 PM, John Ferlan wrote:
> > 
> > 
> > On 04/12/2018 04:44 AM, Erik Skultety wrote:
> >> On Thu, Apr 12, 2018 at 09:14:50AM +0200, Michal Privoznik wrote:
> >>> In 2ada9ef1465f we've tried to turn virDomainChrSourceDef into
> >>> virObject. Well, this requires 'virObject' member to be stored on
> >>> the first position of the struct. This adjustment is missing in
> >>> the original commit leading to all sorts of funny memleaks and
> >>> data corruptions.
> >>>
> >>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >>> ---
> >>>  src/conf/domain_conf.h | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> > 
> > Doh!  Thanks....  I guess not only do we need a way to detect that we're
> > using VIR_ALLOC on an object (as Eric pointed out in my original
> > patches), but we really should detect when we use virObjectNew without
> > the virObject parent. For some reason, I have a recollection of altering
> > changes during my meanderings through virObject code in order to point
> > out more directly some misuses, but it was rejected. Although I cannot
> > recall if this particular instance of not having virObject parent was
> > addressed...
> 
> I don't think it was addressed there. And honestly, I don't know how to
> check for this in some automated way. Compiler is not going to help -
> sure we can have very primitive check like sizeof(virSomeStruct) >=
> sizeof(virObject), but that will fail only for very small structs (which
> is not the case here). Also, writing script that would check for this is
> going to end up in a lot of pain IMO. The only thing I can think of is
> review.

I think we could do a compile time check for it with a little cpp black
magic and some naming conventions.

First we declare that the class name variable must always match the
name of the object struct, but with Class suffix.

Second we declare that the object struct must have a field called "parent"
in it.

Then we rename virObjectNew to virObjectNewImpl and add a macro:

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

IOW we're casting the void * return value to the object struct, then we're
referencing the parent field and casting it back to our object struct.

We would have to change all calls to pass the struct name instead of
class variable name.

This doesn't ensure that "parent" is the first field, but with a little
more thinking you can probably come up with a funkier macro to validate
that aspect too.

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