Re: [libvirt] [PATCH 04/13] Add a generic reference counted virObject type
On Fri, Jul 13, 2012 at 04:38:26PM -0600, Eric Blake wrote:
On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange(a)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 connklass = virClassNew("virConnect",
> sizeof(virConnect),
> virConnectDispose);
I know why you use 'klass' instead of 'class' in isolation (for C++
compatibility), but when it is part of a longer word, it just looks weird.
> 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)
Should these return the resulting refcount, and/or add a query function
that tells the current ref count? There are some APIs where knowing how
many references remain may be useful, rather than just the boolean
information of being the last reference.
My intent was to design an API that encourages/forces safe usage. Since
the ref count accessors do not require any kind of locking, code that
has any logic operating on the *current* ref count is quite likely going
to be broken, as the current ref count can change at any moment. So, IMHO,
when incrementing/decrementing the ref count, the only safe bit of info
is whether you just released the last reference, or whether you just
incremented the first reference. This leads me to believe that virObjectRef
should be void and virObjectDec should be boolean.
> +
> +struct _virClass {
> + unsigned int magic;
> + const char *name;
> + size_t objectSize;
> +
> + virObjectDisposeCallback dispose;
> +};
Meta-question - do we want to free _virClass objects on clean exit, to
keep valgrind analysis happy? Or are we okay with permanently
allocating them and leaking those references on exit? If the former,
then should _virClass be reference counted (that is, the _virClass
struct _also_ inherits from virObject, and each call to virObjectNew()
increments the class reference count, and each dispose decrements the
class reference count). Then again, it would make for an interesting
chicken-and-egg issue for setting up the _virClass class description for
each instance of a _virClass object.
If we wanted to free classes, its probably better just to directly
do ref counting in the virClass struct, and not try to make it
inherit virObject.
> +void *virObjectNew(virClassPtr klass)
> +{
> + virObjectPtr obj = NULL;
> + char *somebytes;
> +
> + if (VIR_ALLOC_N(somebytes, klass->objectSize) < 0) {
> + virReportOOMError();
> + return NULL;
> + }
> + obj = (void*)somebytes;
This cast works, but I would have written:
obj = (virObjectPtr) somebytes;
Good point.
> +
> +virClassPtr virClassNew(const char *name,
> + size_t objectSize,
> + virObjectDisposeCallback dispose)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
Must the dispose callback be present, even if it has nothing to do?
Hmm. I guess we could allow it to be NULL really.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|