Re: [libvirt] [PATCH 1/6] Add virObject and virAtomic.

6 Apr 2011

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@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    