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

[Daniel P. Berrange]

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

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

We should raise an error here

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

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

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

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|