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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list