3:22 p.m.
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.
---
Notes:
Version 2:
- merge initializers
- replace virQEMUDriverPtr with virQEMUCloseCallbacksPtr in all
virQEMUCloseCallbacks* APIs
src/qemu/qemu_conf.c | 158 +++++++++++++++++++++++++++++-----------------
src/qemu/qemu_conf.h | 41 ++++++------
src/qemu/qemu_driver.c | 16 +++--
src/qemu/qemu_migration.c | 7 +-
src/qemu/qemu_process.c | 11 ++--
5 files changed, 140 insertions(+), 93 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4ff912d..f3c3cf3 100644
--- 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);
}
/* Construct the hash key for sharedDisks as "major:minor" */
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index ea4f393..c6bc883 100644
--- 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);
int qemuAddSharedDisk(virQEMUDriverPtr driver,
const char *disk_path)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc35b91..76fbb51 100644
--- 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 */
@@ -958,7 +958,7 @@ qemuShutdown(void) {
virSysinfoDefFree(qemu_driver->hostsysinfo);
- qemuDriverCloseCallbackShutdown(qemu_driver);
+ virObjectUnref(qemu_driver->closeCallbacks);
VIR_FREE(qemu_driver->qemuImgBinary);
@@ -1052,11 +1052,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;
@@ -9630,8 +9631,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;
@@ -9857,7 +9858,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 36e55d2..8485b20 100644
--- 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 67362af..0fcc14f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4667,22 +4667,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;
}
--
1.8.1.2