qemu: Turn closeCallbacks into virObjectLockable

To avoid having to hold the qemu driver lock while iterating through
close callbacks and calling them. This fixes a real deadlock when a
domain which is being migrated from another host gets autodestoyed as a
result of broken connection to the other host.

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 33fd67d9b28cbfb1c585a6a34c4250d1006823a4..82245e27de5d2a65b7a2e16c03fb8a8fd71c8f70 100644 (file)
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -57,28 +57,47 @@
 
 #define VIR_FROM_THIS VIR_FROM_QEMU
 
+typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
+typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
+struct _qemuDriverCloseDef {
+    virConnectPtr conn;
+    virQEMUCloseCallback cb;
+};
+
+struct _virQEMUCloseCallbacks {
+    virObjectLockable parent;
+
+    /* UUID string to qemuDriverCloseDef mapping */
+    virHashTablePtr list;
+};
+
+
 static virClassPtr virQEMUDriverConfigClass;
+static virClassPtr virQEMUCloseCallbacksClass;
 static void virQEMUDriverConfigDispose(void *obj);
+static void virQEMUCloseCallbacksDispose(void *obj);
 
 static int virQEMUConfigOnceInit(void)
 {
-    if (!(virQEMUDriverConfigClass = virClassNew(virClassForObject(),
-                                                 "virQEMUDriverConfig",
-                                                 sizeof(virQEMUDriverConfig),
-                                                 virQEMUDriverConfigDispose)))
-        return -1;
+    virQEMUDriverConfigClass = virClassNew(virClassForObject(),
+                                           "virQEMUDriverConfig",
+                                           sizeof(virQEMUDriverConfig),
+                                           virQEMUDriverConfigDispose);
 
-    return 0;
+    virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(),
+                                             "virQEMUCloseCallbacks",
+                                             sizeof(virQEMUCloseCallbacks),
+                                             virQEMUCloseCallbacksDispose);
+
+    if (!virQEMUDriverConfigClass || !virQEMUCloseCallbacksClass)
+        return -1;
+    else
+        return 0;
 }
 
 VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
 
 
-struct _qemuDriverCloseDef {
-    virConnectPtr conn;
-    qemuDriverCloseCallback cb;
-};
-
 static void
 qemuDriverLock(virQEMUDriverPtr driver)
 {
@@ -639,44 +658,57 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
 
 
 static void
-qemuDriverCloseCallbackFree(void *payload,
-                            const void *name ATTRIBUTE_UNUSED)
+virQEMUCloseCallbacksFreeData(void *payload,
+                              const void *name ATTRIBUTE_UNUSED)
 {
     VIR_FREE(payload);
 }
 
-int
-qemuDriverCloseCallbackInit(virQEMUDriverPtr driver)
+virQEMUCloseCallbacksPtr
+virQEMUCloseCallbacksNew(void)
 {
-    driver->closeCallbacks = virHashCreate(5, qemuDriverCloseCallbackFree);
-    if (!driver->closeCallbacks)
-        return -1;
+    virQEMUCloseCallbacksPtr closeCallbacks;
 
-    return 0;
+    if (virQEMUConfigInitialize() < 0)
+        return NULL;
+
+    if (!(closeCallbacks = virObjectLockableNew(virQEMUCloseCallbacksClass)))
+        return NULL;
+
+    closeCallbacks->list = virHashCreate(5, virQEMUCloseCallbacksFreeData);
+    if (!closeCallbacks->list) {
+        virObjectUnref(closeCallbacks);
+        return NULL;
+    }
+
+    return closeCallbacks;
 }
 
-void
-qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver)
+static void
+virQEMUCloseCallbacksDispose(void *obj)
 {
-    virHashFree(driver->closeCallbacks);
+    virQEMUCloseCallbacksPtr closeCallbacks = obj;
+
+    virHashFree(closeCallbacks->list);
 }
 
 int
-qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
-                           virDomainObjPtr vm,
-                           virConnectPtr conn,
-                           qemuDriverCloseCallback cb)
+virQEMUCloseCallbacksSet(virQEMUCloseCallbacksPtr closeCallbacks,
+                         virDomainObjPtr vm,
+                         virConnectPtr conn,
+                         virQEMUCloseCallback cb)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
     int ret = -1;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p",
               vm->def->name, uuidstr, conn, cb);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(closeCallbacks);
+
+    closeDef = virHashLookup(closeCallbacks->list, uuidstr);
     if (closeDef) {
         if (closeDef->conn != conn) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -701,7 +733,7 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
 
         closeDef->conn = conn;
         closeDef->cb = cb;
-        if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) {
+        if (virHashAddEntry(closeCallbacks->list, uuidstr, closeDef) < 0) {
             VIR_FREE(closeDef);
             goto cleanup;
         }
@@ -709,25 +741,26 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
 
     ret = 0;
 cleanup:
-    qemuDriverUnlock(driver);
+    virObjectUnlock(closeCallbacks);
     return ret;
 }
 
 int
-qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
-                             virDomainObjPtr vm,
-                             qemuDriverCloseCallback cb)
+virQEMUCloseCallbacksUnset(virQEMUCloseCallbacksPtr closeCallbacks,
+                           virDomainObjPtr vm,
+                           virQEMUCloseCallback cb)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
     int ret = -1;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, cb=%p",
               vm->def->name, uuidstr, cb);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(closeCallbacks);
+
+    closeDef = virHashLookup(closeCallbacks->list, uuidstr);
     if (!closeDef)
         goto cleanup;
 
@@ -738,46 +771,49 @@ qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    ret = virHashRemoveEntry(driver->closeCallbacks, uuidstr);
+    ret = virHashRemoveEntry(closeCallbacks->list, uuidstr);
 cleanup:
-    qemuDriverUnlock(driver);
+    virObjectUnlock(closeCallbacks);
     return ret;
 }
 
-qemuDriverCloseCallback
-qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
-                           virDomainObjPtr vm,
-                           virConnectPtr conn)
+virQEMUCloseCallback
+virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks,
+                         virDomainObjPtr vm,
+                         virConnectPtr conn)
 {
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     qemuDriverCloseDefPtr closeDef;
-    qemuDriverCloseCallback cb = NULL;
+    virQEMUCloseCallback cb = NULL;
 
-    qemuDriverLock(driver);
     virUUIDFormat(vm->def->uuid, uuidstr);
     VIR_DEBUG("vm=%s, uuid=%s, conn=%p",
               vm->def->name, uuidstr, conn);
 
-    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
+    virObjectLock(closeCallbacks);
+
+    closeDef = virHashLookup(closeCallbacks->list, uuidstr);
     if (closeDef && (!conn || closeDef->conn == conn))
         cb = closeDef->cb;
 
+    virObjectUnlock(closeCallbacks);
+
     VIR_DEBUG("cb=%p", cb);
-    qemuDriverUnlock(driver);
     return cb;
 }
 
-struct qemuDriverCloseCallbackData {
+struct virQEMUCloseCallbacksData {
+    virHashTablePtr list;
     virQEMUDriverPtr driver;
     virConnectPtr conn;
 };
 
 static void
-qemuDriverCloseCallbackRun(void *payload,
-                           const void *name,
-                           void *opaque)
+virQEMUCloseCallbacksRunOne(void *payload,
+                            const void *name,
+                            void *opaque)
 {
-    struct qemuDriverCloseCallbackData *data = opaque;
+    struct virQEMUCloseCallbacksData *data = opaque;
     qemuDriverCloseDefPtr closeDef = payload;
     unsigned char uuid[VIR_UUID_BUFLEN];
     char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -808,20 +844,26 @@ qemuDriverCloseCallbackRun(void *payload,
     if (dom)
         virObjectUnlock(dom);
 
-    virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);
+    virHashRemoveEntry(data->list, uuidstr);
 }
 
 void
-qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
-                              virConnectPtr conn)
+virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks,
+                         virConnectPtr conn,
+                         virQEMUDriverPtr driver)
 {
-    struct qemuDriverCloseCallbackData data = {
-        driver, conn
+    struct virQEMUCloseCallbacksData data = {
+        .driver = driver,
+        .conn = conn
     };
+
     VIR_DEBUG("conn=%p", conn);
-    qemuDriverLock(driver);
-    virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data);
-    qemuDriverUnlock(driver);
+    virObjectLock(closeCallbacks);
+
+    data.list = closeCallbacks->list;
+    virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data);
+
+    virObjectUnlock(closeCallbacks);
 }
 
 struct _qemuSharedDiskEntry {

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 87d8b77cd4b78038e484fc1a43b6565b5a7a334d..6bb3dee8c3e84ce0ac96a44134b6dfe6d66424e1 100644 (file)
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -47,8 +47,8 @@
 
 # define QEMUD_CPUMASK_LEN CPU_SETSIZE
 
-typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
-typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
+typedef struct _virQEMUCloseCallbacks virQEMUCloseCallbacks;
+typedef virQEMUCloseCallbacks *virQEMUCloseCallbacksPtr;
 
 typedef struct _virQEMUDriver virQEMUDriver;
 typedef virQEMUDriver *virQEMUDriverPtr;
@@ -216,8 +216,8 @@ struct _virQEMUDriver {
     /* Immutable pointer. lockless access */
     virLockManagerPluginPtr lockManager;
 
-    /* Immutable pointer. Unsafe APIs. XXX */
-    virHashTablePtr closeCallbacks;
+    /* Immutable pointer, self-clocking APIs */
+    virQEMUCloseCallbacksPtr closeCallbacks;
 };
 
 typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
@@ -254,23 +254,24 @@ struct qemuDomainDiskInfo {
     int io_status;
 };
 
-typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver,
-                                                   virDomainObjPtr vm,
-                                                   virConnectPtr conn);
-int qemuDriverCloseCallbackInit(virQEMUDriverPtr driver);
-void qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver);
-int qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
+typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver,
+                                                virDomainObjPtr vm,
+                                                virConnectPtr conn);
+virQEMUCloseCallbacksPtr virQEMUCloseCallbacksNew(void);
+int virQEMUCloseCallbacksSet(virQEMUCloseCallbacksPtr closeCallbacks,
+                             virDomainObjPtr vm,
+                             virConnectPtr conn,
+                             virQEMUCloseCallback cb);
+int virQEMUCloseCallbacksUnset(virQEMUCloseCallbacksPtr closeCallbacks,
                                virDomainObjPtr vm,
-                               virConnectPtr conn,
-                               qemuDriverCloseCallback cb);
-int qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
-                                 virDomainObjPtr vm,
-                                 qemuDriverCloseCallback cb);
-qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
-                                                   virDomainObjPtr vm,
-                                                   virConnectPtr conn);
-void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
-                                   virConnectPtr conn);
+                               virQEMUCloseCallback cb);
+virQEMUCloseCallback
+virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks,
+                         virDomainObjPtr vm,
+                         virConnectPtr conn);
+void virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks,
+                              virConnectPtr conn,
+                              virQEMUDriverPtr driver);
 
 typedef struct _qemuSharedDiskEntry qemuSharedDiskEntry;
 typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e4ace3012541c73e98b357a9f01c6a7615fe9879..76b280003597a10ecce68ef0e9638d196adbb9e7 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -757,7 +757,7 @@ qemuStartup(bool privileged,
         cfg->hugepagePath = mempath;
     }
 
-    if (qemuDriverCloseCallbackInit(qemu_driver) < 0)
+    if (!(qemu_driver->closeCallbacks = virQEMUCloseCallbacksNew()))
         goto error;
 
     /* Get all the running persistent or transient configs first */
@@ -955,7 +955,7 @@ qemuShutdown(void) {
 
     virSysinfoDefFree(qemu_driver->hostsysinfo);
 
-    qemuDriverCloseCallbackShutdown(qemu_driver);
+    virObjectUnref(qemu_driver->closeCallbacks);
 
     VIR_FREE(qemu_driver->qemuImgBinary);
 
@@ -1049,11 +1049,12 @@ cleanup:
     return ret;
 }
 
-static int qemuClose(virConnectPtr conn) {
+static int qemuClose(virConnectPtr conn)
+{
     virQEMUDriverPtr driver = conn->privateData;
 
     /* Get rid of callbacks registered for this conn */
-    qemuDriverCloseCallbackRunAll(driver, conn);
+    virQEMUCloseCallbacksRun(driver->closeCallbacks, conn, driver);
 
     conn->privateData = NULL;
 
@@ -9702,8 +9703,8 @@ qemuDomainMigrateBegin3(virDomainPtr domain,
          * This prevents any other APIs being invoked while migration is taking
          * place.
          */
-        if (qemuDriverCloseCallbackSet(driver, vm, domain->conn,
-                                       qemuMigrationCleanup) < 0)
+        if (virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, domain->conn,
+                                     qemuMigrationCleanup) < 0)
             goto endjob;
         if (qemuMigrationJobContinue(vm) == 0) {
             vm = NULL;
@@ -9929,7 +9930,8 @@ qemuDomainMigrateConfirm3(virDomainPtr domain,
         phase = QEMU_MIGRATION_PHASE_CONFIRM3;
 
     qemuMigrationJobStartPhase(driver, vm, phase);
-    qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+    virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm,
+                               qemuMigrationCleanup);
 
     ret = qemuMigrationConfirm(driver, domain->conn, vm,
                                cookiein, cookieinlen,

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2f7935828295d932a82db43e1ca47394ab25ad93..0ddb5077cf1f2ea4171b311966e15dea54feb7ca 100644 (file)
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3155,7 +3155,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
     }
 
     qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3);
-    qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+    virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm,
+                               qemuMigrationCleanup);
 
     resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
     ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen,
@@ -3186,8 +3187,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
 
     qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE);
 
-    if (qemuDriverCloseCallbackSet(driver, vm, conn,
-                                   qemuMigrationCleanup) < 0)
+    if (virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn,
+                                 qemuMigrationCleanup) < 0)
         goto endjob;
 
 endjob:

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 476e3ed2d56a9b4cad0b98a173d8629c39b2db64..994417bb7ede9d35b184cfcec13980c321fe90b2 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4626,22 +4626,23 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
                               virConnectPtr conn)
 {
     VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
-    return qemuDriverCloseCallbackSet(driver, vm, conn,
-                                      qemuProcessAutoDestroy);
+    return virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn,
+                                    qemuProcessAutoDestroy);
 }
 
 int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver,
                                  virDomainObjPtr vm)
 {
     VIR_DEBUG("vm=%s", vm->def->name);
-    return qemuDriverCloseCallbackUnset(driver, vm, qemuProcessAutoDestroy);
+    return virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm,
+                                      qemuProcessAutoDestroy);
 }
 
 bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver,
                                   virDomainObjPtr vm)
 {
-    qemuDriverCloseCallback cb;
+    virQEMUCloseCallback cb;
     VIR_DEBUG("vm=%s", vm->def->name);
-    cb = qemuDriverCloseCallbackGet(driver, vm, NULL);
+    cb = virQEMUCloseCallbacksGet(driver->closeCallbacks, vm, NULL);
     return cb == qemuProcessAutoDestroy;
 }