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.
> +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