10:07 a.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.
---
src/qemu/qemu_conf.c | 155 ++++++++++++++++++++++++++++++----------------
src/qemu/qemu_conf.h | 39 ++++++------
src/qemu/qemu_driver.c | 12 ++--
src/qemu/qemu_migration.c | 6 +-
src/qemu/qemu_process.c | 10 +--
5 files changed, 136 insertions(+), 86 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4ff912d..edadf36 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -57,8 +57,25 @@
#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)
{
@@ -71,13 +88,21 @@ static int virQEMUConfigOnceInit(void)
return 0;
}
-VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
+static int
+virQEMUCloseCallbacksOnceInit(void)
+{
+ virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(),
+ "virQEMUCloseCallbacks",
+ sizeof(virQEMUCloseCallbacks),
+ virQEMUCloseCallbacksDispose);
+ if (!virQEMUCloseCallbacksClass)
+ return -1;
+ return 0;
+}
+VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
+VIR_ONCE_GLOBAL_INIT(virQEMUCloseCallbacks)
-struct _qemuDriverCloseDef {
- virConnectPtr conn;
- qemuDriverCloseCallback cb;
-};
static void
qemuDriverLock(virQEMUDriverPtr driver)
@@ -639,44 +664,59 @@ 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 (virQEMUCloseCallbacksInitialize() < 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(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virConnectPtr conn,
+ virQEMUCloseCallback cb)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
qemuDriverCloseDefPtr closeDef;
+ virHashTablePtr list;
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(driver->closeCallbacks);
+
+ list = driver->closeCallbacks->list;
+ closeDef = virHashLookup(list, uuidstr);
if (closeDef) {
if (closeDef->conn != conn) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -701,7 +741,7 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
closeDef->conn = conn;
closeDef->cb = cb;
- if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) {
+ if (virHashAddEntry(list, uuidstr, closeDef) < 0) {
VIR_FREE(closeDef);
goto cleanup;
}
@@ -709,25 +749,28 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
ret = 0;
cleanup:
- qemuDriverUnlock(driver);
+ virObjectUnlock(driver->closeCallbacks);
return ret;
}
int
-qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDriverCloseCallback cb)
+virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virQEMUCloseCallback cb)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
qemuDriverCloseDefPtr closeDef;
+ virHashTablePtr list;
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(driver->closeCallbacks);
+
+ list = driver->closeCallbacks->list;
+ closeDef = virHashLookup(list, uuidstr);
if (!closeDef)
goto cleanup;
@@ -738,46 +781,49 @@ qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
goto cleanup;
}
- ret = virHashRemoveEntry(driver->closeCallbacks, uuidstr);
+ ret = virHashRemoveEntry(list, uuidstr);
cleanup:
- qemuDriverUnlock(driver);
+ virObjectUnlock(driver->closeCallbacks);
return ret;
}
-qemuDriverCloseCallback
-qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- virConnectPtr conn)
+virQEMUCloseCallback
+virQEMUCloseCallbacksGet(virQEMUDriverPtr driver,
+ 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(driver->closeCallbacks);
+
+ closeDef = virHashLookup(driver->closeCallbacks->list, uuidstr);
if (closeDef && (!conn || closeDef->conn == conn))
cb = closeDef->cb;
+ virObjectUnlock(driver->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 +854,25 @@ qemuDriverCloseCallbackRun(void *payload,
if (dom)
virObjectUnlock(dom);
- virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);
+ virHashRemoveEntry(data->list, uuidstr);
}
void
-qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
- virConnectPtr conn)
+virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
+ virConnectPtr conn)
{
- 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(driver->closeCallbacks);
+
+ data.list = driver->closeCallbacks->list;
+ virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data);
+
+ virObjectUnlock(driver->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 14f1427..72380dc 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,22 @@ 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(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virConnectPtr conn,
+ virQEMUCloseCallback cb);
+int virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
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(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virConnectPtr conn);
+void virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
+ virConnectPtr conn);
int qemuAddSharedDisk(virQEMUDriverPtr driver,
const char *disk_path)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 23499ef..257728d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -753,7 +753,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 */
@@ -954,7 +954,7 @@ qemuShutdown(void) {
virSysinfoDefFree(qemu_driver->hostsysinfo);
- qemuDriverCloseCallbackShutdown(qemu_driver);
+ virObjectUnref(qemu_driver->closeCallbacks);
VIR_FREE(qemu_driver->qemuImgBinary);
@@ -1052,7 +1052,7 @@ static int qemuClose(virConnectPtr conn) {
virQEMUDriverPtr driver = conn->privateData;
/* Get rid of callbacks registered for this conn */
- qemuDriverCloseCallbackRunAll(driver, conn);
+ virQEMUCloseCallbacksRun(driver, conn);
conn->privateData = NULL;
@@ -9626,8 +9626,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, vm, domain->conn,
+ qemuMigrationCleanup) < 0)
goto endjob;
if (qemuMigrationJobContinue(vm) == 0) {
vm = NULL;
@@ -9853,7 +9853,7 @@ qemuDomainMigrateConfirm3(virDomainPtr domain,
phase = QEMU_MIGRATION_PHASE_CONFIRM3;
qemuMigrationJobStartPhase(driver, vm, phase);
- qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+ virQEMUCloseCallbacksUnset(driver, 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..11d7d6c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3155,7 +3155,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
}
qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3);
- qemuDriverCloseCallbackUnset(driver, vm, qemuMigrationCleanup);
+ virQEMUCloseCallbacksUnset(driver, vm, qemuMigrationCleanup);
resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen,
@@ -3186,8 +3186,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE);
- if (qemuDriverCloseCallbackSet(driver, vm, conn,
- qemuMigrationCleanup) < 0)
+ if (virQEMUCloseCallbacksSet(driver, vm, conn,
+ qemuMigrationCleanup) < 0)
goto endjob;
endjob:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4251c34..f987618 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4670,22 +4670,22 @@ 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, vm, conn,
+ qemuProcessAutoDestroy);
}
int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver,
virDomainObjPtr vm)
{
VIR_DEBUG("vm=%s", vm->def->name);
- return qemuDriverCloseCallbackUnset(driver, vm, qemuProcessAutoDestroy);
+ return virQEMUCloseCallbacksUnset(driver, 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, vm, NULL);
return cb == qemuProcessAutoDestroy;
}
--
1.8.1.2