On Mon, Mar 21, 2011 at 04:03:23PM +0800, Hu Tao wrote:
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/.
Yes, I mean viratomic.h
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.
Glib's atomic ops code is LGPLv2+ licensed, so we will be fine borrowing the relevant bits of 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 :|