Re: [libvirt] PATCH: 3/5: Locking in the QEMU driver

Friday, 17 October 2008

This patch makes use of the virDomainObjPtr locking infrastructure to
provide thread safety in the QEMU driver. The QEMU driver itself gains
a global lock. The idea is this lock is only held for short periods of
time - typically only while obtaining a virDomainObjPtr instance.

The main limitation of the current locking implemented here, is that
the virDomainObjPtr lock is held for the duration of all monitor
commands. Some of these can take quite a long time to execute, so for
a useful level of concurrency we need to drop the lock & reobtain it
when issuing monitor commands. I intend to deal with this problem in
an add-on patch, since I'd rather keep the initial impl simple to
understand.

The general model for an API call involving a virDomainPtr is

   qemudXXXXX(virDomainPtr dom) {
     struct qemud_driver *driver = conn->privateData;
     virDomainObjPtr vm;

     qemudDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, uuid);
     qemudDriverUnlock(driver);

     .... do work ....

     virDomainObjUnlock(vm);
     return 0;

   error:
     virDomainObjUnlock(vm);
     return -1;
   }


The important thing to note is that if anything goes wrong in the
'... do work...' bit of the driver call, it should never do
a direct 'return -1;'. It must always 'goto error' instead. This
avoids the need to sprinkle calls to virDomainObjUnlock(vm)
everywhere.

Daniel

diff --git a/src/qemu_conf.h b/src/qemu_conf.h
--- a/src/qemu_conf.h
+++ b/src/qemu_conf.h
@@ -48,6 +48,8 @@ enum qemud_cmd_flags {
 
 /* Main driver state */
 struct qemud_driver {
+    pthread_mutex_t lock;
+
     unsigned int qemuVersion;
     int nextvmid;
 
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -76,6 +76,22 @@ static int qemudShutdown(void);
 
 #define qemudLog(level, msg...) fprintf(stderr, msg)
 
+
+static int
+qemudDriverLock(struct qemud_driver *driver)
+{
+    DEBUG("LOCK driver %p %p", (driver), (void*)pthread_self());
+    return pthread_mutex_lock(&(driver)->lock);
+}
+
+static int
+qemudDriverUnlock(struct qemud_driver *driver)
+{
+    DEBUG("UNLOCK driver %p %p", (driver), (void*)pthread_self());
+    return pthread_mutex_unlock(&(driver)->lock);
+}
+
+
 static int qemudSetCloseExec(int fd) {
     int flags;
     if ((flags = fcntl(fd, F_GETFD)) < 0)
@@ -130,13 +146,18 @@ qemudAutostartConfigs(struct qemud_drive
     unsigned int i;
 
     for (i = 0 ; i < driver->domains.count ; i++) {
-        if (driver->domains.objs[i]->autostart &&
-            !virDomainIsActive(driver->domains.objs[i]) &&
-            qemudStartVMDaemon(NULL, driver, driver->domains.objs[i], NULL) < 0) {
+        virDomainObjPtr dom = driver->domains.objs[i];
+        virDomainLock(dom);
+
+        if (dom->autostart &&
+            !virDomainIsActive(dom) &&
+            qemudStartVMDaemon(NULL, driver, dom, NULL) < 0) {
             virErrorPtr err = virGetLastError();
             qemudLog(QEMUD_ERR, _("Failed to autostart VM '%s':
%s\n"),
-                     driver->domains.objs[i]->def->name, err->message);
+                     dom->def->name, err->message);
         }
+
+        virDomainUnlock(dom);
     }
 }
 
@@ -157,6 +178,7 @@ qemudStartup(void) {
 
     /* Don't have a dom0 so start from 1 */
     qemu_driver->nextvmid = 1;
+    pthread_mutex_init(&qemu_driver->lock, NULL);
 
     if (!uid) {
         if (asprintf(&qemu_driver->logDir,
@@ -203,15 +225,20 @@ qemudStartup(void) {
         return -1;
     }
 
+    qemudDriverLock(qemu_driver);
+
     if (virDomainLoadAllConfigs(NULL,
                                 qemu_driver->caps,
                                 &qemu_driver->domains,
                                 qemu_driver->configDir,
                                 qemu_driver->autostartDir) < 0) {
+        qemudDriverUnlock(qemu_driver);
         qemudShutdown();
         return -1;
     }
     qemudAutostartConfigs(qemu_driver);
+
+    qemudDriverUnlock(qemu_driver);
 
     return 0;
 
@@ -235,6 +262,8 @@ qemudReload(void) {
     if (!qemu_driver)
         return 0;
 
+    qemudDriverLock(qemu_driver);
+
     virDomainLoadAllConfigs(NULL,
                             qemu_driver->caps,
                             &qemu_driver->domains,
@@ -242,6 +271,8 @@ qemudReload(void) {
                             qemu_driver->autostartDir);
 
     qemudAutostartConfigs(qemu_driver);
+
+    qemudDriverUnlock(qemu_driver);
 
     return 0;
 }
@@ -257,16 +288,23 @@ static int
 static int
 qemudActive(void) {
     unsigned int i;
+    int isactive = 0;
 
     if (!qemu_driver)
         return 0;
 
-    for (i = 0 ; i < qemu_driver->domains.count ; i++)
+    qemudDriverLock(qemu_driver);
+
+    for (i = 0 ; i < qemu_driver->domains.count ; i++) {
+        virDomainLock(qemu_driver->domains.objs[i]);
         if (virDomainIsActive(qemu_driver->domains.objs[i]))
-            return 1;
+            isactive = 1;
+        virDomainUnlock(qemu_driver->domains.objs[i]);
+    }
 
-    /* Otherwise we're happy to deal with a shutdown */
-    return 0;
+    qemudDriverUnlock(qemu_driver);
+
+    return isactive;
 }
 
 /**
@@ -1222,12 +1260,16 @@ static char *qemudGetCapabilities(virCon
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     char *xml;
 
+    qemudDriverLock(driver);
+
     if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) {
+        qemudDriverUnlock(driver);
         qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
                  "%s", _("failed to allocate space for capabilities
support"));
         return NULL;
     }
 
+    qemudDriverUnlock(driver);
     return xml;
 }
 
@@ -1327,8 +1369,12 @@ static virDomainPtr qemudDomainLookupByI
 static virDomainPtr qemudDomainLookupByID(virConnectPtr conn,
                                           int id) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
+    virDomainObjPtr vm;
     virDomainPtr dom;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, id);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1337,13 +1383,19 @@ static virDomainPtr qemudDomainLookupByI
 
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
     if (dom) dom->id = vm->def->id;
+
+    virDomainUnlock(vm);
     return dom;
 }
 static virDomainPtr qemudDomainLookupByUUID(virConnectPtr conn,
                                             const unsigned char *uuid) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, uuid);
+    virDomainObjPtr vm;
     virDomainPtr dom;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1352,13 +1404,19 @@ static virDomainPtr qemudDomainLookupByU
 
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
     if (dom) dom->id = vm->def->id;
+
+    virDomainUnlock(vm);
     return dom;
 }
 static virDomainPtr qemudDomainLookupByName(virConnectPtr conn,
                                             const char *name) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
-    virDomainObjPtr vm = virDomainFindByName(&driver->domains, name);
+    virDomainObjPtr vm;
     virDomainPtr dom;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByName(&driver->domains, name);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_NO_DOMAIN, NULL);
@@ -1367,15 +1425,23 @@ static virDomainPtr qemudDomainLookupByN
 
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
     if (dom) dom->id = vm->def->id;
+
+    virDomainUnlock(vm);
     return dom;
 }
 
 static int qemudGetVersion(virConnectPtr conn, unsigned long *version) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
-    if (qemudExtractVersion(conn, driver) < 0)
+    qemudDriverLock(driver);
+
+    if (qemudExtractVersion(conn, driver) < 0) {
+        qemudDriverUnlock(driver);
         return -1;
+    }
 
     *version = qemu_driver->qemuVersion;
+
+    qemudDriverUnlock(driver);
     return 0;
 }
 
@@ -1405,39 +1471,51 @@ static int qemudListDomains(virConnectPt
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     int got = 0, i;
 
-    for (i = 0 ; i < driver->domains.count && got < nids ; i++)
+    qemudDriverLock(driver);
+    for (i = 0 ; i < driver->domains.count && got < nids ; i++) {
+        virDomainLock(driver->domains.objs[i]);
         if (virDomainIsActive(driver->domains.objs[i]))
             ids[got++] = driver->domains.objs[i]->def->id;
+        virDomainUnlock(driver->domains.objs[i]);
+    }
+    qemudDriverUnlock(driver);
 
     return got;
 }
+
 static int qemudNumDomains(virConnectPtr conn) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     int n = 0, i;
 
-    for (i = 0 ; i < driver->domains.count ; i++)
+    qemudDriverLock(driver);
+    for (i = 0 ; i < driver->domains.count ; i++) {
+        virDomainLock(driver->domains.objs[i]);
         if (virDomainIsActive(driver->domains.objs[i]))
             n++;
+        virDomainUnlock(driver->domains.objs[i]);
+    }
+    qemudDriverUnlock(driver);
 
     return n;
 }
 static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
                                       unsigned int flags ATTRIBUTE_UNUSED) {
     virDomainDefPtr def;
-    virDomainObjPtr vm;
+    virDomainObjPtr vm = NULL;
     virDomainPtr dom;
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
 
+    qemudDriverLock(driver);
+
     if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
-        return NULL;
+        goto cleanup;
 
     vm = virDomainFindByName(&driver->domains, def->name);
     if (vm) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          _("domain '%s' is already defined"),
                          def->name);
-        virDomainDefFree(def);
-        return NULL;
+        goto cleanup;
     }
     vm = virDomainFindByUUID(&driver->domains, def->uuid);
     if (vm) {
@@ -1447,149 +1525,225 @@ static virDomainPtr qemudDomainCreate(vi
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          _("domain with uuid '%s' is already
defined"),
                          uuidstr);
-        virDomainDefFree(def);
-        return NULL;
+        goto cleanup;
     }
 
     if (!(vm = virDomainAssignDef(conn,
                                   &driver->domains,
-                                  def))) {
-        virDomainDefFree(def);
-        return NULL;
-    }
+                                  def)))
+        goto cleanup;
 
-    if (qemudStartVMDaemon(conn, driver, vm, NULL) < 0) {
-        virDomainRemoveInactive(&driver->domains,
-                                vm);
-        return NULL;
-    }
+    /* XXX should figure out how to release driver lock here
+     * so we don't block during startup */
+    if (qemudStartVMDaemon(conn, driver, vm, NULL) < 0)
+        goto cleanup;
 
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
     if (dom) dom->id = vm->def->id;
+
+    virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
     return dom;
+
+cleanup:
+    if (vm) {
+        virDomainUnlock(vm);
+        virDomainRemoveInactive(&driver->domains,
+                                vm);
+    } else {
+        virDomainDefFree(def);
+    }
+
+    qemudDriverUnlock(driver);
+    return NULL;
 }
 
 
 static int qemudDomainSuspend(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    char *info;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    char *info = NULL;
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
+
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, _("no
domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
-    if (vm->state == VIR_DOMAIN_PAUSED)
-        return 0;
 
-    if (qemudMonitorCommand(vm, "stop", &info) < 0) {
-        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                         "%s", _("suspend operation failed"));
-        return -1;
+    if (vm->state != VIR_DOMAIN_PAUSED) {
+        if (qemudMonitorCommand(vm, "stop", &info) < 0) {
+            qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                             "%s", _("suspend operation failed"));
+            goto cleanup;
+        }
+        vm->state = VIR_DOMAIN_PAUSED;
+        qemudDebug("Reply %s", info);
+        VIR_FREE(info);
     }
-    vm->state = VIR_DOMAIN_PAUSED;
-    qemudDebug("Reply %s", info);
+
+    virDomainUnlock(vm);
+    return 0;
+
+cleanup:
     VIR_FREE(info);
-    return 0;
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
 static int qemudDomainResume(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    char *info;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    char *info = NULL;
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
+
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
-    if (vm->state == VIR_DOMAIN_RUNNING)
-        return 0;
-    if (qemudMonitorCommand(vm, "cont", &info) < 0) {
-        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                         "%s", _("resume operation failed"));
-        return -1;
+    if (vm->state != VIR_DOMAIN_RUNNING) {
+        if (qemudMonitorCommand(vm, "cont", &info) < 0) {
+            qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                             "%s", _("resume operation failed"));
+            goto cleanup;
+        }
+        vm->state = VIR_DOMAIN_RUNNING;
+        qemudDebug("Reply %s", info);
+        VIR_FREE(info);
     }
-    vm->state = VIR_DOMAIN_RUNNING;
-    qemudDebug("Reply %s", info);
+
+    virDomainUnlock(vm);
+    return 0;
+
+cleanup:
     VIR_FREE(info);
-    return 0;
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
 static int qemudDomainShutdown(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
-    char* info;
+    virDomainObjPtr vm;
+    char* info = NULL;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
 
     if (qemudMonitorCommand(vm, "system_powerdown", &info) < 0) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("shutdown operation failed"));
-        return -1;
+        goto cleanup;
     }
     VIR_FREE(info);
+    virDomainUnlock(vm);
     return 0;
 
+cleanup:
+    VIR_FREE(info);
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
 static int qemudDomainDestroy(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
 
     qemudShutdownVMDaemon(dom->conn, driver, vm);
-    if (!vm->persistent)
+    if (!vm->persistent) {
+        virDomainUnlock(vm);
         virDomainRemoveInactive(&driver->domains,
                                 vm);
+        return 0;
+    }
 
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
 static char *qemudDomainGetOSType(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     char *type;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
-        return NULL;
+        goto cleanup;
     }
 
     if (!(type = strdup(vm->def->os.type))) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY,
                          "%s", _("failed to allocate space for
ostype"));
-        return NULL;
+        goto cleanup;
     }
+
+    virDomainUnlock(vm);
     return type;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return NULL;
 }
 
 /* Returns max memory in kb, 0 if error */
 static unsigned long qemudDomainGetMaxMemory(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+    unsigned long ret;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1597,15 +1751,27 @@ static unsigned long qemudDomainGetMaxMe
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching uuid '%s'"),
uuidstr);
-        return 0;
+        goto cleanup;
     }
 
-    return vm->def->maxmem;
+    ret = vm->def->maxmem;
+    virDomainUnlock(vm);
+
+    return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return 0;
 }
 
 static int qemudDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1613,22 +1779,32 @@ static int qemudDomainSetMaxMemory(virDo
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching uuid '%s'"),
uuidstr);
-        return -1;
+        goto cleanup;
     }
 
     if (newmax < vm->def->memory) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s", _("cannot set max memory lower than current
memory"));
-        return -1;
+        goto cleanup;
     }
 
     vm->def->maxmem = newmax;
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1636,33 +1812,44 @@ static int qemudDomainSetMemory(virDomai
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching uuid '%s'"),
uuidstr);
-        return -1;
+        goto cleanup;
     }
 
     if (virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          "%s", _("cannot set memory of an active
domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (newmem > vm->def->maxmem) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s", _("cannot set memory higher than max
memory"));
-        return -1;
+        goto cleanup;
     }
 
     vm->def->memory = newmem;
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int qemudDomainGetInfo(virDomainPtr dom,
                               virDomainInfoPtr info) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
+
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
-        return -1;
+        goto cleanup;
     }
 
     info->state = vm->state;
@@ -1671,15 +1858,23 @@ static int qemudDomainGetInfo(virDomainP
         info->cpuTime = 0;
     } else {
         if (qemudGetProcessInfo(&(info->cpuTime), vm->pid) < 0) {
-            qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
("cannot read cputime for domain"));
-            return -1;
+            qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                             _("cannot read cputime for domain"));
+            goto cleanup;
         }
     }
 
     info->maxMem = vm->def->maxmem;
     info->memory = vm->def->memory;
     info->nrVirtCpu = vm->def->vcpus;
+
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
@@ -1779,12 +1974,16 @@ static int qemudDomainSave(virDomainPtr 
 static int qemudDomainSave(virDomainPtr dom,
                            const char *path) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    virDomainObjPtr vm;
     char *command, *info;
-    int fd;
+    int fd = -1;
     char *safe_path;
-    char *xml;
+    char *xml = NULL;
     struct qemud_save_header header;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
 
     memset(&header, 0, sizeof(header));
     memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
@@ -1793,13 +1992,13 @@ static int qemudDomainSave(virDomainPtr 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
 
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
 
     /* Pause */
@@ -1808,7 +2007,7 @@ static int qemudDomainSave(virDomainPtr 
         if (qemudDomainSuspend(dom) != 0) {
             qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                              "%s", _("failed to pause domain"));
-            return -1;
+            goto cleanup;
         }
     }
 
@@ -1817,7 +2016,7 @@ static int qemudDomainSave(virDomainPtr 
     if (!xml) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to get domain xml"));
-        return -1;
+        goto cleanup;
     }
     header.xml_len = strlen(xml) + 1;
 
@@ -1825,24 +2024,19 @@ static int qemudDomainSave(virDomainPtr 
     if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          _("failed to create '%s'"), path);
-        VIR_FREE(xml);
-        return -1;
+        goto cleanup;
     }
 
     if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to write save header"));
-        close(fd);
-        VIR_FREE(xml);
-        return -1;
+        goto cleanup;
     }
 
     if (safewrite(fd, xml, header.xml_len) != header.xml_len) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to write xml"));
-        close(fd);
-        VIR_FREE(xml);
-        return -1;
+        goto cleanup;
     }
 
     close(fd);
@@ -1890,18 +2084,33 @@ static int qemudDomainSave(virDomainPtr 
 
     /* Shut it down */
     qemudShutdownVMDaemon(dom->conn, driver, vm);
-    if (!vm->persistent)
+    if (!vm->persistent) {
+        virDomainUnlock(vm);
         virDomainRemoveInactive(&driver->domains,
                                 vm);
+        return 0;
+    }
 
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (fd != -1)
+        close(fd);
+    VIR_FREE(xml);
+    virDomainUnlock(vm);
+    return -1;
 }
 
 
 static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     int max;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -1909,30 +2118,37 @@ static int qemudDomainSetVcpus(virDomain
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching uuid '%s'"),
uuidstr);
-        return -1;
+        goto cleanup;
     }
 
     if (virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, "%s",
                          _("cannot change vcpu count of an active domain"));
-        return -1;
+        goto cleanup;
     }
 
     if ((max = qemudDomainGetMaxVcpus(dom)) < 0) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
"%s",
                          _("could not determine max vcpus for the domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (nvcpus > max) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          _("requested vcpus is greater than max allowable"
                            " vcpus for the domain: %d > %d"), nvcpus, max);
-        return -1;
+        goto cleanup;
     }
 
     vm->def->vcpus = nvcpus;
+
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
@@ -1943,26 +2159,39 @@ qemudDomainPinVcpu(virDomainPtr dom,
                    unsigned char *cpumap,
                    int maplen) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     cpu_set_t mask;
     int i, maxcpu;
     virNodeInfo nodeinfo;
 
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
+
+    if (!vm) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+        virUUIDFormat(dom->uuid, uuidstr);
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         _("no domain with matching uuid '%s'"),
uuidstr);
+        goto cleanup;
+    }
+
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s",_("cannot pin vcpus on an inactive
domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (vcpu > (vm->nvcpupids-1)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          _("vcpu number out of range %d > %d"),
                          vcpu, vm->nvcpupids);
-        return -1;
+        goto cleanup;
     }
 
     if (virNodeInfoPopulate(dom->conn, &nodeinfo) < 0)
-        return -1;
+        goto cleanup;
 
     maxcpu = maplen * 8;
     if (maxcpu > nodeinfo.cpus)
@@ -1978,15 +2207,21 @@ qemudDomainPinVcpu(virDomainPtr dom,
         if (sched_setaffinity(vm->vcpupids[vcpu], sizeof(mask), &mask) < 0) {
             qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                              _("cannot set affinity: %s"), strerror(errno));
-            return -1;
+            goto cleanup;
         }
     } else {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          "%s", _("cpu affinity is not supported"));
-        return -1;
+        goto cleanup;
     }
 
+    virDomainUnlock(vm);
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int
@@ -1996,18 +2231,31 @@ qemudDomainGetVcpus(virDomainPtr dom,
                     unsigned char *cpumaps,
                     int maplen) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     virNodeInfo nodeinfo;
     int i, v, maxcpu;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
+
+    if (!vm) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+        virUUIDFormat(dom->uuid, uuidstr);
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         _("no domain with matching uuid '%s'"),
uuidstr);
+        goto cleanup;
+    }
 
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s",_("cannot pin vcpus on an inactive
domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (virNodeInfoPopulate(dom->conn, &nodeinfo) < 0)
-        return -1;
+        goto cleanup;
 
     maxcpu = maplen * 8;
     if (maxcpu > nodeinfo.cpus)
@@ -2017,8 +2265,10 @@ qemudDomainGetVcpus(virDomainPtr dom,
     if (maxinfo > vm->nvcpupids)
         maxinfo = vm->nvcpupids;
 
-    if (maxinfo < 1)
+    if (maxinfo < 1) {
+        virDomainUnlock(vm);
         return 0;
+    }
 
     if (info != NULL) {
         memset(info, 0, sizeof(*info) * maxinfo);
@@ -2040,7 +2290,7 @@ qemudDomainGetVcpus(virDomainPtr dom,
                 if (sched_getaffinity(vm->vcpupids[v], sizeof(mask), &mask) <
0) {
                     qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                                      _("cannot get affinity: %s"),
strerror(errno));
-                    return -1;
+                    goto cleanup;
                 }
 
                 for (i = 0 ; i < maxcpu ; i++)
@@ -2050,20 +2300,30 @@ qemudDomainGetVcpus(virDomainPtr dom,
         } else {
             qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                              "%s", _("cpu affinity is not
available"));
-            return -1;
+            goto cleanup;
         }
     }
 
+    virDomainUnlock(vm);
     return maxinfo;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 #endif /* HAVE_SCHED_GETAFFINITY */
 
 
 static int qemudDomainGetMaxVcpus(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     const char *type;
     int ret;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2071,21 +2331,27 @@ static int qemudDomainGetMaxVcpus(virDom
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          _("no domain with matching uuid '%s'"),
uuidstr);
-        return -1;
+        goto cleanup;
     }
 
     if (!(type = virDomainVirtTypeToString(vm->def->virtType))) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("unknown virt type in domain definition
'%d'"),
                          vm->def->virtType);
-        return -1;
+        goto cleanup;
     }
 
     if ((ret = qemudGetMaxVCPUs(dom->conn, type)) < 0) {
-        return -1;
+        goto cleanup;
     }
 
+    virDomainUnlock(vm);
     return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 
@@ -2093,10 +2359,10 @@ static int qemudDomainRestore(virConnect
                        const char *path) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     virDomainDefPtr def;
-    virDomainObjPtr vm;
-    int fd;
+    virDomainObjPtr vm = NULL;
+    int fd = -1;
     int ret;
-    char *xml;
+    char *xml = NULL;
     struct qemud_save_header header;
 
     /* Verify the header and read the XML */
@@ -2143,13 +2409,14 @@ static int qemudDomainRestore(virConnect
         return -1;
     }
 
+
+    qemudDriverLock(driver);
+
     /* Create a domain from this XML */
     if (!(def = virDomainDefParseString(conn, driver->caps, xml))) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to parse XML"));
-        close(fd);
-        VIR_FREE(xml);
-        return -1;
+        goto cleanup;
     }
     VIR_FREE(xml);
 
@@ -2160,9 +2427,10 @@ static int qemudDomainRestore(virConnect
     if (vm && virDomainIsActive(vm)) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          _("domain is already active as '%s'"),
vm->def->name);
-        close(fd);
-        return -1;
+        goto cleanup;
     }
+    if (vm)
+        virDomainUnlock(vm);
 
     if (!(vm = virDomainAssignDef(conn,
                                   &driver->domains,
@@ -2170,22 +2438,25 @@ static int qemudDomainRestore(virConnect
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to assign new VM"));
         virDomainDefFree(def);
-        close(fd);
-        return -1;
+        goto cleanup;
     }
 
     /* Set the migration source and start it up. */
     vm->stdin_fd = fd;
     ret = qemudStartVMDaemon(conn, driver, vm, "stdio");
     close(fd);
+    fd = -1;
     vm->stdin_fd = -1;
     if (ret < 0) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("failed to start VM"));
-        if (!vm->persistent)
+        if (!vm->persistent) {
+            virDomainUnlock(vm);
             virDomainRemoveInactive(&driver->domains,
                                     vm);
-        return -1;
+            vm = NULL;
+        }
+        goto cleanup;
     }
 
     /* If it was running before, resume it now. */
@@ -2194,30 +2465,57 @@ static int qemudDomainRestore(virConnect
         if (qemudMonitorCommand(vm, "cont", &info) < 0) {
             qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
                              "%s", _("failed to resume domain"));
-            return -1;
+            goto cleanup;
         }
         VIR_FREE(info);
         vm->state = VIR_DOMAIN_RUNNING;
     }
 
+    virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
     return 0;
+
+cleanup:
+    if (fd != -1)
+        close(fd);
+    VIR_FREE(xml);
+    if (vm)
+        virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
+    return -1;
 }
 
 
 static char *qemudDomainDumpXML(virDomainPtr dom,
                                 int flags ATTRIBUTE_UNUSED) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+    char *ret;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
+
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
-        return NULL;
+        goto cleanup;
     }
 
-    return virDomainDefFormat(dom->conn,
-                              (flags & VIR_DOMAIN_XML_INACTIVE) &&
vm->newDef ?
-                              vm->newDef : vm->def,
-                              flags);
+    ret = virDomainDefFormat(dom->conn,
+                             (flags & VIR_DOMAIN_XML_INACTIVE) &&
vm->newDef ?
+                             vm->newDef : vm->def,
+                             flags);
+    if (!ret)
+        goto cleanup;
+
+    virDomainUnlock(vm);
+    return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return NULL;
 }
 
 
@@ -2226,15 +2524,20 @@ static int qemudListDefinedDomains(virCo
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     int got = 0, i;
 
+    qemudDriverLock(driver);
     for (i = 0 ; i < driver->domains.count && got < nnames ; i++) {
+        virDomainLock(driver->domains.objs[i]);
         if (!virDomainIsActive(driver->domains.objs[i])) {
             if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) {
                 qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY,
                                  "%s", _("failed to allocate space for VM
name string"));
+                virDomainUnlock(driver->domains.objs[i]);
                 goto cleanup;
             }
         }
+        virDomainUnlock(driver->domains.objs[i]);
     }
+    qemudDriverUnlock(driver);
 
     return got;
 
@@ -2248,9 +2551,14 @@ static int qemudNumDefinedDomains(virCon
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     int n = 0, i;
 
-    for (i = 0 ; i < driver->domains.count ; i++)
+    qemudDriverLock(driver);
+    for (i = 0 ; i < driver->domains.count ; i++) {
+        virDomainLock(driver->domains.objs[i]);
         if (!virDomainIsActive(driver->domains.objs[i]))
             n++;
+        virDomainUnlock(driver->domains.objs[i]);
+    }
+    qemudDriverUnlock(driver);
 
     return n;
 }
@@ -2258,77 +2566,110 @@ static int qemudNumDefinedDomains(virCon
 
 static int qemudDomainStart(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+    int ret;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
+        qemudDriverUnlock(driver);
         return -1;
     }
 
-    return qemudStartVMDaemon(dom->conn, driver, vm, NULL);
+    ret = qemudStartVMDaemon(dom->conn, driver, vm, NULL);
+    virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
+    return ret;
 }
 
 
 static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) {
     struct qemud_driver *driver = (struct qemud_driver *)conn->privateData;
     virDomainDefPtr def;
-    virDomainObjPtr vm;
+    virDomainObjPtr vm = NULL;
     virDomainPtr dom;
 
+    qemudDriverLock(driver);
+
     if (!(def = virDomainDefParseString(conn, driver->caps, xml)))
-        return NULL;
+        goto cleanup;
 
     if (!(vm = virDomainAssignDef(conn,
                                   &driver->domains,
                                   def))) {
         virDomainDefFree(def);
-        return NULL;
+        goto cleanup;
     }
     vm->persistent = 1;
 
     if (virDomainSaveConfig(conn,
                             driver->configDir,
                             vm->newDef ? vm->newDef : vm->def) < 0) {
+        virDomainUnlock(vm);
         virDomainRemoveInactive(&driver->domains,
                                 vm);
-        return NULL;
+        vm = NULL;
+        goto cleanup;
     }
 
     dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
     if (dom) dom->id = vm->def->id;
+
+    virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
     return dom;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    qemudDriverUnlock(driver);
+    return NULL;
 }
 
 static int qemudDomainUndefine(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm = NULL;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
-        return -1;
+        goto cleanup;
     }
 
     if (virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                          "%s", _("cannot delete active domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (!vm->persistent) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                          "%s", _("cannot undefine transient
domain"));
-        return -1;
+        goto cleanup;
     }
 
     if (virDomainDeleteConfig(dom->conn, driver->configDir,
driver->autostartDir, vm) < 0)
-        return -1;
+        goto cleanup;
 
+    virDomainUnlock(vm);
     virDomainRemoveInactive(&driver->domains,
                             vm);
 
+    qemudDriverUnlock(driver);
+
     return 0;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    qemudDriverLock(driver);
+    return -1;
 }
 
 /* Return the disks name for use in monitor commands */
@@ -2681,9 +3022,13 @@ static int qemudDomainAttachDevice(virDo
 static int qemudDomainAttachDevice(virDomainPtr dom,
                                    const char *xml) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     virDomainDeviceDefPtr dev;
     int ret = 0, supported = 0;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2694,13 +3039,11 @@ static int qemudDomainAttachDevice(virDo
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                          "%s", _("cannot attach device on inactive
domain"));
-        return -1;
+        goto cleanup;
     }
 
-    dev = virDomainDeviceDefParse(dom->conn, vm->def, xml);
-    if (dev == NULL) {
-        return -1;
-    }
+    if (!(dev = virDomainDeviceDefParse(dom->conn, vm->def, xml)))
+        goto cleanup;
 
     if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
         switch (dev->data.disk->device) {
@@ -2723,8 +3066,8 @@ static int qemudDomainAttachDevice(virDo
     } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
         dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
         dev->data.hostdev->source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
-                supported = 1;
-                ret = qemudDomainAttachHostDevice(dom, dev);
+        supported = 1;
+        ret = qemudDomainAttachHostDevice(dom, dev);
     }
 
     if (!supported) {
@@ -2734,13 +3077,24 @@ static int qemudDomainAttachDevice(virDo
     }
 
     VIR_FREE(dev);
+
+    virDomainUnlock(vm);
     return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int qemudDomainGetAutostart(virDomainPtr dom,
                             int *autostart) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2750,71 +3104,80 @@ static int qemudDomainGetAutostart(virDo
 
     *autostart = vm->autostart;
 
+    virDomainUnlock(vm);
     return 0;
 }
 
 static int qemudDomainSetAutostart(virDomainPtr dom,
                                    int autostart) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     char *configFile = NULL, *autostartLink = NULL;
-    int ret = -1;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                          "%s", _("no domain with matching uuid"));
-        return -1;
+        goto cleanup;
     }
 
     if (!vm->persistent) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
                          "%s", _("cannot set autostart for transient
domain"));
-        return -1;
+        goto cleanup;
     }
 
     autostart = (autostart != 0);
 
-    if (vm->autostart == autostart)
-        return 0;
+    if (vm->autostart != autostart) {
+        if ((configFile = virDomainConfigFile(dom->conn, driver->configDir,
vm->def->name)) == NULL)
+            goto cleanup;
+        if ((autostartLink = virDomainConfigFile(dom->conn, driver->autostartDir,
vm->def->name)) == NULL)
+            goto cleanup;
 
-    if ((configFile = virDomainConfigFile(dom->conn, driver->configDir,
vm->def->name)) == NULL)
-        goto cleanup;
-    if ((autostartLink = virDomainConfigFile(dom->conn, driver->autostartDir,
vm->def->name)) == NULL)
-        goto cleanup;
+        if (autostart) {
+            int err;
 
-    if (autostart) {
-        int err;
+            if ((err = virFileMakePath(driver->autostartDir))) {
+                qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+                                 _("cannot create autostart directory %s:
%s"),
+                                 driver->autostartDir, strerror(err));
+                goto cleanup;
+            }
 
-        if ((err = virFileMakePath(driver->autostartDir))) {
-            qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
-                             _("cannot create autostart directory %s: %s"),
-                             driver->autostartDir, strerror(err));
-            goto cleanup;
+            if (symlink(configFile, autostartLink) < 0) {
+                qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+                                 _("Failed to create symlink '%s to
'%s': %s"),
+                                 autostartLink, configFile, strerror(errno));
+                goto cleanup;
+            }
+        } else {
+            if (unlink(autostartLink) < 0 && errno != ENOENT && errno
!= ENOTDIR) {
+                qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+                                 _("Failed to delete symlink '%s':
%s"),
+                                 autostartLink, strerror(errno));
+                goto cleanup;
+            }
         }
 
-        if (symlink(configFile, autostartLink) < 0) {
-            qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
-                             _("Failed to create symlink '%s to '%s':
%s"),
-                             autostartLink, configFile, strerror(errno));
-            goto cleanup;
-        }
-    } else {
-        if (unlink(autostartLink) < 0 && errno != ENOENT && errno !=
ENOTDIR) {
-            qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
-                             _("Failed to delete symlink '%s': %s"),
-                             autostartLink, strerror(errno));
-            goto cleanup;
-        }
+        vm->autostart = autostart;
     }
 
-    vm->autostart = autostart;
-    ret = 0;
+    VIR_FREE(configFile);
+    VIR_FREE(autostartLink);
+    virDomainUnlock(vm);
+    return 0;
 
 cleanup:
+    if (vm)
+        virDomainUnlock(vm);
     VIR_FREE(configFile);
     VIR_FREE(autostartLink);
 
-    return ret;
+    return -1;
 }
 
 /* This uses the 'info blockstats' monitor command which was
@@ -2833,8 +3196,12 @@ qemudDomainBlockStats (virDomainPtr dom,
     const char *qemu_dev_name = NULL;
     size_t len;
     int i, ret = -1;
-    const virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    virDomainObjPtr vm;
     virDomainDiskDefPtr disk = NULL;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -2844,7 +3211,7 @@ qemudDomainBlockStats (virDomainPtr dom,
     if (!virDomainIsActive (vm)) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                           "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
 
     for (i = 0 ; i < vm->def->ndisks ; i++) {
@@ -2857,7 +3224,7 @@ qemudDomainBlockStats (virDomainPtr dom,
     if (!disk) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                           _("invalid path: %s"), path);
-        return -1;
+        goto cleanup;
     }
 
     qemu_dev_name = qemudDiskDeviceName(dom, disk);
@@ -2868,7 +3235,7 @@ qemudDomainBlockStats (virDomainPtr dom,
     if (qemudMonitorCommand (vm, "info blockstats", &info) < 0) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                           "%s", _("'info blockstats' command
failed"));
-        goto out;
+        goto cleanup;
     }
     DEBUG ("info blockstats reply: %s", info);
 
@@ -2881,7 +3248,7 @@ qemudDomainBlockStats (virDomainPtr dom,
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                           "%s",
                           _("'info blockstats' not supported by this
qemu"));
-        goto out;
+        goto cleanup;
     }
 
     stats->rd_req = -1;
@@ -2933,7 +3300,7 @@ qemudDomainBlockStats (virDomainPtr dom,
                 p++;
             }
             ret = 0;
-            goto out;
+            goto done;
         }
 
         /* Skip to next line. */
@@ -2945,10 +3312,20 @@ qemudDomainBlockStats (virDomainPtr dom,
     /* If we reach here then the device was not found. */
     qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                       _("device not found: %s (%s)"), path, qemu_dev_name);
- out:
+    goto cleanup;
+
+  done:
     VIR_FREE(qemu_dev_name);
     VIR_FREE(info);
-    return ret;
+    virDomainUnlock(vm);
+    return 0;
+
+cleanup:
+    VIR_FREE(qemu_dev_name);
+    VIR_FREE(info);
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int
@@ -2958,25 +3335,29 @@ qemudDomainInterfaceStats (virDomainPtr 
 {
 #ifdef __linux__
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
-    int i;
+    virDomainObjPtr vm;
+    int i, ret;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                           _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
 
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
 
     if (!path || path[0] == '\0') {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s", _("NULL or empty path"));
-        return -1;
+        goto cleanup;
     }
 
     /* Check the path is one of the domain's network interfaces. */
@@ -2988,10 +3369,17 @@ qemudDomainInterfaceStats (virDomainPtr 
 
     qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                       _("invalid path, '%s' is not a known interface"),
path);
+    goto cleanup;
+
+ ok:
+    ret = linuxDomainInterfaceStats (dom->conn, path, stats);
+    virDomainUnlock(vm);
+    return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
     return -1;
- ok:
-
-    return linuxDomainInterfaceStats (dom->conn, path, stats);
 #else
     qemudReportError (dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                       "%s", __FUNCTION__);
@@ -3007,19 +3395,23 @@ qemudDomainBlockPeek (virDomainPtr dom,
                       unsigned int flags ATTRIBUTE_UNUSED)
 {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    virDomainObjPtr vm;
     int fd, ret = -1, i;
+
+    qemudDriverLock(driver);
+    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+    qemudDriverUnlock(driver);
 
     if (!vm) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                           "%s", _("no domain with matching uuid"));
-        return -1;
+        goto cleanup;
     }
 
     if (!path || path[0] == '\0') {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                          "%s", _("NULL or empty path"));
-        return -1;
+        goto cleanup;
     }
 
     /* Check the path belongs to this domain. */
@@ -3030,7 +3422,7 @@ qemudDomainBlockPeek (virDomainPtr dom,
     }
     qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                       "%s", _("invalid path"));
-    return -1;
+    goto cleanup;
 
 found:
     /* The path is correct, now try to open it and get its size. */
@@ -3055,7 +3447,14 @@ found:
     ret = 0;
  done:
     if (fd >= 0) close (fd);
+
+    virDomainUnlock(vm);
     return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 static int
@@ -3065,34 +3464,38 @@ qemudDomainMemoryPeek (virDomainPtr dom,
                        unsigned int flags)
 {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
-    virDomainObjPtr vm = virDomainFindByID(&driver->domains, dom->id);
+    virDomainObjPtr vm;
     char cmd[256], *info;
     char tmp[] = TEMPDIR "/qemu.mem.XXXXXX";
     int fd = -1, ret = -1;
 
+    qemudDriverLock(driver);
+    vm = virDomainFindByID(&driver->domains, dom->id);
+    qemudDriverUnlock(driver);
+
     if (flags != VIR_MEMORY_VIRTUAL) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
                           "%s", _("QEMU driver only supports virtual
memory addrs"));
-        return -1;
+        goto cleanup;
     }
 
     if (!vm) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
                           _("no domain with matching id %d"), dom->id);
-        return -1;
+        goto cleanup;
     }
 
     if (!virDomainIsActive(vm)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                          "%s", _("domain is not running"));
-        return -1;
+        goto cleanup;
     }
 
     /* Create a temporary filename. */
     if ((fd = mkstemp (tmp)) == -1) {
         qemudReportError (dom->conn, dom, NULL, VIR_ERR_SYSTEM_ERROR,
                           "%s", strerror (errno));
-        return -1;
+        goto cleanup;
     }
 
     /* Issue the memsave command. */
@@ -3104,7 +3507,7 @@ qemudDomainMemoryPeek (virDomainPtr dom,
     }
 
     DEBUG ("memsave reply: %s", info);
-    free (info);
+    VIR_FREE(info);
 
     /* Read the memory file into buffer. */
     if (saferead (fd, buffer, size) == (ssize_t) -1) {
@@ -3117,7 +3520,13 @@ done:
 done:
     if (fd >= 0) close (fd);
     unlink (tmp);
+    virDomainUnlock(vm);
     return ret;
+
+cleanup:
+    if (vm)
+        virDomainUnlock(vm);
+    return -1;
 }
 
 


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

Re: [libvirt] PATCH: 3/5: Locking in the QEMU driver