[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]