[libvirt] [RFC PATCH 1/2] Add virObject.

[Hu Tao]

On Fri, Mar 18, 2011 at 11:02:25AM +0000, Daniel P. Berrange wrote:
> On Wed, Mar 16, 2011 at 06:29:52PM +0800, Hu Tao wrote:
> > virObject is the base struct that manages reference-counting
> > for all structs that need the ability of reference-counting.
> > ---
> >  src/Makefile.am          |    1 +
> >  src/libvirt_private.syms |    5 ++++
> >  src/util/object.c        |   55 ++++++++++++++++++++++++++++++++++++++++++++++
> >  src/util/object.h        |   39 ++++++++++++++++++++++++++++++++
> >  4 files changed, 100 insertions(+), 0 deletions(-)
> >  create mode 100644 src/util/object.c
> >  create mode 100644 src/util/object.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 645119e..3ebabe9 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -81,6 +81,7 @@ UTIL_SOURCES =							\
> >  		util/util.c util/util.h				\
> >  		util/xml.c util/xml.h				\
> >  		util/virtaudit.c util/virtaudit.h               \
> > +		util/object.c util/object.h			\
> 
> Can you rename this to virobject.c / virobject.h.  NB without

OK.

> the 't'.  I want rename the virtaudit/virterror stuff later.
> Ideally all new files in util/ should have a 'vir' prefix,
> since our current names are so short & generic they can easily
> clash and / or cause confusion.  eg, the name of the file should
> match the name of the APIs inside, so virObjectPtr -> virobject.h
> 
> > diff --git a/src/util/object.c b/src/util/object.c
> > new file mode 100644
> > index 0000000..31ea27b
> > --- /dev/null
> > +++ b/src/util/object.c
> > +#include <assert.h>
> 
> NB, our coding guidelines don't allow use of assert().
> 
> You can use  sa_assert() which is a no-op just designed to
> be detectable by static analysis tools.

OK.

> 
> > +#include "object.h"
> > +
> > +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj))
> > +{
> > +    if (!free)
> > +        return -1;
> 
> We should raise an error here

OK.

> 
> > +
> > +    obj->ref = 1;
> > +    obj->free = free;
> > +
> > +    return 0;
> > +}
> > +
> > +#define virAtomicInc(value) \
> > +    __sync_add_and_fetch(&value, 1)
> > +#define virAtomicDec(value) \
> > +    __sync_sub_and_fetch(&value, 1)
> 
> I reckon it would be a good idea to put these into a separate
> file  'src/util/virtatomic.{h,c}'.

Agree. Will do it.  Is the `virt' prefix a typo? Since you've
suggested `vir' prefix for files in util/.

> 
> Also, I think we need to make this more portable, since at least
> on Windows, I don't think we want to assume use of GCC. I think
> we should at the very least have an impl for GCC builtins, and
> an impl for Win32 to start with. If someone wants to do further
> impls later they can... If you look at glib's gatomic.c you'll
> see example code for Win32 atomic ops which is LGPLv2+ licensed

Yes, atomic ops should be portable, not limited just on gcc. Can we
borrow glib's win32 atomic ops? Will it cause any license conflict?
Anyway I will have a look at glib's win32 atomic implementation.
Thanks for the information.

> 
> > +void virObjectRef(virObjectPtr obj)
> > +{
> > +    assert(obj->ref > 0);
> > +    virAtomicInc(obj->ref);
> > +}
> > +
> > +void virObjectUnref(virObjectPtr obj)
> > +{
> > +    assert(obj->ref > 0);
> > +    if (virAtomicDec(obj->ref) == 0)
> > +        obj->free(obj);
> > +}
> 
> Aside from assert() not being allowed, this usage surely isn't
> safe because it doesn't do an atomic read on 'obj->ref' when
> comparing it.

Right, will update it.


-- 
Thanks,
Hu Tao