Re: [libvirt] [PATCH 1/2] qemu: Turn closeCallbacks into virObjectLockable

19 Feb 2013

On Mon, Feb 18, 2013 at 05:07:44PM +0100, Jiri Denemark wrote:
...
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.
Since you're turning this into a full object, I think it would be
nice to move it to a standalone file, so it can be reused by the
LXC driver, which currently re-invents this same kind of code.
...
---
 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)
No need for two initializers, just make the virQEMUConfigOnceInit
method create both classes.
...
@@ -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,
I was rather expecting to see the virQEMUDriverPtr replacd
with a virQEMUCloseCallbacksPtr in all the methods now it
becomes a full object.
...
+                             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)
ACK to the general approach, but I think there's a few tweaks needed
still.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

    

Re: [libvirt] [PATCH 1/2] qemu: Turn closeCallbacks into virObjectLockable

Daniel P. Berrange