11:26 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/