Re: [libvirt] PATCH: 4/28: Thread safety for test driver

1 Dec 2008

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@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

    