On Mon, Dec 01, 2008 at 06:26:03PM +0100, Daniel Veillard wrote:
> On Sun, Nov 30, 2008 at 11:27:14PM +0000, Daniel P. Berrange wrote:
> > This patch makes the test driver thread safe, adding a global driver lock,
> > and the neccessary locking calls on domain/network/storagepool objects.
> >
> > You'll notice there are many calls to
> >
> > virDomainObjUnlock
> >
> > but very few corresponding calls to
> >
> > virDomainObjLock
> >
> > This is because the contract of virDomainFindByUUID declares that the
> > object it returns is already locked.
> >
> > Methods which create / delete virDomainObj instances have to keep the
> > global driver lock held for their whole duration, but others can drop
> > it immediately after getting the virDomainObjPtr instance.
>
> Okay, what about virDomainFindByID and virDomainFindByName ? They lock
> too or we are just avoiding them ?
All the virXXXXFindByYYY methods return a locked object. I'll be
documenting writing a doc about the threading rules for the internal
code to explain this more clearlyl....
>
> > +++ b/src/test.c
> > @@ -58,6 +58,8 @@ typedef struct _testCell *testCellPtr;
> > #define MAX_CELLS 128
> >
> > struct _testConn {
> > + PTHREAD_MUTEX_T(lock);
> > +
>
> hum,
> [...]
> > +
> > +static void testDriverLock(testConnPtr driver)
> > +{
> > + pthread_mutex_lock(&driver->lock);
> > +}
> > +
> > +static void testDriverUnlock(testConnPtr driver)
> > +{
> > + pthread_mutex_unlock(&driver->lock);
> > +}
> >
> > static virCapsPtr
> > testBuildCapabilities(virConnectPtr conn) {
> > @@ -200,6 +212,8 @@ static int testOpenDefault(virConnectPtr
> > return VIR_DRV_OPEN_ERROR;
> > }
> > conn->privateData = privconn;
> > + pthread_mutex_init(&privconn->lock, NULL);
>
> Can we abstract the mutex types and ops a little ? if libvirt were
> to be ported to a platform/compiler where pthreads are not available
> they would be able to fallback to xmlMutexPtr from libvirt which is
> ported to all platforms where libxml2 is usable (as far as I know).
The internal.h header already arranges for all this to compile
away to nothing if pthread.h is not present, so there's no need
for any wrappers. xmlMutexPtr is not sufficient, because other
in the code also use condition variables for which there are no
wrappers in libxml. That said, all modern UNIX, Linux, OS-X
and Windows have pthread libraries available, so I don't think
there's any important platform where we'll need to disable this.
>
> > + testDriverLock(privconn);
> >
> > if (gettimeofday(&tv, NULL) < 0) {
> > testError(NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("getting time of day"));
> > @@ -232,6 +246,7 @@ static int testOpenDefault(virConnectPtr
> > domobj->def->id = privconn->nextDomID++;
> > domobj->state = VIR_DOMAIN_RUNNING;
> > domobj->persistent = 1;
> > + virDomainObjUnlock(domobj);
>
> the locking path is unclear here testOpenDefault uses
> virDomainAssignDef which then relies on virDomainFindByName() which
> would lock the domain object, right ?
Yes, the virXXXXAssignDef method is declared to return a locked object.
>
> [...]
> > @@ -797,10 +859,14 @@ static int testDestroyDomain (virDomainP
> > if (!privdom->persistent) {
> > virDomainRemoveInactive(&privconn->domains,
> > privdom);
>
> will virDomainRemoveInactive also remove the lock before destroying
> the object ? the answer is surely in a later patch
You are required to hold both the driver lock, and the privdom
lock when calling this. It will unlock the domain object and
then free it. It may seemm redudant for the caller to lock the
privdom, and then have virDomainRemoveInactive immediately
unlock it again, but this ensures no other thread still has an
active lock on the object & cannot re-acquire one since we hold
the global driver lock.
> > @@ -884,9 +959,17 @@ static int testShutdownDomain (virDomain
> > privdom->state = VIR_DOMAIN_SHUTOFF;
> > domain->id = -1;
> > privdom->def->id = -1;
> > + if (!privdom->persistent) {
> > + virDomainRemoveInactive(&privconn->domains,
> > + privdom);
> > + privdom = NULL;
> > + }
> > ret = 0;
>
> hum, not specific to threading, we are improving the driver here.
Yes, i noticed that bug while fixing this.
> > diff --git a/tests/virsh-all b/tests/virsh-all
> > --- a/tests/virsh-all
> > +++ b/tests/virsh-all
> > @@ -22,6 +22,7 @@ fi
> > fi
> >
> > test -z "$srcdir" && srcdir=$(pwd)
> > +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/..
> > . "$srcdir/test-lib.sh"
>
> ???
It allows you to rnu the test by just doing
./virsh-all
Rather than tedious
make check TESTS=virsh-all
> I hope it's worth the effort, it's a lot of complexity added.
> One of the things which worries me is that detecting errors will be
> hard, you end up with a locked server that can be far from trivial
> to debug.
> I'm really wondering how we could automate testing or at least ease the
> debug,
It occurred to me that a static analysis tool like CIL really ought to
be able to check correctness fairly easily. Rich has discussed this a
little before
http://et.redhat.com/~rjones/cil-analysis-of-libvirt/
Basically since we have a known set of functions which lock the object
and CIL can give us the code flow within a method, we ought to be able
to write something that looks at each call of virDomainFindByUUID,
and then traces the code paths to functiuon return, and validates that
the unlock call was made. It could likewise check for people calling
things before acquiring the lock.
Would be an interesting project..... :-)
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list