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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list