Re: [libvirt] [PATCH 2/7] Add a virObjectLockable class holding a mutex
On Fri, Jan 11, 2013 at 04:23:23PM -0700, Eric Blake wrote:
On 01/11/2013 05:13 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange(a)redhat.com>
>
> A great many virObject instances require a mutex, so introduce
> a convenient class for this which provides a mutex. This avoids
> repeating the tedious init/destroy code
> ---
> src/libvirt_private.syms | 4 +++
> src/util/virobject.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/util/virobject.h | 20 +++++++++++
> 3 files changed, 109 insertions(+), 2 deletions(-)
> +void *virObjectLockableNew(virClassPtr klass)
> +{
> + virObjectLockablePtr obj;
> +
> + if (!virClassIsDerivedFrom(klass, virClassForObjectLockable())) {
> + virReportInvalidArg(klass,
> + _("Class %s must derive from
virObjectLockable"),
> + virClassName(klass));
> + return NULL;
> + }
> +
> + if (!(obj = virObjectNew(klass)))
> + return NULL;
> +
> + if (virMutexInit(&obj->lock) < 0) {
> + virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to initialize mutex"));
> + virObjectUnref(obj);
> + return NULL;
> + }
This creates the new object locked...
No, mutexes are initially unlocked.
> +
> + return obj;
> +}
> +
> +
> +static void virObjectLockableDispose(void *anyobj)
> +{
> + virObjectLockablePtr obj = anyobj;
> +
> + virMutexDestroy(&obj->lock);
...but this doesn't unlock anything. You may want to document that the
user is required to unlock the object before losing the last reference.
So this isn't an issue, though we ought to document locking rules
I guess.
> + * @anyobj: any instance of virObjectLockablePtr
> + *
> + * Acquire a lock on @anyobj. The lock must be
> + * released by virObjectUnlock.
> + */
> +void virObjectLock(void *anyobj)
> +{
> + virObjectLockablePtr obj = anyobj;
Is it worth a sanity check that anyobj is actually an object of the
right class, before we blindly dereference something wrong due to a
coding error?
> +void virObjectUnlock(void *anyobj)
> +{
> + virObjectLockablePtr obj = anyobj;
> +
And again, would a sanity check help?
Possibily, though what action should we take. These methods really
need to be void, because it is not practical to check return values
everywhere. Meanwhile we don't like to abort(). So that leaves the
possibility of a VIR_WARN ?
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 :|