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

[Daniel Veillard]

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