3:19 p.m.
On 04/06/2011 01:19 AM, Hu Tao wrote:
virObject is the base struct that manages reference-counting
for all structs that need the ability of reference-counting.
virAtomic provides atomic operations which are thread-safe.
---
src/Makefile.am | 2 +
src/libvirt_private.syms | 5 ++++
src/util/viratomic.c | 46 ++++++++++++++++++++++++++++++++++++++++
src/util/viratomic.h | 30 ++++++++++++++++++++++++++
src/util/virobject.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
src/util/virobject.h | 39 ++++++++++++++++++++++++++++++++++
6 files changed, 174 insertions(+), 0 deletions(-)
create mode 100644 src/util/viratomic.c
create mode 100644 src/util/viratomic.h
create mode 100644 src/util/virobject.c
create mode 100644 src/util/virobject.h
+++ b/src/libvirt_private.syms
@@ -993,3 +993,8 @@ virXPathUInt;
virXPathULong;
virXPathULongHex;
virXPathULongLong;
+
+# object.h
virobject.h, and float this section to appear before virtaudit.h (hmm,
maybe we should do a preliminary patch to rename that to viraudit.h, so
that we aren't mixing vir vs. virt in quite so many places).
+virObjectInit;
+virObjectRef;
+virObjectUnref;
Missing exports from viratomic.h.
diff --git a/src/util/viratomic.c b/src/util/viratomic.c
new file mode 100644
index 0000000..629f189
--- /dev/null
+++ b/src/util/viratomic.c
@@ -0,0 +1,46 @@
+
+#ifdef WIN32
+long virAtomicInc(long *value)
+{
+ return InterlockedIncrement(value);
+}
+
+long virAtomicDec(long *value)
+{
+ return InterlockedDecrement(value);
This is an OS-specific replacement.
+}
+#else /* WIN32 */
+long virAtomicInc(long *value)
+{
+ return __sync_add_and_fetch(value, 1);
This is a gcc builtin, and will fail to compile with other C99
compilers. Meanwhile, won't the gcc builtin just work for mingw (that
is, no need to use the OS-specific InterlockedIncrement if you have the
compiler builtin instead).
I think this file needs three implementations:
#if defined __GNUC__ || <detect Intel's compiler, which also has these>
use compiler builtins of __sync_add_and_fetch
#elif defined WIN32
use OS primitives, like InterlockedIncrement
#else
we're hosed when it comes to lightweight versions, but we can still
implement a heavyweight replacement that uses virMutex
#endif
+++ b/src/util/viratomic.h
@@ -0,0 +1,30 @@
+#ifndef __VIR_ATOMIC_H
+#define __VIR_ATOMIC_H
+
+long virAtomicInc(long *value);
+long virAtomicDec(long *value);
Mark both of these ATTRIBUTE_NONNULL(1)
I'm debating whether they should also be marked ATTRIBUTE_RETURN_CHECK
+++ b/src/util/virobject.c
@@ -0,0 +1,52 @@
+
+#include "viratomic.h"
+#include "virobject.h"
+#include "logging.h"
+
+int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj))
You should declare a typedef:
typedef void (*virObjectFreer)(virObjectPtr);
then it becomes simpler to read:
int virObjectInit(virObjectPtr obj, virObjectFreer f)
Use a different name (I used freer/f above) to avoid -Wshadow warnings
with free().
+{
+ if (!free) {
Especially since shadowing means it's impossible to tell if this
statement is always true (the address of free() exists) or conditional
(there is a local variable named free shadowing the global function),
and context-sensitive code reviews are tougher :)
+ VIR_ERROR0("method free is required.");
+ return -1;
+ }
Should this function also check and return -1 if obj->free was not NULL
on entry (that is, guarantee that you can't initialize an object twice)?
+
+ obj->ref = 1;
+ obj->free = free;
+
+ return 0;
+}
+
+void virObjectRef(virObjectPtr obj)
+{
+ sa_assert(obj->ref > 0);
This is useless. It only helps static analyzers (like clang),
+ virAtomicInc(&obj->ref);
but there's nothing to analyze that depends on knowing the value was
positive.
I'm debating whether to do checking. Maybe we should do:
if (virAtomicInc(&obj->ref) < 2)
VIR_WARN("invalid call to virObjectRef");
+void virObjectUnref(virObjectPtr obj)
+{
+ sa_assert(obj->ref > 0);
Again, I'm not sure that this buys anything. But we may want to do:
int ref = virAtomicDec(&obj->ref);
if (ref < 0)
VIR_WARN("invalid call to virObjectUnref");
else if (ref == 0)
obj->free(obj)
+ if (virAtomicDec(&obj->ref) == 0)
+ obj->free(obj);
+}
diff --git a/src/util/virobject.h b/src/util/virobject.h
new file mode 100644
index 0000000..cd7d3e8
--- /dev/null
+++ b/src/util/virobject.h
+
+typedef struct _virObject virObject;
+typedef virObject *virObjectPtr;
+
+struct _virObject {
+ long ref;
+ void (*free)(virObjectPtr obj);
Is virObjectPtr the right thing to use here? If we assume that all
clients will always have a virObjectPtr as their first member, then they
can cast that back into the larger object. Is void* opaque any better,
although that then it requires a third parameter for virObjectInit?
Maybe it's worth some documentation on intended use in this header (am I
getting this usage right? I haven't looked at how you used it later in
the series, but am just guessing):
struct _virFoo {
virObject obj; /* Must be first member */
...
};
static void virFooFree(virObjectPtr obj)
{
virFooPtr foo = obj;
...
}
virFooPtr virFooNew(void)
{
virFooPtr foo;
if (VIR_ALLOC(foo) < 0) {
virReportOOMError();
return NULL;
}
virObjectInit(&foo->obj, virFooFree);
...
return foo;
}
+};
+
+int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj));
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK
+void virObjectRef(virObjectPtr obj);
ATTRIBUTE_NONNULL(1)
Also, should this return the current reference count? Not all callers
will need it, but returning void is harsh if someone might be able to
use it.
+void virObjectUnref(virObjectPtr obj);
Likewise.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org