[libvirt] PATCH: Fix misc locking bugs, and add lock checker

[Daniel Veillard]

On Thu, May 14, 2009 at 05:53:41PM +0100, Daniel P. Berrange wrote:
> This patch fixes a number of locking bugs, some serious, some not.
> 
> It also adds the CIL lock checker I previously wrote. It is improved since
> last time, because it explicitly looks for the virDriver static variables
> and uses that to extract list of functions to be checked. This removes a
> huge number of false positives. It also now checks for a couple of dead
> lock scenarios, in addition to previous checks for lock ordering correctness.
> 
> The serious locking bugs 
> 
>  - qemudDomainMigratePrepare2: didn't unlock VM causing deadlock
>  - storagePoolRefresh: unlocked the driver too early
>  - storageVolumeCreateXML: locked the pool without having locked driver
>  - umlDomainGetAutostart: missing unlock call with later deadlock
> 
[...]
> --- a/src/network_driver.c	Thu May 14 17:38:11 2009 +0100
> +++ b/src/network_driver.c	Thu May 14 17:47:01 2009 +0100
> @@ -1217,7 +1217,6 @@ static int networkStart(virNetworkPtr ne
>  
>      networkDriverLock(driver);
>      network = virNetworkFindByUUID(&driver->networks, net->uuid);
> -    networkDriverUnlock(driver);
>  
>      if (!network) {
>          networkReportError(net->conn, NULL, net, VIR_ERR_INVALID_NETWORK,
> @@ -1230,6 +1229,7 @@ static int networkStart(virNetworkPtr ne
>  cleanup:
>      if (network)
>          virNetworkObjUnlock(network);
> +    networkDriverUnlock(driver);
>      return ret;
>  }

  A lot of the changes which are not in that list look like the above,
instead of unlocking immediately after finding the network structure,
the unlocking is postponed until the end of the operation. What's the
reasonning ? Is that a safety against possible changes in the functions
i.e. just a preventive change or is there something we expected to be
safe which turns out not ?

> @@ -1240,7 +1240,6 @@ static int networkDestroy(virNetworkPtr 
> @@ -1349,7 +1349,6 @@ static int networkSetAutostart(virNetwor
> @@ -2229,7 +2230,6 @@ static int qemudDomainSuspend(virDomainP
> @@ -2282,7 +2280,6 @@ static int qemudDomainResume(virDomainPt
> @@ -3153,7 +3148,6 @@ static int qemudDomainGetSecurityLabel(v
> @@ -3617,7 +3612,6 @@ static int qemudDomainStart(virDomainPtr
> @@ -4144,7 +4136,6 @@ static int qemudDomainAttachDevice(virDo
> @@ -4296,7 +4288,6 @@ static int qemudDomainDetachDevice(virDo
> @@ -4359,7 +4351,6 @@ static int qemudDomainSetAutostart(virDo
> @@ -792,7 +792,6 @@ storagePoolRefresh(virStoragePoolPtr obj
> @@ -939,7 +939,6 @@ storagePoolSetAutostart(virStoragePoolPt
> @@ -1553,7 +1553,6 @@ static int umlDomainStart(virDomainPtr d
> @@ -1684,7 +1684,6 @@ static int umlDomainSetAutostart(virDoma

  they all share the pattern of changes above

> diff -r 71a7d1d0ad9b src/qemu_driver.c
> --- a/src/qemu_driver.c	Thu May 14 17:38:11 2009 +0100
> +++ b/src/qemu_driver.c	Thu May 14 17:47:01 2009 +0100
> @@ -1811,9 +1811,10 @@ static virDrvOpenStatus qemudOpen(virCon
>  static int qemudClose(virConnectPtr conn) {
>      struct qemud_driver *driver = conn->privateData;
>  
> +    qemuDriverLock(driver);
>      /* Get rid of callbacks registered for this conn */
>      virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks);
> -
> +    qemuDriverUnlock(driver);
>      conn->privateData = NULL;

  okay

>      return 0;
> @@ -4985,6 +4977,7 @@ qemudDomainMigratePrepare2 (virConnectPt
>                                vm->def->name);
>              goto cleanup;
>          }
> +        virDomainObjUnlock(vm);
>      }

  okay

>      if (!(vm = virDomainAssignDef(dconn,
> diff -r 71a7d1d0ad9b src/storage_driver.c
> --- a/src/storage_driver.c	Thu May 14 17:38:11 2009 +0100
> +++ b/src/storage_driver.c	Thu May 14 17:47:01 2009 +0100
> @@ -1274,7 +1274,10 @@ storageVolumeCreateXML(virStoragePoolPtr
>  
>          buildret = backend->buildVol(obj->conn, buildvoldef);
>  
> +        storageDriverLock(driver);
>          virStoragePoolObjLock(pool);
> +        storageDriverUnlock(driver);
> +
>          voldef->building = 0;
>          pool->asyncjobs--;

  okay

> diff -r 71a7d1d0ad9b src/test.c
> --- a/src/test.c	Thu May 14 17:38:11 2009 +0100
> +++ b/src/test.c	Thu May 14 17:47:01 2009 +0100
> @@ -659,10 +659,12 @@ static virDrvOpenStatus testOpen(virConn
>  
>      if (ret == VIR_DRV_OPEN_SUCCESS) {
>          testConnPtr privconn = conn->privateData;
> +        testDriverLock(privconn);
>          /* Init callback list */
>          if (VIR_ALLOC(privconn->domainEventCallbacks) < 0 ||
>              !(privconn->domainEventQueue = virDomainEventQueueNew())) {
>              virReportOOMError(NULL);
> +            testDriverUnlock(privconn);
>              testClose(conn);
>              return VIR_DRV_OPEN_ERROR;
>          }
> @@ -671,6 +673,7 @@ static virDrvOpenStatus testOpen(virConn
>               virEventAddTimeout(-1, testDomainEventFlush, privconn, NULL)) < 0)
>              DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. "
>                     "continuing without events.");
> +        testDriverUnlock(privconn);
>      }

  okay

>      return (ret);

  Everything else is the CIL testing, looks good ! ACK

but a hint about the systematic changes about extended locking would
be good :-)

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/