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

On Wed, Apr 06, 2011 at 02:19:57PM -0600, Eric Blake wrote: > 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, Oops, there are always things escape from under your nose:( > 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. Why the linker doesn't complain about this this time? > > > 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 Yes. > compilers. Meanwhile, won't the gcc builtin just work for mingw (that Not sure about this. I don't have a mingw environment to test, but I trust gcc and guess it does. > 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 Agreed, this is a better way. > > > +++ 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) OK. > > I'm debating whether they should also be marked ATTRIBUTE_RETURN_CHECK No, there are cases you just want to inc/dec a value but do not check the modified value. > > > +++ 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(). OK. > > > +{ > > + 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)? Agreed. And it is dangerous to initialize an object more than once because the ref count may already changed and a second initializaton just do the wrong thing. > > > + > > + 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) This means bad things happened, and it is not enough to just give a warning. Think about this: two threads visit an object whose memory is corrupted that it's ref count becomes 0 but it doesn't get freed. thread 1: thread 2: ref the object (ref count becomes 1) unref the object (ref count becomes 0, object being freed) do something with the object, but it is already freed by thread 2 We should catch abnormal ref count by some way(if not sa_assert) > 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, Exactly the way(you have already saw my other patches when I was typing this:)) > although that then it requires a third parameter for virObjectInit? > Maybe it's worth some documentation on intended use in this header (am I OK. > 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. No. It is meaningless to return the current reference count, because it may already changed by others after the caller reads the returned value on which it is not safe to make some choice.