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 ?
> +++ 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).
> + 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 ?
> if (!(netdef = virNetworkDefParseString(conn, defaultNetworkXML)))
> goto error;
> @@ -241,6 +256,7 @@ static int testOpenDefault(virConnectPtr
> }
> netobj->active = 1;
> netobj->persistent = 1;
> + virNetworkObjUnlock(netobj);
same with virNetworkAssignDef()
> if (!(pooldef = virStoragePoolDefParse(conn, defaultPoolXML, NULL)))
> goto error;
> @@ -250,10 +266,15 @@ static int testOpenDefault(virConnectPtr
> virStoragePoolDefFree(pooldef);
> goto error;
> }
> - if (testStoragePoolObjSetDefaults(poolobj) == -1)
> +
> + if (testStoragePoolObjSetDefaults(poolobj) == -1) {
> + virStoragePoolObjUnlock(poolobj);
> goto error;
ideally done in the error: switch but this makes things harder
> + }
> poolobj->active = 1;
> + virStoragePoolObjUnlock(poolobj);
>
> + testDriverUnlock(privconn);
> return VIR_DRV_OPEN_SUCCESS;
>
> error:
> @@ -261,6 +282,7 @@ error:
> virNetworkObjListFree(&privconn->networks);
> virStoragePoolObjListFree(&privconn->pools);
> virCapabilitiesFree(privconn->caps);
> + testDriverUnlock(privconn);
> VIR_FREE(privconn);
> return VIR_DRV_OPEN_ERROR;
> }
> @@ -307,6 +329,8 @@ static int testOpenFromFile(virConnectPt
okay, similar to testOpenDefault()
[...]
> @@ -710,7 +748,11 @@ static virDomainPtr testLookupDomainByID
> virDomainPtr ret = NULL;
> virDomainObjPtr dom;
>
> - if ((dom = virDomainFindByID(&privconn->domains, id)) == NULL) {
> + testDriverLock(privconn);
> + dom = virDomainFindByID(&privconn->domains, id);
> + testDriverUnlock(privconn);
so all virDomainFindBy__ do the locking, fine.
[...]
> @@ -750,7 +800,11 @@ static virDomainPtr testLookupDomainByNa
> virDomainPtr ret = NULL;
> virDomainObjPtr dom;
>
> - if ((dom = virDomainFindByName(&privconn->domains, name)) == NULL) {
> + testDriverLock(privconn);
> + dom = virDomainFindByName(&privconn->domains, name);
> + testDriverUnlock(privconn);
> +
> + if (dom == NULL) {
actually a lot of those sequences are common, and correspond to the
macros. later we may want to refactor maybe
[...]
> @@ -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
[...]
> @@ -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.
> cleanup:
> + if (privdom)
> + virDomainObjUnlock(privdom);
> + testDriverUnlock(privconn);
> return ret;
> }
>
okay, others functions are easier, but it's still painful to review.
> 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"
???
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,
+1
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list