[libvirt] [PATCH 03/13] Add a generic reference counted virObject type

[Eric Blake]

On 07/31/2012 10:58 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> This introduces a fairly basic reference counted virObject type
> and an associated virClass type, that use atomic operations for
> ref counting.
> 
> In a global initializer (recommended to be invoked using the
> virOnceInit API), a virClass type must be allocated for each
> object type. This requires a class name, a "dispose" callback
> which will be invoked to free memory associated with the object's
> fields, and the size in bytes of the object struct.
> 
> eg,
> 
>    virClassPtr  connclass = virClassNew("virConnect",
>                                         sizeof(virConnect),
>                                         virConnectDispose);

Should we macroize any of this common pattern, as in:

#define VIR_CLASS_NEW(name) \
  virClassNew(#name, sizeof(name), name##Dispose)

virClassPtr connclass = VIR_CLASS_NEW(virConnect);

then again, you allow for a NULL dispose function, so duplicating the
common idiom isn't too bad if we don't want to force a dispose function
to exist.

> 
> The struct for the object, must include 'virObject' as its
> first member
> 
> eg
> 
>   struct _virConnect {
>     virObject object;
> 
>     virURIPtr uri;
>   };
> 
> The 'dispose' callback is only responsible for freeing
> fields in the object, not the object itself. eg a suitable
> impl for the above struct would be
> 
>   void virConnectDispose(void *obj) {
>      virConnectPtr conn = obj;
>      virURIFree(conn->uri);
>   }
> 
> There is no need to reset fields to 'NULL' or '0' in the
> dispose callback, since the entire object will be memset
> to 0, and the klass pointer & magic integer fields will
> be poisoned with 0xDEADBEEF

Should you mention that the object is then freed after being poisoned?
That is, the memset is merely precautionary to make use-after-free bugs
easier to detect.

> 
> When creating an instance of an object, one needs simply
> pass the virClassPtr eg
> 
>    virConnectPtr conn = virObjectNew(connklass);

s/connklass/connclass/ to match above

>    if (!conn)
>       return NULL;
>    conn->uri = virURIParse("foo:///bar")
> 
> Object references can be manipulated with
> 
>    virObjectRef(conn)
>    virObjectUnref(conn)
> 
> The latter returns a true value, if the object has been
> freed (ie its ref count hit zero)
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---

> +++ b/src/Makefile.am
> @@ -84,6 +84,7 @@ UTIL_SOURCES =							\
>  		util/virauth.c util/virauth.h			\
>  		util/virauthconfig.c util/virauthconfig.h	\
>  		util/virfile.c util/virfile.h			\
> +		util/virobject.c util/virobject.h		\
>  		util/virnodesuspend.c util/virnodesuspend.h	\

Sorting is off by one line.

> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +#define virObjectError(code, ...)                                      \
> +    virReportErrorHelper(VIR_FROM_THIS, code, __FILE__,                \
> +                         __FUNCTION__, __LINE__, __VA_ARGS__)

This is stale now.


> +virClassPtr virClassNew(const char *name,
> +                        size_t objectSize,
> +                        virObjectDisposeCallback dispose)
> +{
> +    virClassPtr klass;
> +
> +    if (VIR_ALLOC(klass) < 0)
> +        return NULL;

Missing a virReportOOMError(), especially since:

> +
> +    if (!(klass->name = strdup(name)))
> +        goto no_memory;

the rest of your code has it.

> +    klass->magic = virAtomicIntAdd(&magicCounter, 1);

virAtomicIntInc(), now that it returns a value?

> +/**
> + * virObjectUnref:
> + * @anyobj: any instance of virObjectPtr
> + *
> + * Decrement the reference count on @anyobj and if
> + * it hits zero, runs the "dispose" callback associated
> + * with the object class.

and then free anyobj.

> +/**
> + * virObjectIsClass:
> + * @anyobj: any instance of virObjectPtr
> + * @klass: the class to check
> + *
> + * Checks whether @anyobj is an instance of
> + * @klass
> + *
> + * Returns true if @anyobj is an instance of @klass
> + */
> +bool virObjectIsClass(void *anyobj,
> +                      virClassPtr klass)
> +{
> +    virObjectPtr obj = anyobj;
> +    return obj != NULL && (obj->magic == klass->magic) && (obj->klass == klass);

Theoretically redundant, but I'm not opposed to the extra checking here
as one more line of defense at quickly debugging bad code with
use-after-free problems.

> +++ b/src/util/virobject.h
> @@ -0,0 +1,60 @@
> +/*

> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA

Fails 'make syntax-check'.


> +struct _virObject {
> +    unsigned int magic;
> +    virClassPtr klass;
> +    int refs;
> +};

On a machine with 64-bit void*, this is 24 bytes, but you could make it
16 bytes by swapping refs to occur before klass.  Not that we're
strapped for space, but there are some speed benefits to smaller structs.

> +bool virObjectUnref(void *obj)
> +    ATTRIBUTE_NONNULL(1);

This attribute is wrong, since you implemented the function to handle a
NULL input.

> +void *virObjectRef(void *obj)
> +    ATTRIBUTE_NONNULL(1);

Likewise.

> +
> +bool virObjectIsClass(void *obj,
> +                      virClassPtr klass)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Likewise for the first argument (but the 2nd is indeed non-NULL).

Overall I like the direction this is headed.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
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/20120803/d6459802/attachment-0001.sig>