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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list