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

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.