[libvirt] [PATCH v2 2/2] Add virObject and virAtomic.

[Eric Blake]

On 04/20/2011 03:15 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 |   11 ++++++
>  src/util/viratomic.c     |   80 ++++++++++++++++++++++++++++++++++++++++++
>  src/util/viratomic.h     |   32 +++++++++++++++++
>  src/util/virobject.c     |   64 ++++++++++++++++++++++++++++++++++
>  src/util/virobject.h     |   86 ++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 275 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
> @@ -958,6 +958,17 @@ virUUIDGenerate;
>  virUUIDParse;
>  
>  
> +# virobject.h
> +virObjectInit;
> +virObjectRef;
> +virObjectUnref;
> +
> +
> +# viratomic.h
> +virAtomicInc;
> +virAtomicDec;

Swap these two sections (chunks are alphabetic by header), as well as
swap these two lines (within a header, lines are alphabetic).

> +++ b/src/util/viratomic.c

> +#else
> +
> +#include "threads.h"
> +
> +static virMutex virAtomicLock = VIR_MUTEX_INITIALIZER;

Hmm, if I implement virOnce first, maybe we should use that instead.

> +
> +long virAtomicInc(long *value)
> +{
> +    long result;
> +    virMutexLock(&virAtomicLock);
> +    *value += 1;
> +    result = *value;
> +    virMutexUnlock(&virAtomicLock);

Clunky, but does the job ;)

> +#include <stdlib.h>
> +
> +#include "viratomic.h"
> +#include "virobject.h"
> +#include "logging.h"
> +
> +int virObjectInit(virObjectPtr obj, virObjectFree f)
> +{
> +    if (obj->free) {
> +        /* This means the obj is already initialized. A second
> +         * initialization will destroy ref field, which is bad.
> +         */
> +        VIR_ERROR("Object %p is already initialized.", obj);
> +        return -1;
> +    }
> +
> +    if (!f) {
> +        VIR_ERROR0("virObjectFree method is required.");
> +	abort();

Either both failures should abort(), or both should return -1.  They are
both programming errors, so I'm personally okay with abort(), but this
would be the first use of abort() outside gnulib files, so I'll see if
either Dan has opinions as well.  In fact, if we go with abort() in both
places, then this can return void instead of int.  Likewise, by adding
ATTRIBUTE_NONNULL(2) to the prototype, then the compiler (well, at least
clang) will help catch coding errors of failing to pass a callback function.

Oops - you used TAB instead of space; 'make syntax-check' should have
caught that.

> +
> +void virObjectRef(virObjectPtr obj)
> +{
> +    if (virAtomicInc(&obj->ref) < 2)
> +        abort(); /* BUG if we get here */

If we keep the abort, then we _need_ the VIR_ERROR before hand saying
why we quit.

> +}
> +
> +void virObjectUnref(virObjectPtr obj)
> +{
> +    long ref = virAtomicDec(&obj->ref);
> +    if (ref == 0)
> +        obj->free(obj);
> +    else if (ref < 0)
> +        abort(); /* BUG if we get here */

Again, don't ever abort() without VIR_ERROR stating why.

And maybe we should think about installing a SIGABRT handler so that
aborts print a nicer message for the end user?

> +
> +typedef struct _virObject virObject;
> +typedef virObject *virObjectPtr;
> +
> +typedef void (*virObjectFree)(virObjectPtr obj);

I dislike extra casts, and this signature forces every client to have an
explicit cast in their free function.  Could we instead make this:

typedef void (*virObjectFree)(void *obj);

with some additional fallout documented below?

Also, can we attach ATTRIBUTE_NONNULL(1) to a typedef?  (I'm not quite
sure how that works with gcc).

> +
> +/* A thread owns a virObject by incrementing its reference-count by 1.
> + * If a thread owns a virObject, the virObject is guarenteed not be

s/guarenteed/guaranteed to/

> + * freed until the thread releases ownership by decrementing its
> + * reference-count by 1. A thread can't access a virObject after it
> + * releases the ownership of virObject because it can be freed at
> + * anytime.
> + *
> + * A thread can own a virObject legally in these ways:
> + *
> + * - a thread owns a virObject that it creates.
> + * - a thread owns a virObject if another thread passes the ownership
> + *   to it. Example: qemuMonitorOpen
> + * - a thread gets a virObject from a container.
> + *   Example: virDomainFindByUUID
> + * - a container owns a virObject by incrementing its reference-count
> + *   by 1 before adding it to the container
> + * - if a virObject is removed from a container its reference-count
> + *   must be decremented by 1
> + */
> +/*
> + * Usage of virObject:
> + *
> + * 1. embed virObject into your struct as the first member.
> + *
> + *    struct foo {
> + *        virObject obj;    // must be the first member
> + *        ...
> + *    }
> + *
> + * 2. call virObjectInit(obj, fooFree) in your struct's initialization
> + *    function. fooFree is the free method which be called automatically
> + *    when the ref count becomes 0.

The argument to fooFree will be &obj, which in turn can be cast as
struct foo* because obj is the first member.

struct foo * fooCreate()
{
    struct foo *myfoo;
    if (VIR_ALLOC(myfoo) < 0) {
        virReportOOMError();
        return NULL;
    }
    virObjectInit(&myfoo->obj, fooFree);
    ... // all other contents of myfoo
    return myfoo;
}

> + *
> + * 3. implement two functions to ref/unref your struct like this:
> + *
> + *    void FooRef(struct foo *f)
> + *    {
> + *        virObjectRef(f->obj);
> + *    }
> + *
> + *    void FooUnref(struct foo *f)
> + *    {
> + *        virObjectUnref(f->obj);
> + *    }

4. implement the free method for your struct like this:
static void fooFree(void *obj)
{
    struct foo *myfoo = obj;
    ... // all other contents of myfoo
    VIR_FREE(myfoo);
}

> + */
> +struct _virObject {
> +    long ref;
> +    virObjectFree free;
> +};
> +
> +int virObjectInit(virObjectPtr obj, virObjectFree f);

ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)

> +void virObjectRef(virObjectPtr obj);

ATTRIBUTE_NONNULL(1)

> +void virObjectUnref(virObjectPtr obj);

ATTRIBUTE_NONNULL(1)

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110420/b97c1f96/attachment-0001.sig>