[libvirt] [PATCH 0/9] Fine grained locking in LXC driver

The first round of my patches that breaks LXC driver into smaller pieces. With my testing on 10 domains which are started up and destroyed in a loop with 10 iterations (each loop is run in a separate thread in client): Before: real 0m45.973s user 0m0.080s sys 0m0.020s After: real 0m14.951s user 0m0.080s sys 0m0.020s Michal Privoznik (9): qemu: Move close callbacks handling into util/virclosecallbacks.c Introduce a virLXCDriverConfigPtr object lxc: Use atomic ops for driver->nactive Introduce annotations for virLXCDriverPtr fields lxc: switch to virCloseCallbacks API Stop accessing driver->caps directly in LXC driver lxc: Make activeUsbHostdevs use locks Remove lxcDriverLock from almost everywhere Introduce lxcDomObjFromDomain po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/libvirt_private.syms | 8 + src/lxc/lxc_conf.c | 120 ++++++-- src/lxc/lxc_conf.h | 64 ++-- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 705 +++++++++++++++---------------------------- src/lxc/lxc_hostdev.c | 8 + src/lxc/lxc_process.c | 181 ++++------- src/lxc/lxc_process.h | 1 - src/qemu/qemu_conf.c | 295 +----------------- src/qemu/qemu_conf.h | 25 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 18 +- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_process.c | 14 +- src/util/virclosecallbacks.c | 356 ++++++++++++++++++++++ src/util/virclosecallbacks.h | 56 ++++ 18 files changed, 900 insertions(+), 964 deletions(-) create mode 100644 src/util/virclosecallbacks.c create mode 100644 src/util/virclosecallbacks.h -- 1.8.1.5

--- po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/libvirt_private.syms | 7 + src/qemu/qemu_conf.c | 295 +------------------------------------- src/qemu/qemu_conf.h | 25 +--- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 18 +-- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_process.c | 14 +- src/util/virclosecallbacks.c | 332 +++++++++++++++++++++++++++++++++++++++++++ src/util/virclosecallbacks.h | 53 +++++++ 11 files changed, 418 insertions(+), 337 deletions(-) create mode 100644 src/util/virclosecallbacks.c create mode 100644 src/util/virclosecallbacks.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 0b65765..2e4ebc8 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -146,6 +146,7 @@ src/util/viraudit.c src/util/virauth.c src/util/virauthconfig.c src/util/vircgroup.c +src/util/virclosecallbacks.c src/util/vircommand.c src/util/virconf.c src/util/virdbus.c diff --git a/src/Makefile.am b/src/Makefile.am index d9e703f..8fa8680 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -83,6 +83,7 @@ UTIL_SOURCES = \ util/virbitmap.c util/virbitmap.h \ util/virbuffer.c util/virbuffer.h \ util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h \ + util/virclosecallbacks.c util/virclosecallbacks.h \ util/vircommand.c util/vircommand.h \ util/virconf.c util/virconf.h \ util/virdbus.c util/virdbus.h \ @@ -882,7 +883,8 @@ libvirt_util_la_SOURCES = \ $(UTIL_SOURCES) libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \ - $(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS) + $(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS) \ + -I$(top_srcdir)/src/conf libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc4e750..53b1153 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1204,6 +1204,13 @@ virCgroupSetMemorySoftLimit; virCgroupSetMemSwapHardLimit; +# util/virclosecallbacks.h +virCloseCallbacksGet; +virCloseCallbacksNew; +virCloseCallbacksRun; +virCloseCallbacksSet; +virCloseCallbacksUnset; + # util/vircommand.h virCommandAbort; virCommandAddArg; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c91551f..64214b9 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -56,25 +56,8 @@ #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) { @@ -83,12 +66,7 @@ static int virQEMUConfigOnceInit(void) sizeof(virQEMUDriverConfig), virQEMUDriverConfigDispose); - virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(), - "virQEMUCloseCallbacks", - sizeof(virQEMUCloseCallbacks), - virQEMUCloseCallbacksDispose); - - if (!virQEMUDriverConfigClass || !virQEMUCloseCallbacksClass) + if (!virQEMUDriverConfigClass) return -1; else return 0; @@ -662,277 +640,6 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver, return ret; } - -static void -virQEMUCloseCallbacksFreeData(void *payload, - const void *name ATTRIBUTE_UNUSED) -{ - VIR_FREE(payload); -} - -virQEMUCloseCallbacksPtr -virQEMUCloseCallbacksNew(void) -{ - virQEMUCloseCallbacksPtr closeCallbacks; - - 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; -} - -static void -virQEMUCloseCallbacksDispose(void *obj) -{ - virQEMUCloseCallbacksPtr closeCallbacks = obj; - - virHashFree(closeCallbacks->list); -} - -int -virQEMUCloseCallbacksSet(virQEMUCloseCallbacksPtr closeCallbacks, - virDomainObjPtr vm, - virConnectPtr conn, - virQEMUCloseCallback cb) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - qemuDriverCloseDefPtr closeDef; - int ret = -1; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p", - vm->def->name, uuidstr, conn, cb); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (closeDef) { - if (closeDef->conn != conn) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Close callback for domain %s already registered" - " with another connection %p"), - vm->def->name, closeDef->conn); - goto cleanup; - } - if (closeDef->cb && closeDef->cb != cb) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Another close callback is already defined for" - " domain %s"), vm->def->name); - goto cleanup; - } - - closeDef->cb = cb; - } else { - if (VIR_ALLOC(closeDef) < 0) - goto cleanup; - - closeDef->conn = conn; - closeDef->cb = cb; - if (virHashAddEntry(closeCallbacks->list, uuidstr, closeDef) < 0) { - VIR_FREE(closeDef); - goto cleanup; - } - } - - ret = 0; -cleanup: - virObjectUnlock(closeCallbacks); - return ret; -} - -int -virQEMUCloseCallbacksUnset(virQEMUCloseCallbacksPtr closeCallbacks, - virDomainObjPtr vm, - virQEMUCloseCallback cb) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - qemuDriverCloseDefPtr closeDef; - int ret = -1; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, cb=%p", - vm->def->name, uuidstr, cb); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (!closeDef) - goto cleanup; - - if (closeDef->cb && closeDef->cb != cb) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Trying to remove mismatching close callback for" - " domain %s"), vm->def->name); - goto cleanup; - } - - ret = virHashRemoveEntry(closeCallbacks->list, uuidstr); -cleanup: - virObjectUnlock(closeCallbacks); - return ret; -} - -virQEMUCloseCallback -virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks, - virDomainObjPtr vm, - virConnectPtr conn) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - qemuDriverCloseDefPtr closeDef; - virQEMUCloseCallback cb = NULL; - - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s, uuid=%s, conn=%p", - vm->def->name, uuidstr, conn); - - virObjectLock(closeCallbacks); - - closeDef = virHashLookup(closeCallbacks->list, uuidstr); - if (closeDef && (!conn || closeDef->conn == conn)) - cb = closeDef->cb; - - virObjectUnlock(closeCallbacks); - - VIR_DEBUG("cb=%p", cb); - return cb; -} - - -typedef struct _virQEMUCloseCallbacksListEntry virQEMUCloseCallbacksListEntry; -typedef virQEMUCloseCallbacksListEntry *virQEMUCloseCallbacksListEntryPtr; -struct _virQEMUCloseCallbacksListEntry { - unsigned char uuid[VIR_UUID_BUFLEN]; - virQEMUCloseCallback callback; -}; - - -typedef struct _virQEMUCloseCallbacksList virQEMUCloseCallbacksList; -typedef virQEMUCloseCallbacksList *virQEMUCloseCallbacksListPtr; -struct _virQEMUCloseCallbacksList { - size_t nentries; - virQEMUCloseCallbacksListEntryPtr entries; -}; - - -struct virQEMUCloseCallbacksData { - virConnectPtr conn; - virQEMUCloseCallbacksListPtr list; - bool oom; -}; - - -static void -virQEMUCloseCallbacksGetOne(void *payload, - const void *key, - void *opaque) -{ - struct virQEMUCloseCallbacksData *data = opaque; - qemuDriverCloseDefPtr closeDef = payload; - const char *uuidstr = key; - unsigned char uuid[VIR_UUID_BUFLEN]; - - if (virUUIDParse(uuidstr, uuid) < 0) - return; - - VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", - closeDef->conn, data->conn, uuidstr, closeDef->cb); - - if (data->conn != closeDef->conn || !closeDef->cb) - return; - - if (VIR_EXPAND_N(data->list->entries, - data->list->nentries, 1) < 0) { - data->oom = true; - return; - } - - memcpy(data->list->entries[data->list->nentries - 1].uuid, - uuid, VIR_UUID_BUFLEN); - data->list->entries[data->list->nentries - 1].callback = closeDef->cb; -} - - -static virQEMUCloseCallbacksListPtr -virQEMUCloseCallbacksGetForConn(virQEMUCloseCallbacksPtr closeCallbacks, - virConnectPtr conn) -{ - virQEMUCloseCallbacksListPtr list = NULL; - struct virQEMUCloseCallbacksData data; - - if (VIR_ALLOC(list) < 0) - return NULL; - - data.conn = conn; - data.list = list; - data.oom = false; - - virHashForEach(closeCallbacks->list, virQEMUCloseCallbacksGetOne, &data); - - if (data.oom) { - VIR_FREE(list->entries); - VIR_FREE(list); - virReportOOMError(); - return NULL; - } - - return list; -} - - -void -virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, - virConnectPtr conn, - virQEMUDriverPtr driver) -{ - virQEMUCloseCallbacksListPtr list; - size_t i; - - VIR_DEBUG("conn=%p", conn); - - /* We must not hold the lock while running the callbacks, - * so first we obtain the list of callbacks, then remove - * them all from the hash. At that point we can release - * the lock and run the callbacks safely. */ - - virObjectLock(closeCallbacks); - list = virQEMUCloseCallbacksGetForConn(closeCallbacks, conn); - if (!list) - return; - - for (i = 0; i < list->nentries; i++) { - virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); - } - virObjectUnlock(closeCallbacks); - - for (i = 0; i < list->nentries; i++) { - virDomainObjPtr vm; - - if (!(vm = virDomainObjListFindByUUID(driver->domains, - list->entries[i].uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(list->entries[i].uuid, uuidstr); - VIR_DEBUG("No domain object with UUID %s", uuidstr); - continue; - } - - vm = list->entries[i].callback(driver, vm, conn); - if (vm) - virObjectUnlock(vm); - } - VIR_FREE(list->entries); - VIR_FREE(list); -} - struct _qemuSharedDeviceEntry { size_t ref; char **domains; /* array of domain names */ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 19893c8..8229cfc 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -42,6 +42,7 @@ # include "virthreadpool.h" # include "locking/lock_manager.h" # include "qemu_capabilities.h" +# include "virclosecallbacks.h" # ifdef CPU_SETSIZE /* Linux */ # define QEMUD_CPUMASK_LEN CPU_SETSIZE @@ -51,9 +52,6 @@ # error "Port me" # endif -typedef struct _virQEMUCloseCallbacks virQEMUCloseCallbacks; -typedef virQEMUCloseCallbacks *virQEMUCloseCallbacksPtr; - typedef struct _virQEMUDriver virQEMUDriver; typedef virQEMUDriver *virQEMUDriverPtr; @@ -229,7 +227,7 @@ struct _virQEMUDriver { virLockManagerPluginPtr lockManager; /* Immutable pointer, self-clocking APIs */ - virQEMUCloseCallbacksPtr closeCallbacks; + virCloseCallbacksPtr closeCallbacks; }; typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef; @@ -266,25 +264,6 @@ struct qemuDomainDiskInfo { int io_status; }; -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, - virQEMUCloseCallback cb); -virQEMUCloseCallback -virQEMUCloseCallbacksGet(virQEMUCloseCallbacksPtr closeCallbacks, - virDomainObjPtr vm, - virConnectPtr conn); -void virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, - virConnectPtr conn, - virQEMUDriverPtr driver); - typedef struct _qemuSharedDeviceEntry qemuSharedDeviceEntry; typedef qemuSharedDeviceEntry *qemuSharedDeviceEntryPtr; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b7b066d..dab0513 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -776,7 +776,7 @@ qemuStateInitialize(bool privileged, cfg->hugepagePath = mempath; } - if (!(qemu_driver->closeCallbacks = virQEMUCloseCallbacksNew())) + if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew())) goto error; /* Get all the running persistent or transient configs first */ @@ -1076,7 +1076,7 @@ static int qemuConnectClose(virConnectPtr conn) virQEMUDriverPtr driver = conn->privateData; /* Get rid of callbacks registered for this conn */ - virQEMUCloseCallbacksRun(driver->closeCallbacks, conn, driver); + virCloseCallbacksRun(driver->closeCallbacks, conn, driver->domains, driver); conn->privateData = NULL; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 62e0cbc..19343a6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1897,7 +1897,7 @@ cleanup: * qemuDomainMigratePerform3 and qemuDomainMigrateConfirm3. */ virDomainObjPtr -qemuMigrationCleanup(virQEMUDriverPtr driver, +qemuMigrationCleanup(void *driver, virDomainObjPtr vm, virConnectPtr conn) { @@ -2100,8 +2100,8 @@ qemuMigrationBegin(virConnectPtr conn, * This prevents any other APIs being invoked while migration is taking * place. */ - if (virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationCleanup) < 0) + if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, + qemuMigrationCleanup) < 0) goto endjob; if (qemuMigrationJobContinue(vm) == 0) { vm = NULL; @@ -2750,8 +2750,8 @@ qemuMigrationConfirm(virConnectPtr conn, phase = QEMU_MIGRATION_PHASE_CONFIRM3; qemuMigrationJobStartPhase(driver, vm, phase); - virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationCleanup); + virCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuMigrationCleanup); ret = qemuMigrationConfirmPhase(driver, conn, vm, cookiein, cookieinlen, @@ -4123,8 +4123,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, } qemuMigrationJobStartPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3); - virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuMigrationCleanup); + virCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuMigrationCleanup); resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, @@ -4155,8 +4155,8 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE); - if (virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuMigrationCleanup) < 0) + if (virCloseCallbacksSet(driver->closeCallbacks, vm, conn, + qemuMigrationCleanup) < 0) goto endjob; endjob: diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 43b26de..bf962b1 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -90,7 +90,7 @@ bool qemuMigrationJobFinish(virQEMUDriverPtr driver, virDomainObjPtr obj) int qemuMigrationSetOffline(virQEMUDriverPtr driver, virDomainObjPtr vm); -virDomainObjPtr qemuMigrationCleanup(virQEMUDriverPtr driver, +virDomainObjPtr qemuMigrationCleanup(void *driver, virDomainObjPtr vm, virConnectPtr conn); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3d5e8f6..9b4a448 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4468,7 +4468,7 @@ cleanup: static virDomainObjPtr -qemuProcessAutoDestroy(virQEMUDriverPtr driver, +qemuProcessAutoDestroy(void *driver, virDomainObjPtr dom, virConnectPtr conn) { @@ -4515,23 +4515,23 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver, virConnectPtr conn) { VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn); - return virQEMUCloseCallbacksSet(driver->closeCallbacks, vm, conn, - qemuProcessAutoDestroy); + return virCloseCallbacksSet(driver->closeCallbacks, vm, conn, + qemuProcessAutoDestroy); } int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver, virDomainObjPtr vm) { VIR_DEBUG("vm=%s", vm->def->name); - return virQEMUCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuProcessAutoDestroy); + return virCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuProcessAutoDestroy); } bool qemuProcessAutoDestroyActive(virQEMUDriverPtr driver, virDomainObjPtr vm) { - virQEMUCloseCallback cb; + virCloseCallback cb; VIR_DEBUG("vm=%s", vm->def->name); - cb = virQEMUCloseCallbacksGet(driver->closeCallbacks, vm, NULL); + cb = virCloseCallbacksGet(driver->closeCallbacks, vm, NULL); return cb == qemuProcessAutoDestroy; } diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c new file mode 100644 index 0000000..a926456 --- /dev/null +++ b/src/util/virclosecallbacks.c @@ -0,0 +1,332 @@ +/* + * virclosecallbacks.c: Connection close callbacks routines + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + * Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "viralloc.h" +#include "virclosecallbacks.h" +#include "virlog.h" +#include "virobject.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct _virDriverCloseDef virDriverCloseDef; +typedef virDriverCloseDef *virDriverCloseDefPtr; +struct _virDriverCloseDef { + virConnectPtr conn; + virCloseCallback cb; +}; + +struct _virCloseCallbacks { + virObjectLockable parent; + + /* UUID string to qemuDriverCloseDef mapping */ + virHashTablePtr list; +}; + + +static virClassPtr virCloseCallbacksClass; +static void virCloseCallbacksDispose(void *obj); + +static int virCloseCallbacksOnceInit(void) +{ + virCloseCallbacksClass = virClassNew(virClassForObjectLockable(), + "virCloseCallbacks", + sizeof(virCloseCallbacks), + virCloseCallbacksDispose); + + if (!virCloseCallbacksClass) + return -1; + else + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virCloseCallbacks) + + +static void +virCloseCallbacksFreeData(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(payload); +} + +virCloseCallbacksPtr +virCloseCallbacksNew(void) +{ + virCloseCallbacksPtr closeCallbacks; + + if (virCloseCallbacksInitialize() < 0) + return NULL; + + if (!(closeCallbacks = virObjectLockableNew(virCloseCallbacksClass))) + return NULL; + + closeCallbacks->list = virHashCreate(5, virCloseCallbacksFreeData); + if (!closeCallbacks->list) { + virObjectUnref(closeCallbacks); + return NULL; + } + + return closeCallbacks; +} + +static void +virCloseCallbacksDispose(void *obj) +{ + virCloseCallbacksPtr closeCallbacks = obj; + + virHashFree(closeCallbacks->list); +} + +int +virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn, + virCloseCallback cb) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDriverCloseDefPtr closeDef; + int ret = -1; + + virUUIDFormat(vm->def->uuid, uuidstr); + VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p", + vm->def->name, uuidstr, conn, cb); + + virObjectLock(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); + if (closeDef) { + if (closeDef->conn != conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Close callback for domain %s already registered" + " with another connection %p"), + vm->def->name, closeDef->conn); + goto cleanup; + } + if (closeDef->cb && closeDef->cb != cb) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Another close callback is already defined for" + " domain %s"), vm->def->name); + goto cleanup; + } + + closeDef->cb = cb; + } else { + if (VIR_ALLOC(closeDef) < 0) + goto cleanup; + + closeDef->conn = conn; + closeDef->cb = cb; + if (virHashAddEntry(closeCallbacks->list, uuidstr, closeDef) < 0) { + VIR_FREE(closeDef); + goto cleanup; + } + } + + ret = 0; +cleanup: + virObjectUnlock(closeCallbacks); + return ret; +} + +int +virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virCloseCallback cb) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDriverCloseDefPtr closeDef; + int ret = -1; + + virUUIDFormat(vm->def->uuid, uuidstr); + VIR_DEBUG("vm=%s, uuid=%s, cb=%p", + vm->def->name, uuidstr, cb); + + virObjectLock(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); + if (!closeDef) + goto cleanup; + + if (closeDef->cb && closeDef->cb != cb) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Trying to remove mismatching close callback for" + " domain %s"), vm->def->name); + goto cleanup; + } + + ret = virHashRemoveEntry(closeCallbacks->list, uuidstr); +cleanup: + virObjectUnlock(closeCallbacks); + return ret; +} + +virCloseCallback +virCloseCallbacksGet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDriverCloseDefPtr closeDef; + virCloseCallback cb = NULL; + + virUUIDFormat(vm->def->uuid, uuidstr); + VIR_DEBUG("vm=%s, uuid=%s, conn=%p", + vm->def->name, uuidstr, conn); + + virObjectLock(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); + if (closeDef && (!conn || closeDef->conn == conn)) + cb = closeDef->cb; + + virObjectUnlock(closeCallbacks); + + VIR_DEBUG("cb=%p", cb); + return cb; +} + +typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; +typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr; +struct _virCloseCallbacksListEntry { + unsigned char uuid[VIR_UUID_BUFLEN]; + virCloseCallback callback; +}; + +typedef struct _virCloseCallbacksList virCloseCallbacksList; +typedef virCloseCallbacksList *virCloseCallbacksListPtr; +struct _virCloseCallbacksList { + size_t nentries; + virCloseCallbacksListEntryPtr entries; +}; + +struct virCloseCallbacksData { + virConnectPtr conn; + virCloseCallbacksListPtr list; + bool oom; +}; + +static void +virCloseCallbacksGetOne(void *payload, + const void *key, + void *opaque) +{ + struct virCloseCallbacksData *data = opaque; + virDriverCloseDefPtr closeDef = payload; + const char *uuidstr = key; + unsigned char uuid[VIR_UUID_BUFLEN]; + + if (virUUIDParse(uuidstr, uuid) < 0) + return; + + VIR_DEBUG("conn=%p, thisconn=%p, uuid=%s, cb=%p", + closeDef->conn, data->conn, uuidstr, closeDef->cb); + + if (data->conn != closeDef->conn || !closeDef->cb) + return; + + if (VIR_EXPAND_N(data->list->entries, + data->list->nentries, 1) < 0) { + data->oom = true; + return; + } + + memcpy(data->list->entries[data->list->nentries - 1].uuid, + uuid, VIR_UUID_BUFLEN); + data->list->entries[data->list->nentries - 1].callback = closeDef->cb; +} + +static virCloseCallbacksListPtr +virCloseCallbacksGetForConn(virCloseCallbacksPtr closeCallbacks, + virConnectPtr conn) +{ + virCloseCallbacksListPtr list = NULL; + struct virCloseCallbacksData data; + + if (VIR_ALLOC(list) < 0) + return NULL; + + data.conn = conn; + data.list = list; + data.oom = false; + + virHashForEach(closeCallbacks->list, virCloseCallbacksGetOne, &data); + + if (data.oom) { + VIR_FREE(list->entries); + VIR_FREE(list); + virReportOOMError(); + return NULL; + } + + return list; +} + + +void +virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, + virConnectPtr conn, + virDomainObjListPtr domains, + void *driver) +{ + virCloseCallbacksListPtr list; + size_t i; + + VIR_DEBUG("conn=%p", conn); + + /* We must not hold the lock while running the callbacks, + * so first we obtain the list of callbacks, then remove + * them all from the hash. At that point we can release + * the lock and run the callbacks safely. */ + + virObjectLock(closeCallbacks); + list = virCloseCallbacksGetForConn(closeCallbacks, conn); + if (!list) + return; + + for (i = 0; i < list->nentries; i++) { + virHashRemoveEntry(closeCallbacks->list, + list->entries[i].uuid); + } + virObjectUnlock(closeCallbacks); + + for (i = 0; i < list->nentries; i++) { + virDomainObjPtr vm; + + if (!(vm = virDomainObjListFindByUUID(domains, + list->entries[i].uuid))) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(list->entries[i].uuid, uuidstr); + VIR_DEBUG("No domain object with UUID %s", uuidstr); + continue; + } + + vm = list->entries[i].callback(driver, vm, conn); + if (vm) + virObjectUnlock(vm); + } + VIR_FREE(list->entries); + VIR_FREE(list); +} diff --git a/src/util/virclosecallbacks.h b/src/util/virclosecallbacks.h new file mode 100644 index 0000000..9db641f --- /dev/null +++ b/src/util/virclosecallbacks.h @@ -0,0 +1,53 @@ +/* + * virclosecallbacks.h: Connection close callbacks routines + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + * Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __VIR_CLOSE_CALLBACKS__ +# define __VIR_CLOSE_CALLBACKS__ + +# include "domain_conf.h" + +typedef struct _virCloseCallbacks virCloseCallbacks; +typedef virCloseCallbacks *virCloseCallbacksPtr; + +typedef virDomainObjPtr (*virCloseCallback)(void *driver, + virDomainObjPtr vm, + virConnectPtr conn); +virCloseCallbacksPtr virCloseCallbacksNew(void); +int virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn, + virCloseCallback cb); +int virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virCloseCallback cb); +virCloseCallback +virCloseCallbacksGet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn); +void +virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, + virConnectPtr conn, + virDomainObjListPtr domains, + void *driver); +#endif /* __VIR_CLOSE_CALLBACKS__ */ -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:18PM +0200, Michal Privoznik wrote:
--- po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/libvirt_private.syms | 7 + src/qemu/qemu_conf.c | 295 +------------------------------------- src/qemu/qemu_conf.h | 25 +--- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 18 +-- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_process.c | 14 +- src/util/virclosecallbacks.c | 332 +++++++++++++++++++++++++++++++++++++++++++ src/util/virclosecallbacks.h | 53 +++++++ 11 files changed, 418 insertions(+), 337 deletions(-) create mode 100644 src/util/virclosecallbacks.c create mode 100644 src/util/virclosecallbacks.h
+ +typedef struct _virCloseCallbacks virCloseCallbacks; +typedef virCloseCallbacks *virCloseCallbacksPtr; + +typedef virDomainObjPtr (*virCloseCallback)(void *driver, + virDomainObjPtr vm, + virConnectPtr conn);
I have a preference for the 'void *' callback parameter to be last in the parameter list, and also named 'void *opaque'
+virCloseCallbacksPtr virCloseCallbacksNew(void); +int virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn, + virCloseCallback cb); +int virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virCloseCallback cb); +virCloseCallback +virCloseCallbacksGet(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm, + virConnectPtr conn); +void +virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, + virConnectPtr conn, + virDomainObjListPtr domains, + void *driver);
Again s/driver/opaque/, and in the various other files where this name is used. ACK if the param is renamed & moved to be last 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 :|

Currently the virLXCDriverPtr struct contains an wide variety of data with varying access needs. Move all the static config data into a dedicated virLXCDriverConfigPtr object. The only locking requirement is to hold the driver lock, while obtaining an instance of virLXCDriverConfigPtr. Once a reference is held on the config object, it can be used completely lockless since it is immutable. NB, not all APIs correctly hold the driver lock while getting a reference to the config object in this patch. This is safe for now since the config is never updated on the fly. Later patches will address this fully. --- src/lxc/lxc_conf.c | 81 +++++++++++++++++++++------- src/lxc/lxc_conf.h | 41 +++++++++----- src/lxc/lxc_driver.c | 145 ++++++++++++++++++++++++++++++++++---------------- src/lxc/lxc_process.c | 39 +++++++++----- 4 files changed, 214 insertions(+), 92 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 78b1559..4e97f10 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -41,6 +41,22 @@ #define VIR_FROM_THIS VIR_FROM_LXC +static virClassPtr virLXCDriverConfigClass; +static void virLXCDriverConfigDispose(void *obj); + +static int virLXCConfigOnceInit(void) +{ + if (!(virLXCDriverConfigClass = virClassNew(virClassForObject(), + "virLXCDriverConfig", + sizeof(virLXCDriverConfig), + virLXCDriverConfigDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virLXCConfig) + /* Functions */ virCapsPtr lxcCapsInit(virLXCDriverPtr driver) @@ -146,28 +162,42 @@ lxcDomainXMLConfInit(void) } -int lxcLoadDriverConfig(virLXCDriverPtr driver) +virLXCDriverConfigPtr +virLXCDriverConfigNew(void) { - char *filename; - virConfPtr conf; - virConfValuePtr p; + virLXCDriverConfigPtr cfg; + + if (virLXCConfigInitialize() < 0) + return NULL; - driver->securityDefaultConfined = false; - driver->securityRequireConfined = false; + if (!(cfg = virObjectNew(virLXCDriverConfigClass))) + return NULL; + + cfg->securityDefaultConfined = false; + cfg->securityRequireConfined = false; /* Set the container configuration directory */ - if (VIR_STRDUP(driver->configDir, LXC_CONFIG_DIR) < 0) + if (VIR_STRDUP(cfg->configDir, LXC_CONFIG_DIR) < 0) goto error; - if (VIR_STRDUP(driver->stateDir, LXC_STATE_DIR) < 0) + if (VIR_STRDUP(cfg->stateDir, LXC_STATE_DIR) < 0) goto error; - if (VIR_STRDUP(driver->logDir, LXC_LOG_DIR) < 0) + if (VIR_STRDUP(cfg->logDir, LXC_LOG_DIR) < 0) goto error; - if (VIR_STRDUP(driver->autostartDir, LXC_AUTOSTART_DIR) < 0) + if (VIR_STRDUP(cfg->autostartDir, LXC_AUTOSTART_DIR) < 0) goto error; + return cfg; +error: + virObjectUnref(cfg); + return NULL; +} - if (VIR_STRDUP(filename, SYSCONFDIR "/libvirt/lxc.conf") < 0) - goto error; +int +virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, + const char *filename) +{ + virConfPtr conf; + virConfValuePtr p; /* Avoid error from non-existant or unreadable file. */ if (access(filename, R_OK) == -1) @@ -186,12 +216,12 @@ int lxcLoadDriverConfig(virLXCDriverPtr driver) p = virConfGetValue(conf, "log_with_libvirtd"); CHECK_TYPE("log_with_libvirtd", VIR_CONF_LONG); - if (p) driver->log_libvirtd = p->l; + if (p) cfg->log_libvirtd = p->l; p = virConfGetValue(conf, "security_driver"); CHECK_TYPE("security_driver", VIR_CONF_STRING); if (p && p->str) { - if (VIR_STRDUP(driver->securityDriverName, p->str) < 0) { + if (VIR_STRDUP(cfg->securityDriverName, p->str) < 0) { virConfFree(conf); return -1; } @@ -199,11 +229,11 @@ int lxcLoadDriverConfig(virLXCDriverPtr driver) p = virConfGetValue(conf, "security_default_confined"); CHECK_TYPE("security_default_confined", VIR_CONF_LONG); - if (p) driver->securityDefaultConfined = p->l; + if (p) cfg->securityDefaultConfined = p->l; p = virConfGetValue(conf, "security_require_confined"); CHECK_TYPE("security_require_confined", VIR_CONF_LONG); - if (p) driver->securityRequireConfined = p->l; + if (p) cfg->securityRequireConfined = p->l; #undef CHECK_TYPE @@ -211,9 +241,22 @@ int lxcLoadDriverConfig(virLXCDriverPtr driver) virConfFree(conf); done: - VIR_FREE(filename); return 0; +} -error: - return -1; +virLXCDriverConfigPtr virLXCDriverGetConfig(virLXCDriverPtr driver) +{ + return virObjectRef(driver->config); +} + +static void +virLXCDriverConfigDispose(void *obj) +{ + virLXCDriverConfigPtr cfg = obj; + + VIR_FREE(cfg->configDir); + VIR_FREE(cfg->autostartDir); + VIR_FREE(cfg->stateDir); + VIR_FREE(cfg->logDir); + VIR_FREE(cfg->securityDriverName); } diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 5a5b9aa..6ca6198 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -46,44 +46,57 @@ typedef struct _virLXCDriver virLXCDriver; typedef virLXCDriver *virLXCDriverPtr; +typedef struct _virLXCDriverConfig virLXCDriverConfig; +typedef virLXCDriverConfig *virLXCDriverConfigPtr; + +struct _virLXCDriverConfig { + virObject parent; + + char *configDir; + char *autostartDir; + char *stateDir; + char *logDir; + int log_libvirtd; + int have_netns; + + char *securityDriverName; + bool securityDefaultConfined; + bool securityRequireConfined; +}; + struct _virLXCDriver { virMutex lock; + virLXCDriverConfigPtr config; + virCapsPtr caps; - virDomainXMLOptionPtr xmlopt; virCgroupPtr cgroup; + virDomainXMLOptionPtr xmlopt; + virSysinfoDefPtr hostsysinfo; size_t nactive; + virStateInhibitCallback inhibitCallback; void *inhibitOpaque; virDomainObjListPtr domains; - char *configDir; - char *autostartDir; - char *stateDir; - char *logDir; - int log_libvirtd; - int have_netns; virUSBDeviceListPtr activeUsbHostdevs; virDomainEventStatePtr domainEventState; - char *securityDriverName; - bool securityDefaultConfined; - bool securityRequireConfined; virSecurityManagerPtr securityManager; - /* Mapping of 'char *uuidstr' -> virConnectPtr - * of guests which will be automatically killed - * when the virConnectPtr is closed*/ virHashTablePtr autodestroy; }; -int lxcLoadDriverConfig(virLXCDriverPtr driver); +virLXCDriverConfigPtr virLXCDriverConfigNew(void); +virLXCDriverConfigPtr virLXCDriverGetConfig(virLXCDriverPtr driver); +int virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, + const char *filename); virCapsPtr lxcCapsInit(virLXCDriverPtr driver); virDomainXMLOptionPtr lxcDomainXMLConfInit(void); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 098051b..855dad3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -456,8 +456,11 @@ static virDomainPtr lxcDomainDefineXML(virConnectPtr conn, const char *xml) virDomainPtr dom = NULL; virDomainEventPtr event = NULL; virDomainDefPtr oldDef = NULL; + virLXCDriverConfigPtr cfg = NULL; lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, VIR_DOMAIN_XML_INACTIVE))) @@ -469,7 +472,7 @@ static virDomainPtr lxcDomainDefineXML(virConnectPtr conn, const char *xml) if (virSecurityManagerVerify(driver->securityManager, def) < 0) goto cleanup; - if ((def->nets != NULL) && !(driver->have_netns)) { + if ((def->nets != NULL) && !(cfg->have_netns)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("System lacks NETNS support")); goto cleanup; @@ -482,7 +485,7 @@ static virDomainPtr lxcDomainDefineXML(virConnectPtr conn, const char *xml) def = NULL; vm->persistent = 1; - if (virDomainSaveConfig(driver->configDir, + if (virDomainSaveConfig(cfg->configDir, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); vm = NULL; @@ -507,6 +510,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virObjectUnref(cfg); return dom; } @@ -517,10 +521,13 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(0, -1); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -539,8 +546,8 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, goto cleanup; } - if (virDomainDeleteConfig(driver->configDir, - driver->autostartDir, + if (virDomainDeleteConfig(cfg->configDir, + cfg->autostartDir, vm) < 0) goto cleanup; @@ -563,6 +570,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -1027,10 +1035,13 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1043,7 +1054,7 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if ((vm->def->nets != NULL) && !(driver->have_netns)) { + if ((vm->def->nets != NULL) && !(cfg->have_netns)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("System lacks NETNS support")); goto cleanup; @@ -1074,6 +1085,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -1103,16 +1115,20 @@ static int lxcDomainCreate(virDomainPtr dom) static virDomainPtr lxcDomainCreateXML(virConnectPtr conn, const char *xml, - unsigned int flags) { + unsigned int flags) +{ virLXCDriverPtr driver = conn->privateData; virDomainObjPtr vm = NULL; virDomainDefPtr def; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, VIR_DOMAIN_XML_INACTIVE))) @@ -1124,7 +1140,7 @@ lxcDomainCreateXML(virConnectPtr conn, if (virSecurityManagerVerify(driver->securityManager, def) < 0) goto cleanup; - if ((def->nets != NULL) && !(driver->have_netns)) { + if ((def->nets != NULL) && !(cfg->have_netns)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("System lacks NETNS support")); goto cleanup; @@ -1163,6 +1179,7 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); lxcDriverUnlock(driver); + virObjectUnref(cfg); return dom; } @@ -1456,26 +1473,24 @@ static int lxcCheckNetNsSupport(void) } -static int -lxcSecurityInit(virLXCDriverPtr driver) +static virSecurityManagerPtr +lxcSecurityInit(virLXCDriverConfigPtr cfg) { - VIR_INFO("lxcSecurityInit %s", driver->securityDriverName); - virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, + VIR_INFO("lxcSecurityInit %s", cfg->securityDriverName); + virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName, LXC_DRIVER_NAME, false, - driver->securityDefaultConfined, - driver->securityRequireConfined); + cfg->securityDefaultConfined, + cfg->securityRequireConfined); if (!mgr) goto error; - driver->securityManager = mgr; - - return 0; + return mgr; error: VIR_ERROR(_("Failed to initialize security drivers")); virObjectUnref(mgr); - return -1; + return NULL; } @@ -1484,6 +1499,7 @@ static int lxcStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *ld; + virLXCDriverConfigPtr cfg = NULL; /* Valgrind gets very annoyed when we clone containers, so * disable LXC when under valgrind @@ -1525,14 +1541,17 @@ static int lxcStateInitialize(bool privileged, lxc_driver->hostsysinfo = virSysinfoRead(); - lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ - lxc_driver->have_netns = lxcCheckNetNsSupport(); + if (!(lxc_driver->config = cfg = virLXCDriverConfigNew())) + goto cleanup; + + cfg->log_libvirtd = 0; /* by default log to container logfile */ + cfg->have_netns = lxcCheckNetNsSupport(); /* Call function to load lxc driver configuration information */ - if (lxcLoadDriverConfig(lxc_driver) < 0) + if (virLXCLoadDriverConfig(cfg, SYSCONFDIR "/libvirt/lxc.conf") < 0) goto cleanup; - if (lxcSecurityInit(lxc_driver) < 0) + if (!(lxc_driver->securityManager = lxcSecurityInit(cfg))) goto cleanup; if ((lxc_driver->activeUsbHostdevs = virUSBDeviceListNew()) == NULL) @@ -1549,7 +1568,7 @@ static int lxcStateInitialize(bool privileged, /* Get all the running persistent or transient configs first */ if (virDomainObjListLoadAllConfigs(lxc_driver->domains, - lxc_driver->stateDir, + cfg->stateDir, NULL, 1, lxc_driver->caps, lxc_driver->xmlopt, @@ -1561,8 +1580,8 @@ static int lxcStateInitialize(bool privileged, /* Then inactive persistent configs */ if (virDomainObjListLoadAllConfigs(lxc_driver->domains, - lxc_driver->configDir, - lxc_driver->autostartDir, 0, + cfg->configDir, + cfg->autostartDir, 0, lxc_driver->caps, lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, @@ -1604,19 +1623,23 @@ static void lxcNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque) */ static int lxcStateReload(void) { + virLXCDriverConfigPtr cfg = NULL; + if (!lxc_driver) return 0; lxcDriverLock(lxc_driver); + cfg = virLXCDriverGetConfig(lxc_driver); + virDomainObjListLoadAllConfigs(lxc_driver->domains, - lxc_driver->configDir, - lxc_driver->autostartDir, 0, + cfg->configDir, + cfg->autostartDir, 0, lxc_driver->caps, lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, lxcNotifyLoadDomain, lxc_driver); lxcDriverUnlock(lxc_driver); - + virObjectUnref(cfg); return 0; } @@ -1638,10 +1661,7 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->caps); virObjectUnref(lxc_driver->securityManager); virObjectUnref(lxc_driver->xmlopt); - VIR_FREE(lxc_driver->configDir); - VIR_FREE(lxc_driver->autostartDir); - VIR_FREE(lxc_driver->stateDir); - VIR_FREE(lxc_driver->logDir); + virObjectUnref(lxc_driver->config); lxcDriverUnlock(lxc_driver); virMutexDestroy(&lxc_driver->lock); VIR_FREE(lxc_driver); @@ -1852,6 +1872,7 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, int ret = -1; int rc; virLXCDomainObjPrivatePtr priv; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -1867,6 +1888,8 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -1945,12 +1968,12 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, } } - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - rc = virDomainSaveConfig(driver->configDir, vmdef); + rc = virDomainSaveConfig(cfg->configDir, vmdef); if (rc < 0) goto cleanup; @@ -1965,6 +1988,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -2106,6 +2130,7 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; virLXCDomainObjPrivatePtr priv; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2117,6 +2142,8 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -2179,7 +2206,7 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, } } - if (virDomainSaveConfig(driver->configDir, persistentDef) < 0) + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) goto cleanup; } @@ -2188,6 +2215,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -2391,13 +2419,17 @@ cleanup: } static int lxcDomainSetAutostart(virDomainPtr dom, - int autostart) { + int autostart) +{ virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; char *configFile = NULL, *autostartLink = NULL; int ret = -1; + virLXCDriverConfigPtr cfg = NULL; lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -2424,20 +2456,20 @@ static int lxcDomainSetAutostart(virDomainPtr dom, goto cleanup; } - configFile = virDomainConfigFile(driver->configDir, + configFile = virDomainConfigFile(cfg->configDir, vm->def->name); if (configFile == NULL) goto cleanup; - autostartLink = virDomainConfigFile(driver->autostartDir, + autostartLink = virDomainConfigFile(cfg->autostartDir, vm->def->name); if (autostartLink == NULL) goto cleanup; if (autostart) { - if (virFileMakePath(driver->autostartDir) < 0) { + if (virFileMakePath(cfg->autostartDir) < 0) { virReportSystemError(errno, _("Cannot create autostart directory %s"), - driver->autostartDir); + cfg->autostartDir); goto cleanup; } @@ -2465,6 +2497,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -2560,8 +2593,11 @@ static int lxcDomainSuspend(virDomainPtr dom) virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; + virLXCDriverConfigPtr cfg = NULL; lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -2594,7 +2630,7 @@ static int lxcDomainSuspend(virDomainPtr dom) VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); } - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; ret = 0; @@ -2604,6 +2640,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -2614,8 +2651,11 @@ static int lxcDomainResume(virDomainPtr dom) virDomainEventPtr event = NULL; int ret = -1; virLXCDomainObjPrivatePtr priv; + virLXCDriverConfigPtr cfg = NULL; lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -2651,7 +2691,7 @@ static int lxcDomainResume(virDomainPtr dom) VIR_DOMAIN_EVENT_RESUMED_UNPAUSED); } - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto cleanup; ret = 0; @@ -2661,6 +2701,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -4284,6 +4325,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4291,6 +4333,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4366,7 +4410,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { ret = -1; goto cleanup; } @@ -4374,7 +4418,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(driver->configDir, vmdef); + ret = virDomainSaveConfig(cfg->configDir, vmdef); if (!ret) { virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; @@ -4389,6 +4433,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -4411,6 +4456,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -4419,6 +4465,8 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4495,7 +4543,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(driver->configDir, vmdef); + ret = virDomainSaveConfig(cfg->configDir, vmdef); if (!ret) { virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; @@ -4510,6 +4558,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } @@ -4524,6 +4573,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; + virLXCDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -4531,6 +4581,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); lxcDriverLock(driver); + cfg = virLXCDriverGetConfig(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4607,7 +4659,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) { + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { ret = -1; goto cleanup; } @@ -4615,7 +4667,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(driver->configDir, vmdef); + ret = virDomainSaveConfig(cfg->configDir, vmdef); if (!ret) { virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; @@ -4630,6 +4682,7 @@ cleanup: if (vm) virObjectUnlock(vm); lxcDriverUnlock(driver); + virObjectUnref(cfg); return ret; } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 396e0eb..1024576 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -224,6 +224,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, size_t i; virLXCDomainObjPrivatePtr priv = vm->privateData; virNetDevVPortProfilePtr vport = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); VIR_DEBUG("Stopping VM name=%s pid=%d reason=%d", vm->def->name, (int)vm->pid, (int)reason); @@ -248,8 +249,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, priv->monitor = NULL; } - virPidFileDelete(driver->stateDir, vm->def->name); - virDomainDeleteConfig(driver->stateDir, NULL, vm); + virPidFileDelete(cfg->stateDir, vm->def->name); + virDomainDeleteConfig(cfg->stateDir, NULL, vm); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); vm->pid = -1; @@ -300,6 +301,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, vm->def->id = -1; vm->newDef = NULL; } + virObjectUnref(cfg); } @@ -366,6 +368,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virLXCDriverPtr driver = conn->privateData; virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); /* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -401,13 +404,14 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virDomainNetGetActualVirtPortProfile(net), &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - driver->stateDir, + cfg->stateDir, virDomainNetGetActualBandwidth(net)) < 0) goto cleanup; ret = res_ifname; cleanup: + virObjectUnref(cfg); return ret; } @@ -672,10 +676,12 @@ static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED { virLXCDriverPtr driver = lxc_driver; virLXCDomainObjPrivatePtr priv; + virLXCDriverConfigPtr cfg; ino_t inode; lxcDriverLock(driver); virObjectLock(vm); + cfg = virLXCDriverGetConfig(driver); lxcDriverUnlock(driver); priv = vm->privateData; @@ -691,10 +697,11 @@ static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED } virDomainAuditInit(vm, initpid, inode); - if (virDomainSaveStatus(lxc_driver->xmlopt, lxc_driver->stateDir, vm) < 0) + if (virDomainSaveStatus(lxc_driver->xmlopt, cfg->stateDir, vm) < 0) VIR_WARN("Cannot update XML with PID for LXC %s", vm->def->name); virObjectUnlock(vm); + virObjectUnref(cfg); } static virLXCMonitorCallbacks monitorCallbacks = { @@ -708,6 +715,7 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver, virDomainObjPtr vm) { virLXCMonitorPtr monitor = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); if (virSecurityManagerSetSocketLabel(driver->securityManager, vm->def) < 0) goto cleanup; @@ -716,7 +724,7 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver, * deleted while the monitor is active */ virObjectRef(vm); - monitor = virLXCMonitorNew(vm, driver->stateDir, &monitorCallbacks); + monitor = virLXCMonitorNew(vm, cfg->stateDir, &monitorCallbacks); if (monitor == NULL) virObjectUnref(vm); @@ -730,6 +738,7 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver, } cleanup: + virObjectUnref(cfg); return monitor; } @@ -809,6 +818,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, char *filterstr; char *outputstr; virCommandPtr cmd; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); cmd = virCommandNew(vm->def->emulator); @@ -829,7 +839,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, VIR_FREE(filterstr); } - if (driver->log_libvirtd) { + if (cfg->log_libvirtd) { if (virLogGetNbOutputs() > 0) { outputstr = virLogGetOutputs(); if (!outputstr) { @@ -869,6 +879,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, return cmd; cleanup: virCommandFree(cmd); + virObjectUnref(cfg); return NULL; } @@ -1042,6 +1053,7 @@ int virLXCProcessStart(virConnectPtr conn, virCommandPtr cmd = NULL; virLXCDomainObjPrivatePtr priv = vm->privateData; virErrorPtr err = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCgroupFree(&priv->cgroup); @@ -1070,15 +1082,15 @@ int virLXCProcessStart(virConnectPtr conn, return -1; } - if (virFileMakePath(driver->logDir) < 0) { + if (virFileMakePath(cfg->logDir) < 0) { virReportSystemError(errno, _("Cannot create log directory '%s'"), - driver->logDir); + cfg->logDir); return -1; } if (virAsprintf(&logfile, "%s/%s.log", - driver->logDir, vm->def->name) < 0) + cfg->logDir, vm->def->name) < 0) return -1; /* Do this up front, so any part of the startup process can add @@ -1168,7 +1180,7 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; /* Save the configuration for the controller */ - if (virDomainSaveConfig(driver->stateDir, vm->def) < 0) + if (virDomainSaveConfig(cfg->stateDir, vm->def) < 0) goto cleanup; if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT, @@ -1242,7 +1254,7 @@ int virLXCProcessStart(virConnectPtr conn, goto cleanup; /* And get its pid */ - if ((r = virPidFileRead(driver->stateDir, vm->def->name, &vm->pid)) < 0) { + if ((r = virPidFileRead(cfg->stateDir, vm->def->name, &vm->pid)) < 0) { char out[1024]; if (virLXCProcessReadLogOutput(vm, logfile, pos, out, 1024) > 0) @@ -1251,7 +1263,7 @@ int virLXCProcessStart(virConnectPtr conn, else virReportSystemError(-r, _("Failed to read pid file %s/%s.pid"), - driver->stateDir, vm->def->name); + cfg->stateDir, vm->def->name); goto cleanup; } @@ -1290,7 +1302,7 @@ int virLXCProcessStart(virConnectPtr conn, * location for the benefit of libvirt_lxc. We're now overwriting * it with the live status XML instead. This is a (currently * harmless) inconsistency we should fix one day */ - if (virDomainSaveStatus(driver->xmlopt, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) goto error; /* finally we can call the 'started' hook script if any */ @@ -1353,6 +1365,7 @@ cleanup: VIR_FORCE_CLOSE(handshakefds[0]); VIR_FORCE_CLOSE(handshakefds[1]); VIR_FREE(logfile); + virObjectUnref(cfg); if (err) { virSetError(err); -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:19PM +0200, Michal Privoznik wrote:
Currently the virLXCDriverPtr struct contains an wide variety of data with varying access needs. Move all the static config data into a dedicated virLXCDriverConfigPtr object. The only locking requirement is to hold the driver lock, while obtaining an instance of virLXCDriverConfigPtr. Once a reference is held on the config object, it can be used completely lockless since it is immutable.
NB, not all APIs correctly hold the driver lock while getting a reference to the config object in this patch. This is safe for now since the config is never updated on the fly. Later patches will address this fully. --- src/lxc/lxc_conf.c | 81 +++++++++++++++++++++------- src/lxc/lxc_conf.h | 41 +++++++++----- src/lxc/lxc_driver.c | 145 ++++++++++++++++++++++++++++++++++---------------- src/lxc/lxc_process.c | 39 +++++++++----- 4 files changed, 214 insertions(+), 92 deletions(-) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 5a5b9aa..6ca6198 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -46,44 +46,57 @@ typedef struct _virLXCDriver virLXCDriver; typedef virLXCDriver *virLXCDriverPtr;
+typedef struct _virLXCDriverConfig virLXCDriverConfig; +typedef virLXCDriverConfig *virLXCDriverConfigPtr; + +struct _virLXCDriverConfig { + virObject parent; + + char *configDir; + char *autostartDir; + char *stateDir; + char *logDir; + int log_libvirtd; + int have_netns; + + char *securityDriverName; + bool securityDefaultConfined; + bool securityRequireConfined; +}; + struct _virLXCDriver { virMutex lock;
+ virLXCDriverConfigPtr config; + virCapsPtr caps; - virDomainXMLOptionPtr xmlopt;
virCgroupPtr cgroup;
+ virDomainXMLOptionPtr xmlopt; +
If I'm being fussy I'd say you shouldn't be moving the 'xmlopt' field in this patch.
virSysinfoDefPtr hostsysinfo;
size_t nactive; + virStateInhibitCallback inhibitCallback; void *inhibitOpaque;
virDomainObjListPtr domains; - char *configDir; - char *autostartDir; - char *stateDir; - char *logDir; - int log_libvirtd; - int have_netns;
virUSBDeviceListPtr activeUsbHostdevs;
virDomainEventStatePtr domainEventState;
- char *securityDriverName; - bool securityDefaultConfined; - bool securityRequireConfined; virSecurityManagerPtr securityManager;
- /* Mapping of 'char *uuidstr' -> virConnectPtr - * of guests which will be automatically killed - * when the virConnectPtr is closed*/
Nor removing this comment, until the later patch which changes this field to use the generic callbacks.
virHashTablePtr autodestroy; };
ACK anyway. This code pattern matches what was done in the QEMU driver which has proved itself succesful. 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 :|

--- src/lxc/lxc_conf.h | 2 +- src/lxc/lxc_process.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 6ca6198..f9a3e53 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -77,7 +77,7 @@ struct _virLXCDriver { virSysinfoDefPtr hostsysinfo; - size_t nactive; + unsigned int nactive; virStateInhibitCallback inhibitCallback; void *inhibitOpaque; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 1024576..4b83729 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -48,6 +48,7 @@ #include "lxc_hostdev.h" #include "virhook.h" #include "virstring.h" +#include "viratomic.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -256,8 +257,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, vm->pid = -1; vm->def->id = -1; - driver->nactive--; - if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque); virLXCDomainReAttachHostDevices(driver, vm->def); @@ -1273,9 +1273,8 @@ int virLXCProcessStart(virConnectPtr conn, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason); priv->doneStopEvent = false; - if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); - driver->nactive++; if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { char out[1024]; @@ -1458,9 +1457,8 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); - if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); - driver->nactive++; if (!(priv->monitor = virLXCProcessConnectMonitor(driver, vm))) goto error; -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:20PM +0200, Michal Privoznik wrote:
--- src/lxc/lxc_conf.h | 2 +- src/lxc/lxc_process.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 6ca6198..f9a3e53 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -77,7 +77,7 @@ struct _virLXCDriver {
virSysinfoDefPtr hostsysinfo;
- size_t nactive; + unsigned int nactive;
virStateInhibitCallback inhibitCallback; void *inhibitOpaque; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 1024576..4b83729 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -48,6 +48,7 @@ #include "lxc_hostdev.h" #include "virhook.h" #include "virstring.h" +#include "viratomic.h"
#define VIR_FROM_THIS VIR_FROM_LXC
@@ -256,8 +257,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, vm->pid = -1; vm->def->id = -1;
- driver->nactive--; - if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque);
virLXCDomainReAttachHostDevices(driver, vm->def); @@ -1273,9 +1273,8 @@ int virLXCProcessStart(virConnectPtr conn, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason); priv->doneStopEvent = false;
- if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); - driver->nactive++;
I think you need virAtomicIntInc() == 1 here, otherwise this will execute on every single VM start, instead of only the first.
if (lxcContainerWaitForContinue(handshakefds[0]) < 0) { char out[1024]; @@ -1458,9 +1457,8 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN);
- if (!driver->nactive && driver->inhibitCallback) + if (virAtomicIntInc(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); - driver->nactive++;
Again here, we want == 1. virAtomicIntInc() == 1, is the pattern the QEMU driver uses for this code. 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 :|

Annotate the fields in virLXCDriverPtr to indicate the locking rules for their use --- src/lxc/lxc_conf.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index f9a3e53..831e3e5 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -67,29 +67,43 @@ struct _virLXCDriverConfig { struct _virLXCDriver { virMutex lock; + /* Require lock to get reference on 'config', + * then lockless thereafter */ virLXCDriverConfigPtr config; + /* Require lock while using. Unsafe. XXX */ virCapsPtr caps; + /* Immutable pointer. Unsafe APIs XXX */ virCgroupPtr cgroup; + /* Immutable pointer, Immutable object */ virDomainXMLOptionPtr xmlopt; + /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo; + /* Atomic inc/dec only */ unsigned int nactive; + /* Immutable pointers. Caller must provide locking */ virStateInhibitCallback inhibitCallback; void *inhibitOpaque; + /* Immutable pointer, self-locking APIs */ virDomainObjListPtr domains; + /* Immutable pointer. Requires lock to be held before + * calling APIs. */ virUSBDeviceListPtr activeUsbHostdevs; + /* Immutable pointer, self-locking APIs */ virDomainEventStatePtr domainEventState; + /* Immutable pointer. self-locking APIs */ virSecurityManagerPtr securityManager; + /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr autodestroy; }; -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:21PM +0200, Michal Privoznik wrote:
Annotate the fields in virLXCDriverPtr to indicate the locking rules for their use --- src/lxc/lxc_conf.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index f9a3e53..831e3e5 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -67,29 +67,43 @@ struct _virLXCDriverConfig { struct _virLXCDriver { virMutex lock;
+ /* Require lock to get reference on 'config', + * then lockless thereafter */ virLXCDriverConfigPtr config;
+ /* Require lock while using. Unsafe. XXX */ virCapsPtr caps;
+ /* Immutable pointer. Unsafe APIs XXX */ virCgroupPtr cgroup;
Oh this field is completely unused & should have been deleted a while back. Please just kill as a separate patch.
+ /* Immutable pointer, Immutable object */ virDomainXMLOptionPtr xmlopt;
+ /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo;
+ /* Atomic inc/dec only */ unsigned int nactive;
+ /* Immutable pointers. Caller must provide locking */ virStateInhibitCallback inhibitCallback; void *inhibitOpaque;
+ /* Immutable pointer, self-locking APIs */ virDomainObjListPtr domains;
+ /* Immutable pointer. Requires lock to be held before + * calling APIs. */ virUSBDeviceListPtr activeUsbHostdevs;
+ /* Immutable pointer, self-locking APIs */ virDomainEventStatePtr domainEventState;
+ /* Immutable pointer. self-locking APIs */ virSecurityManagerPtr securityManager;
+ /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr autodestroy; };
ACK, all makes sense. 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 :|

--- src/libvirt_private.syms | 1 + src/lxc/lxc_conf.h | 5 +- src/lxc/lxc_driver.c | 6 +-- src/lxc/lxc_process.c | 113 ++++++++----------------------------------- src/lxc/lxc_process.h | 1 - src/util/virclosecallbacks.c | 24 +++++++++ src/util/virclosecallbacks.h | 3 ++ 7 files changed, 53 insertions(+), 100 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 53b1153..f55ab57 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1206,6 +1206,7 @@ virCgroupSetMemSwapHardLimit; # util/virclosecallbacks.h virCloseCallbacksGet; +virCloseCallbacksGetConn; virCloseCallbacksNew; virCloseCallbacksRun; virCloseCallbacksSet; diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 831e3e5..6c81368 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -35,6 +35,7 @@ # include "vircgroup.h" # include "virsysinfo.h" # include "virusb.h" +# include "virclosecallbacks.h" # define LXC_DRIVER_NAME "LXC" @@ -103,8 +104,8 @@ struct _virLXCDriver { /* Immutable pointer. self-locking APIs */ virSecurityManagerPtr securityManager; - /* Immutable pointer. Unsafe APIs. XXX */ - virHashTablePtr autodestroy; + /* Immutable pointer, self-locking APIs */ + virCloseCallbacksPtr closeCallbacks; }; virLXCDriverConfigPtr virLXCDriverConfigNew(void); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 855dad3..60d21e3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -163,7 +163,7 @@ static int lxcConnectClose(virConnectPtr conn) virLXCDriverPtr driver = conn->privateData; lxcDriverLock(driver); - virLXCProcessAutoDestroyRun(driver, conn); + virCloseCallbacksRun(driver->closeCallbacks, conn, driver->domains, driver); lxcDriverUnlock(driver); conn->privateData = NULL; @@ -1563,7 +1563,7 @@ static int lxcStateInitialize(bool privileged, if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit())) goto cleanup; - if (virLXCProcessAutoDestroyInit(lxc_driver) < 0) + if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew())) goto cleanup; /* Get all the running persistent or transient configs first */ @@ -1653,7 +1653,7 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->domains); virDomainEventStateFree(lxc_driver->domainEventState); - virLXCProcessAutoDestroyShutdown(lxc_driver); + virObjectUnref(lxc_driver->closeCallbacks); virSysinfoDefFree(lxc_driver->hostsysinfo); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4b83729..931aee9 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -54,122 +54,45 @@ #define START_POSTFIX ": starting up\n" -int virLXCProcessAutoDestroyInit(virLXCDriverPtr driver) +static virDomainObjPtr +lxcProcessAutoDestroy(void *drv, + virDomainObjPtr dom, + virConnectPtr conn) { - if (!(driver->autodestroy = virHashCreate(5, NULL))) - return -1; - - return 0; -} - -struct virLXCProcessAutoDestroyData { - virLXCDriverPtr driver; - virConnectPtr conn; -}; - -static void virLXCProcessAutoDestroyDom(void *payload, - const void *name, - void *opaque) -{ - struct virLXCProcessAutoDestroyData *data = opaque; - virConnectPtr conn = payload; - const char *uuidstr = name; - unsigned char uuid[VIR_UUID_BUFLEN]; - virDomainObjPtr dom; + virLXCDriverPtr driver = drv; virDomainEventPtr event = NULL; virLXCDomainObjPrivatePtr priv; - VIR_DEBUG("conn=%p uuidstr=%s thisconn=%p", conn, uuidstr, data->conn); - - if (data->conn != conn) - return; - - if (virUUIDParse(uuidstr, uuid) < 0) { - VIR_WARN("Failed to parse %s", uuidstr); - return; - } - - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, - uuid))) { - VIR_DEBUG("No domain object to kill"); - return; - } + VIR_DEBUG("driver=%p dom=%s conn=%p", driver, dom->def->name, conn); priv = dom->privateData; VIR_DEBUG("Killing domain"); - virLXCProcessStop(data->driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED); + virLXCProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED); virDomainAuditStop(dom, "destroyed"); event = virDomainEventNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); priv->doneStopEvent = true; - if (dom && !dom->persistent) - virDomainObjListRemove(data->driver->domains, dom); + if (dom && !dom->persistent) { + virDomainObjListRemove(driver->domains, dom); + dom = NULL; + } - if (dom) - virObjectUnlock(dom); if (event) - virDomainEventStateQueue(data->driver->domainEventState, event); - virHashRemoveEntry(data->driver->autodestroy, uuidstr); + virDomainEventStateQueue(driver->domainEventState, event); + + return dom; } /* * Precondition: driver is locked */ -void virLXCProcessAutoDestroyRun(virLXCDriverPtr driver, virConnectPtr conn) -{ - struct virLXCProcessAutoDestroyData data = { - driver, conn - }; - VIR_DEBUG("conn=%p", conn); - virHashForEach(driver->autodestroy, virLXCProcessAutoDestroyDom, &data); -} - -void virLXCProcessAutoDestroyShutdown(virLXCDriverPtr driver) -{ - virHashFree(driver->autodestroy); -} - -int virLXCProcessAutoDestroyAdd(virLXCDriverPtr driver, - virDomainObjPtr vm, - virConnectPtr conn) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s uuid=%s conn=%p", vm->def->name, uuidstr, conn); - if (virHashAddEntry(driver->autodestroy, uuidstr, conn) < 0) - return -1; - return 0; -} - -int virLXCProcessAutoDestroyRemove(virLXCDriverPtr driver, - virDomainObjPtr vm) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s uuid=%s", vm->def->name, uuidstr); - if (virHashRemoveEntry(driver->autodestroy, uuidstr) < 0) - return -1; - return 0; -} - -static virConnectPtr -virLXCProcessAutoDestroyGetConn(virLXCDriverPtr driver, - virDomainObjPtr vm) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - VIR_DEBUG("vm=%s uuid=%s", vm->def->name, uuidstr); - return virHashLookup(driver->autodestroy, uuidstr); -} - - static int virLXCProcessReboot(virLXCDriverPtr driver, virDomainObjPtr vm) { - virConnectPtr conn = virLXCProcessAutoDestroyGetConn(driver, vm); + virConnectPtr conn = virCloseCallbacksGetConn(driver->closeCallbacks, vm); int reason = vm->state.reason; bool autodestroy = false; int ret = -1; @@ -242,7 +165,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, } /* Stop autodestroy in case guest is restarted */ - virLXCProcessAutoDestroyRemove(driver, vm); + virCloseCallbacksUnset(driver->closeCallbacks, vm, + lxcProcessAutoDestroy); if (priv->monitor) { virLXCMonitorClose(priv->monitor); @@ -1288,7 +1212,8 @@ int virLXCProcessStart(virConnectPtr conn, } if (autoDestroy && - virLXCProcessAutoDestroyAdd(driver, vm, conn) < 0) + virCloseCallbacksSet(driver->closeCallbacks, vm, + conn, lxcProcessAutoDestroy) < 0) goto error; if (virDomainObjSetDefTransient(driver->caps, driver->xmlopt, diff --git a/src/lxc/lxc_process.h b/src/lxc/lxc_process.h index 779cc5f..4403ae8 100644 --- a/src/lxc/lxc_process.h +++ b/src/lxc/lxc_process.h @@ -33,7 +33,6 @@ int virLXCProcessStop(virLXCDriverPtr driver, virDomainObjPtr vm, virDomainShutoffReason reason); -int virLXCProcessAutoDestroyInit(virLXCDriverPtr driver); void virLXCProcessAutoDestroyRun(virLXCDriverPtr driver, virConnectPtr conn); void virLXCProcessAutoDestroyShutdown(virLXCDriverPtr driver); diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index a926456..1de822d 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -208,6 +208,30 @@ virCloseCallbacksGet(virCloseCallbacksPtr closeCallbacks, return cb; } +virConnectPtr +virCloseCallbacksGetConn(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDriverCloseDefPtr closeDef; + virConnectPtr conn = NULL; + + virUUIDFormat(vm->def->uuid, uuidstr); + VIR_DEBUG("vm=%s, uuid=%s", vm->def->name, uuidstr); + + virObjectLock(closeCallbacks); + + closeDef = virHashLookup(closeCallbacks->list, uuidstr); + if (closeDef) + conn = closeDef->conn; + + virObjectUnlock(closeCallbacks); + + VIR_DEBUG("conn=%p", conn); + return conn; +} + + typedef struct _virCloseCallbacksListEntry virCloseCallbacksListEntry; typedef virCloseCallbacksListEntry *virCloseCallbacksListEntryPtr; struct _virCloseCallbacksListEntry { diff --git a/src/util/virclosecallbacks.h b/src/util/virclosecallbacks.h index 9db641f..ca02e5c 100644 --- a/src/util/virclosecallbacks.h +++ b/src/util/virclosecallbacks.h @@ -45,6 +45,9 @@ virCloseCallback virCloseCallbacksGet(virCloseCallbacksPtr closeCallbacks, virDomainObjPtr vm, virConnectPtr conn); +virConnectPtr +virCloseCallbacksGetConn(virCloseCallbacksPtr closeCallbacks, + virDomainObjPtr vm); void virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, virConnectPtr conn, -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:22PM +0200, Michal Privoznik wrote:
--- src/libvirt_private.syms | 1 + src/lxc/lxc_conf.h | 5 +- src/lxc/lxc_driver.c | 6 +-- src/lxc/lxc_process.c | 113 ++++++++----------------------------------- src/lxc/lxc_process.h | 1 - src/util/virclosecallbacks.c | 24 +++++++++ src/util/virclosecallbacks.h | 3 ++ 7 files changed, 53 insertions(+), 100 deletions(-)
ACK 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 :|

The 'driver->caps' pointer can be changed on the fly. Accessing it currently requires the global driver lock. Isolate this access in a single helper, so a future patch can relax the locking constraints. --- src/lxc/lxc_conf.c | 29 +++++++++++- src/lxc/lxc_conf.h | 7 ++- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 121 +++++++++++++++++++++++++++++++++++++---------- src/lxc/lxc_process.c | 9 +++- 5 files changed, 136 insertions(+), 32 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 4e97f10..6739df9 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -59,7 +59,7 @@ VIR_ONCE_GLOBAL_INIT(virLXCConfig) /* Functions */ -virCapsPtr lxcCapsInit(virLXCDriverPtr driver) +virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver) { virCapsPtr caps; virCapsGuestPtr guest; @@ -153,6 +153,33 @@ error: } +/** + * virLXCDriverGetCapabilities: + * + * Get a reference to the virCapsPtr instance for the + * driver. If @refresh is true, the capabilities will be + * rebuilt first + * + * The caller must release the reference with virObjetUnref + * + * Returns: a reference to a virCapsPtr instance or NULL + */ +virCapsPtr virLXCDriverGetCapabilities(virLXCDriverPtr driver, + bool refresh) +{ + if (refresh) { + virCapsPtr caps = NULL; + if ((caps = virLXCDriverCapsInit(driver)) == NULL) + return NULL; + + virObjectUnref(driver->caps); + driver->caps = caps; + } + + return virObjectRef(driver->caps); +} + + virDomainXMLOptionPtr lxcDomainXMLConfInit(void) { diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 6c81368..f8caebe 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -72,7 +72,8 @@ struct _virLXCDriver { * then lockless thereafter */ virLXCDriverConfigPtr config; - /* Require lock while using. Unsafe. XXX */ + /* Require lock to get a reference on the object, + * lockless access thereafter */ virCapsPtr caps; /* Immutable pointer. Unsafe APIs XXX */ @@ -112,7 +113,9 @@ virLXCDriverConfigPtr virLXCDriverConfigNew(void); virLXCDriverConfigPtr virLXCDriverGetConfig(virLXCDriverPtr driver); int virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, const char *filename); -virCapsPtr lxcCapsInit(virLXCDriverPtr driver); +virCapsPtr virLXCDriverCapsInit(virLXCDriverPtr driver); +virCapsPtr virLXCDriverGetCapabilities(virLXCDriverPtr driver, + bool refresh); virDomainXMLOptionPtr lxcDomainXMLConfInit(void); static inline void lxcDriverLock(virLXCDriverPtr driver) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index ce1f941..f86b0b2 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -159,7 +159,7 @@ static virLXCControllerPtr virLXCControllerNew(const char *name) if (VIR_STRDUP(ctrl->name, name) < 0) goto error; - if ((caps = lxcCapsInit(NULL)) == NULL) + if (!(caps = virLXCDriverCapsInit(NULL))) goto error; if (!(xmlopt = lxcDomainXMLConfInit())) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 60d21e3..df9fb92 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -193,16 +193,23 @@ static int lxcConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) static char *lxcConnectGetCapabilities(virConnectPtr conn) { virLXCDriverPtr driver = conn->privateData; + virCapsPtr caps; char *xml; if (virConnectGetCapabilitiesEnsureACL(conn) < 0) return NULL; lxcDriverLock(driver); - if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL) + if (!(caps = virLXCDriverGetCapabilities(driver, false))) { + lxcDriverUnlock(driver); + return NULL; + } + + if ((xml = virCapabilitiesFormatXML(caps)) == NULL) virReportOOMError(); lxcDriverUnlock(driver); + virObjectUnref(caps); return xml; } @@ -457,11 +464,15 @@ static virDomainPtr lxcDomainDefineXML(virConnectPtr conn, const char *xml) virDomainEventPtr event = NULL; virDomainDefPtr oldDef = NULL; virLXCDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; lxcDriverLock(driver); cfg = virLXCDriverGetConfig(driver); - if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(xml, caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; @@ -509,6 +520,7 @@ cleanup: virObjectUnlock(vm); if (event) virDomainEventStateQueue(driver->domainEventState, event); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return dom; @@ -1119,17 +1131,21 @@ lxcDomainCreateXML(virConnectPtr conn, { virLXCDriverPtr driver = conn->privateData; virDomainObjPtr vm = NULL; - virDomainDefPtr def; + virDomainDefPtr def = NULL; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; virLXCDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); lxcDriverLock(driver); cfg = virLXCDriverGetConfig(driver); - if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (!(def = virDomainDefParseString(xml, caps, driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; @@ -1178,6 +1194,7 @@ cleanup: virObjectUnlock(vm); if (event) virDomainEventStateQueue(driver->domainEventState, event); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return dom; @@ -1257,6 +1274,7 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) { virLXCDriverPtr driver = conn->privateData; + virCapsPtr caps = NULL; int ret = 0; lxcDriverLock(driver); @@ -1265,12 +1283,15 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, if (virNodeGetSecurityModelEnsureACL(conn) < 0) goto cleanup; + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + /* we treat no driver as success, but simply return no data in *secmodel */ - if (driver->caps->host.nsecModels == 0 - || driver->caps->host.secModels[0].model == NULL) + if (caps->host.nsecModels == 0 + || caps->host.secModels[0].model == NULL) goto cleanup; - if (!virStrcpy(secmodel->model, driver->caps->host.secModels[0].model, + if (!virStrcpy(secmodel->model, caps->host.secModels[0].model, VIR_SECURITY_MODEL_BUFLEN)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security model string exceeds max %d bytes"), @@ -1279,7 +1300,7 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, goto cleanup; } - if (!virStrcpy(secmodel->doi, driver->caps->host.secModels[0].doi, + if (!virStrcpy(secmodel->doi, caps->host.secModels[0].doi, VIR_SECURITY_DOI_BUFLEN)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security DOI string exceeds max %d bytes"), @@ -1289,6 +1310,7 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, } cleanup: + virObjectUnref(caps); lxcDriverUnlock(driver); return ret; } @@ -1498,6 +1520,7 @@ static int lxcStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { + virCapsPtr caps = NULL; char *ld; virLXCDriverConfigPtr cfg = NULL; @@ -1557,7 +1580,7 @@ static int lxcStateInitialize(bool privileged, if ((lxc_driver->activeUsbHostdevs = virUSBDeviceListNew()) == NULL) goto cleanup; - if ((lxc_driver->caps = lxcCapsInit(lxc_driver)) == NULL) + if ((virLXCDriverGetCapabilities(lxc_driver, true)) == NULL) goto cleanup; if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit())) @@ -1566,11 +1589,14 @@ static int lxcStateInitialize(bool privileged, if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew())) goto cleanup; + if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false))) + goto cleanup; + /* Get all the running persistent or transient configs first */ if (virDomainObjListLoadAllConfigs(lxc_driver->domains, cfg->stateDir, NULL, 1, - lxc_driver->caps, + caps, lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, NULL, NULL) < 0) @@ -1582,7 +1608,7 @@ static int lxcStateInitialize(bool privileged, if (virDomainObjListLoadAllConfigs(lxc_driver->domains, cfg->configDir, cfg->autostartDir, 0, - lxc_driver->caps, + caps, lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, NULL, NULL) < 0) @@ -1596,6 +1622,7 @@ static int lxcStateInitialize(bool privileged, return 0; cleanup: + virObjectUnref(caps); lxcDriverUnlock(lxc_driver); lxcStateCleanup(); return -1; @@ -1624,21 +1651,28 @@ static void lxcNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque) static int lxcStateReload(void) { virLXCDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; if (!lxc_driver) return 0; lxcDriverLock(lxc_driver); + if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false))) { + lxcDriverUnlock(lxc_driver); + return -1; + } + cfg = virLXCDriverGetConfig(lxc_driver); virDomainObjListLoadAllConfigs(lxc_driver->domains, cfg->configDir, cfg->autostartDir, 0, - lxc_driver->caps, + caps, lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, lxcNotifyLoadDomain, lxc_driver); lxcDriverUnlock(lxc_driver); + virObjectUnref(caps); virObjectUnref(cfg); return 0; } @@ -1866,6 +1900,7 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; size_t i; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; @@ -1902,13 +1937,16 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &vmdef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; } @@ -1987,6 +2025,7 @@ cleanup: virDomainDefFree(vmdef); if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; @@ -2007,6 +2046,7 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef; unsigned long long shares = 0; @@ -2042,7 +2082,10 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, cpu_bw_status = !!rc; } - if (virDomainLiveConfigHelperMethod(driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &persistentDef) < 0) goto cleanup; @@ -2105,6 +2148,7 @@ out: cleanup: if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); return ret; } @@ -2125,6 +2169,7 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; size_t i; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -2156,7 +2201,10 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, if (virDomainSetBlkioParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &persistentDef) < 0) goto cleanup; @@ -2214,6 +2262,7 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; @@ -2228,6 +2277,7 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; size_t i; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -2259,7 +2309,10 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, driver->xmlopt, + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, &persistentDef) < 0) goto cleanup; @@ -2320,6 +2373,7 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); return ret; } @@ -4320,6 +4374,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; @@ -4363,6 +4418,9 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, } } + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify device on transient domain")); @@ -4370,7 +4428,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, } dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - driver->caps, driver->xmlopt, + caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -4382,7 +4440,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, * to CONFIG takes one instance. */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, - driver->caps, driver->xmlopt); + caps, driver->xmlopt); if (!dev_copy) goto cleanup; } @@ -4392,7 +4450,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, goto cleanup; /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) @@ -4432,6 +4490,7 @@ cleanup: virDomainDeviceDefFree(dev); if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; @@ -4451,6 +4510,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; @@ -4501,8 +4561,11 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, goto cleanup; } + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - driver->caps, driver->xmlopt, + caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -4514,7 +4577,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, * to CONFIG takes one instance. */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, - driver->caps, driver->xmlopt); + caps, driver->xmlopt); if (!dev_copy) goto cleanup; } @@ -4524,7 +4587,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, goto cleanup; /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) @@ -4557,6 +4620,7 @@ cleanup: virDomainDeviceDefFree(dev); if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; @@ -4568,6 +4632,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, unsigned int flags) { virLXCDriverPtr driver = dom->conn->privateData; + virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; @@ -4617,8 +4682,11 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, goto cleanup; } + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - driver->caps, driver->xmlopt, + caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE); if (dev == NULL) goto cleanup; @@ -4630,7 +4698,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, * to CONFIG takes one instance. */ dev_copy = virDomainDeviceDefCopy(dev, vm->def, - driver->caps, driver->xmlopt); + caps, driver->xmlopt); if (!dev_copy) goto cleanup; } @@ -4640,7 +4708,7 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, goto cleanup; /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, driver->caps, driver->xmlopt); + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; @@ -4681,6 +4749,7 @@ cleanup: virDomainDeviceDefFree(dev); if (vm) virObjectUnlock(vm); + virObjectUnref(caps); lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 931aee9..3c4a71a 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -976,6 +976,7 @@ int virLXCProcessStart(virConnectPtr conn, char *timestamp; virCommandPtr cmd = NULL; virLXCDomainObjPrivatePtr priv = vm->privateData; + virCapsPtr caps = NULL; virErrorPtr err = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); @@ -1017,12 +1018,15 @@ int virLXCProcessStart(virConnectPtr conn, cfg->logDir, vm->def->name) < 0) return -1; + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto cleanup; + /* Do this up front, so any part of the startup process can add * runtime state to vm->def that won't be persisted. This let's us * report implicit runtime defaults in the XML, like vnc listen/socket */ VIR_DEBUG("Setting current domain def as transient"); - if (virDomainObjSetDefTransient(driver->caps, driver->xmlopt, vm, true) < 0) + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, true) < 0) goto cleanup; /* Run an early hook to set-up missing devices */ @@ -1216,7 +1220,7 @@ int virLXCProcessStart(virConnectPtr conn, conn, lxcProcessAutoDestroy) < 0) goto error; - if (virDomainObjSetDefTransient(driver->caps, driver->xmlopt, + if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, false) < 0) goto error; @@ -1290,6 +1294,7 @@ cleanup: VIR_FORCE_CLOSE(handshakefds[1]); VIR_FREE(logfile); virObjectUnref(cfg); + virObjectUnref(caps); if (err) { virSetError(err); -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:23PM +0200, Michal Privoznik wrote:
The 'driver->caps' pointer can be changed on the fly. Accessing it currently requires the global driver lock. Isolate this access in a single helper, so a future patch can relax the locking constraints. --- src/lxc/lxc_conf.c | 29 +++++++++++- src/lxc/lxc_conf.h | 7 ++- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 121 +++++++++++++++++++++++++++++++++++++---------- src/lxc/lxc_process.c | 9 +++- 5 files changed, 136 insertions(+), 32 deletions(-)
ACK 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 :|

The activeUsbHostdevs item in LXCDriver are lockable, but the lock has to be called explicitly. Call the virObject(Un)Lock() in order to achieve mutual exclusion once lxcDriverLock is removed. --- src/lxc/lxc_driver.c | 2 ++ src/lxc/lxc_hostdev.c | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index df9fb92..1210e77 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4139,7 +4139,9 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, VIR_WARN("cannot deny device %s for domain %s", dst, vm->def->name); + virObjectLock(driver->activeUsbHostdevs); virUSBDeviceListDel(driver->activeUsbHostdevs, usb); + virObjectUnlock(driver->activeUsbHostdevs); virDomainHostdevRemove(vm->def, idx); virDomainHostdevDefFree(def); diff --git a/src/lxc/lxc_hostdev.c b/src/lxc/lxc_hostdev.c index 257e93b..3b371fc 100644 --- a/src/lxc/lxc_hostdev.c +++ b/src/lxc/lxc_hostdev.c @@ -62,10 +62,13 @@ virLXCUpdateActiveUsbHostdevs(virLXCDriverPtr driver, virUSBDeviceSetUsedBy(usb, def->name); + virObjectLock(driver->activeUsbHostdevs); if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) { + virObjectUnlock(driver->activeUsbHostdevs); virUSBDeviceFree(usb); return -1; } + virObjectUnlock(driver->activeUsbHostdevs); } return 0; @@ -83,6 +86,7 @@ virLXCPrepareHostdevUSBDevices(virLXCDriverPtr driver, count = virUSBDeviceListCount(list); + virObjectLock(driver->activeUsbHostdevs); for (i = 0; i < count; i++) { virUSBDevicePtr usb = virUSBDeviceListGet(list, i); if ((tmp = virUSBDeviceListFind(driver->activeUsbHostdevs, usb))) { @@ -110,6 +114,7 @@ virLXCPrepareHostdevUSBDevices(virLXCDriverPtr driver, if (virUSBDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) goto error; } + virObjectUnlock(driver->activeUsbHostdevs); return 0; error: @@ -117,6 +122,7 @@ error: tmp = virUSBDeviceListGet(list, i); virUSBDeviceListSteal(driver->activeUsbHostdevs, tmp); } + virObjectUnlock(driver->activeUsbHostdevs); return -1; } @@ -341,6 +347,7 @@ virLXCDomainReAttachHostUsbDevices(virLXCDriverPtr driver, { size_t i; + virObjectLock(driver->activeUsbHostdevs); for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virUSBDevicePtr usb, tmp; @@ -392,6 +399,7 @@ virLXCDomainReAttachHostUsbDevices(virLXCDriverPtr driver, virUSBDeviceListDel(driver->activeUsbHostdevs, tmp); } } + virObjectUnlock(driver->activeUsbHostdevs); } void virLXCDomainReAttachHostDevices(virLXCDriverPtr driver, -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:24PM +0200, Michal Privoznik wrote:
The activeUsbHostdevs item in LXCDriver are lockable, but the lock has to be called explicitly. Call the virObject(Un)Lock() in order to achieve mutual exclusion once lxcDriverLock is removed. --- src/lxc/lxc_driver.c | 2 ++ src/lxc/lxc_hostdev.c | 8 ++++++++ 2 files changed, 10 insertions(+)
ACK 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 :|

With the majority of fields in the virLXCDriverPtr struct now immutable or self-locking, there is no need for practically any methods to be using the LXC driver lock. Only a handful of helper APIs now need it. --- src/lxc/lxc_conf.c | 14 ++++- src/lxc/lxc_conf.h | 3 - src/lxc/lxc_driver.c | 171 +++++--------------------------------------------- src/lxc/lxc_process.c | 14 +---- 4 files changed, 27 insertions(+), 175 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 6739df9..c1cee3f 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -167,16 +167,22 @@ error: virCapsPtr virLXCDriverGetCapabilities(virLXCDriverPtr driver, bool refresh) { + virCapsPtr ret; if (refresh) { virCapsPtr caps = NULL; if ((caps = virLXCDriverCapsInit(driver)) == NULL) return NULL; + lxcDriverLock(driver); virObjectUnref(driver->caps); driver->caps = caps; + } else { + lxcDriverLock(driver); } - return virObjectRef(driver->caps); + ret = virObjectRef(driver->caps); + lxcDriverUnlock(driver); + return ret; } @@ -273,7 +279,11 @@ done: virLXCDriverConfigPtr virLXCDriverGetConfig(virLXCDriverPtr driver) { - return virObjectRef(driver->config); + virLXCDriverConfigPtr cfg; + lxcDriverLock(driver); + cfg = virObjectRef(driver->config); + lxcDriverUnlock(driver); + return cfg; } static void diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index f8caebe..a6208a2 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -76,9 +76,6 @@ struct _virLXCDriver { * lockless access thereafter */ virCapsPtr caps; - /* Immutable pointer. Unsafe APIs XXX */ - virCgroupPtr cgroup; - /* Immutable pointer, Immutable object */ virDomainXMLOptionPtr xmlopt; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1210e77..92f0d15 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -162,10 +162,7 @@ static int lxcConnectClose(virConnectPtr conn) { virLXCDriverPtr driver = conn->privateData; - lxcDriverLock(driver); virCloseCallbacksRun(driver->closeCallbacks, conn, driver->domains, driver); - lxcDriverUnlock(driver); - conn->privateData = NULL; return 0; } @@ -199,15 +196,11 @@ static char *lxcConnectGetCapabilities(virConnectPtr conn) { if (virConnectGetCapabilitiesEnsureACL(conn) < 0) return NULL; - lxcDriverLock(driver); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) { - lxcDriverUnlock(driver); + if (!(caps = virLXCDriverGetCapabilities(driver, false))) return NULL; - } if ((xml = virCapabilitiesFormatXML(caps)) == NULL) virReportOOMError(); - lxcDriverUnlock(driver); virObjectUnref(caps); return xml; @@ -221,9 +214,7 @@ static virDomainPtr lxcDomainLookupByID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - lxcDriverLock(driver); vm = virDomainObjListFindByID(driver->domains, id); - lxcDriverUnlock(driver); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -251,9 +242,7 @@ static virDomainPtr lxcDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -283,9 +272,7 @@ static virDomainPtr lxcDomainLookupByName(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - lxcDriverLock(driver); vm = virDomainObjListFindByName(driver->domains, name); - lxcDriverUnlock(driver); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with matching name '%s'"), name); @@ -312,9 +299,7 @@ static int lxcDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - lxcDriverLock(driver); obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -341,9 +326,7 @@ static int lxcDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - lxcDriverLock(driver); obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -369,9 +352,7 @@ static int lxcDomainIsUpdated(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - lxcDriverLock(driver); obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -398,10 +379,8 @@ static int lxcConnectListDomains(virConnectPtr conn, int *ids, int nids) { if (virConnectListDomainsEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); n = virDomainObjListGetActiveIDs(driver->domains, ids, nids, virConnectListDomainsCheckACL, conn); - lxcDriverUnlock(driver); return n; } @@ -413,10 +392,8 @@ static int lxcConnectNumOfDomains(virConnectPtr conn) { if (virConnectNumOfDomainsEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); n = virDomainObjListNumOfDomains(driver->domains, true, virConnectNumOfDomainsCheckACL, conn); - lxcDriverUnlock(driver); return n; } @@ -429,10 +406,8 @@ static int lxcConnectListDefinedDomains(virConnectPtr conn, if (virConnectListDefinedDomainsEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); n = virDomainObjListGetInactiveNames(driver->domains, names, nnames, virConnectListDefinedDomainsCheckACL, conn); - lxcDriverUnlock(driver); return n; } @@ -445,10 +420,8 @@ static int lxcConnectNumOfDefinedDomains(virConnectPtr conn) { if (virConnectNumOfDefinedDomainsEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); n = virDomainObjListNumOfDomains(driver->domains, false, virConnectNumOfDefinedDomainsCheckACL, conn); - lxcDriverUnlock(driver); return n; } @@ -463,12 +436,9 @@ static virDomainPtr lxcDomainDefineXML(virConnectPtr conn, const char *xml) virDomainPtr dom = NULL; virDomainEventPtr event = NULL; virDomainDefPtr oldDef = NULL; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -521,7 +491,6 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return dom; } @@ -533,13 +502,10 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(0, -1); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -581,7 +547,6 @@ cleanup: virObjectUnlock(vm); if (event) virDomainEventStateQueue(driver->domainEventState, event); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -599,7 +564,6 @@ static int lxcDomainGetInfo(virDomainPtr dom, int ret = -1, rc; virLXCDomainObjPrivatePtr priv; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -643,7 +607,6 @@ static int lxcDomainGetInfo(virDomainPtr dom, ret = 0; cleanup: - lxcDriverUnlock(driver); if (vm) virObjectUnlock(vm); return ret; @@ -661,9 +624,7 @@ lxcDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -691,9 +652,7 @@ static char *lxcDomainGetOSType(virDomainPtr dom) virDomainObjPtr vm; char *ret = NULL; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -723,9 +682,7 @@ lxcDomainGetMaxMemory(virDomainPtr dom) virDomainObjPtr vm; unsigned long long ret = 0; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -751,9 +708,7 @@ static int lxcDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) { virDomainObjPtr vm; int ret = -1; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -787,9 +742,7 @@ static int lxcDomainSetMemory(virDomainPtr dom, unsigned long newmem) { int ret = -1; virLXCDomainObjPrivatePtr priv; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -852,7 +805,6 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, NULL) < 0) return -1; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -898,7 +850,6 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); return ret; } @@ -918,7 +869,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, virCheckFlags(0, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -994,7 +944,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); return ret; } @@ -1007,9 +956,7 @@ static char *lxcDomainGetXMLDesc(virDomainPtr dom, /* Flags checked by virDomainDefFormat */ - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1047,13 +994,10 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1096,7 +1040,6 @@ cleanup: virObjectUnlock(vm); if (event) virDomainEventStateQueue(driver->domainEventState, event); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -1134,14 +1077,11 @@ lxcDomainCreateXML(virConnectPtr conn, virDomainDefPtr def = NULL; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1195,7 +1135,6 @@ cleanup: if (event) virDomainEventStateQueue(driver->domainEventState, event); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return dom; } @@ -1207,7 +1146,6 @@ static int lxcDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr secla virDomainObjPtr vm; int ret = -1; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); memset(seclabel, 0, sizeof(*seclabel)); @@ -1266,7 +1204,6 @@ static int lxcDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr secla cleanup: if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); return ret; } @@ -1277,7 +1214,6 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, virCapsPtr caps = NULL; int ret = 0; - lxcDriverLock(driver); memset(secmodel, 0, sizeof(*secmodel)); if (virNodeGetSecurityModelEnsureACL(conn) < 0) @@ -1311,7 +1247,6 @@ static int lxcNodeGetSecurityModel(virConnectPtr conn, cleanup: virObjectUnref(caps); - lxcDriverUnlock(driver); return ret; } @@ -1328,11 +1263,9 @@ lxcConnectDomainEventRegister(virConnectPtr conn, if (virConnectDomainEventRegisterEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); ret = virDomainEventStateRegister(conn, driver->domainEventState, callback, opaque, freecb); - lxcDriverUnlock(driver); return ret; } @@ -1348,11 +1281,9 @@ lxcConnectDomainEventDeregister(virConnectPtr conn, if (virConnectDomainEventDeregisterEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); ret = virDomainEventStateDeregister(conn, driver->domainEventState, callback); - lxcDriverUnlock(driver); return ret; } @@ -1372,13 +1303,11 @@ lxcConnectDomainEventRegisterAny(virConnectPtr conn, if (virConnectDomainEventRegisterAnyEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); if (virDomainEventStateRegisterID(conn, driver->domainEventState, dom, eventID, callback, opaque, freecb, &ret) < 0) ret = -1; - lxcDriverUnlock(driver); return ret; } @@ -1394,11 +1323,9 @@ lxcConnectDomainEventDeregisterAny(virConnectPtr conn, if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); ret = virDomainEventStateDeregisterID(conn, driver->domainEventState, callbackID); - lxcDriverUnlock(driver); return ret; } @@ -1425,7 +1352,6 @@ lxcDomainDestroyFlags(virDomainPtr dom, virCheckFlags(0, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1461,7 +1387,6 @@ cleanup: virObjectUnlock(vm); if (event) virDomainEventStateQueue(driver->domainEventState, event); - lxcDriverUnlock(driver); return ret; } @@ -1553,7 +1478,6 @@ static int lxcStateInitialize(bool privileged, VIR_FREE(lxc_driver); return -1; } - lxcDriverLock(lxc_driver); if (!(lxc_driver->domains = virDomainObjListNew())) goto cleanup; @@ -1614,8 +1538,6 @@ static int lxcStateInitialize(bool privileged, NULL, NULL) < 0) goto cleanup; - lxcDriverUnlock(lxc_driver); - virLXCProcessAutostartAll(lxc_driver); virNWFilterRegisterCallbackDriver(&lxcCallbackDriver); @@ -1623,7 +1545,6 @@ static int lxcStateInitialize(bool privileged, cleanup: virObjectUnref(caps); - lxcDriverUnlock(lxc_driver); lxcStateCleanup(); return -1; } @@ -1656,11 +1577,8 @@ lxcStateReload(void) { if (!lxc_driver) return 0; - lxcDriverLock(lxc_driver); - if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false))) { - lxcDriverUnlock(lxc_driver); + if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false))) return -1; - } cfg = virLXCDriverGetConfig(lxc_driver); @@ -1671,7 +1589,6 @@ lxcStateReload(void) { lxc_driver->xmlopt, 1 << VIR_DOMAIN_VIRT_LXC, lxcNotifyLoadDomain, lxc_driver); - lxcDriverUnlock(lxc_driver); virObjectUnref(caps); virObjectUnref(cfg); return 0; @@ -1682,7 +1599,6 @@ static int lxcStateCleanup(void) if (lxc_driver == NULL) return -1; - lxcDriverLock(lxc_driver); virNWFilterUnRegisterCallbackDriver(&lxcCallbackDriver); virObjectUnref(lxc_driver->domains); virDomainEventStateFree(lxc_driver->domainEventState); @@ -1696,7 +1612,6 @@ static int lxcStateCleanup(void) virObjectUnref(lxc_driver->securityManager); virObjectUnref(lxc_driver->xmlopt); virObjectUnref(lxc_driver->config); - lxcDriverUnlock(lxc_driver); virMutexDestroy(&lxc_driver->lock); VIR_FREE(lxc_driver); @@ -1774,7 +1689,6 @@ static char *lxcDomainGetSchedulerType(virDomainPtr dom, virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1815,7 +1729,6 @@ static char *lxcDomainGetSchedulerType(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); return ret; } @@ -1907,7 +1820,7 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, int ret = -1; int rc; virLXCDomainObjPrivatePtr priv; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -1921,10 +1834,6 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, NULL) < 0) return -1; - lxcDriverLock(driver); - - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -2026,7 +1935,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -2061,8 +1969,6 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - lxcDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -2149,7 +2055,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); return ret; } @@ -2175,7 +2080,7 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; virLXCDomainObjPrivatePtr priv; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -2185,10 +2090,6 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, NULL) < 0) return -1; - lxcDriverLock(driver); - - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (vm == NULL) { @@ -2263,7 +2164,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -2288,7 +2188,6 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2374,7 +2273,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); return ret; } @@ -2390,9 +2288,7 @@ lxcDomainInterfaceStats(virDomainPtr dom, size_t i; int ret = -1; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -2448,9 +2344,7 @@ static int lxcDomainGetAutostart(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -2479,10 +2373,7 @@ static int lxcDomainSetAutostart(virDomainPtr dom, virDomainObjPtr vm; char *configFile = NULL, *autostartLink = NULL; int ret = -1; - virLXCDriverConfigPtr cfg = NULL; - - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2550,7 +2441,6 @@ cleanup: VIR_FREE(autostartLink); if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -2647,10 +2537,7 @@ static int lxcDomainSuspend(virDomainPtr dom) virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; - virLXCDriverConfigPtr cfg = NULL; - - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2693,7 +2580,6 @@ cleanup: virDomainEventStateQueue(driver->domainEventState, event); if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -2705,10 +2591,7 @@ static int lxcDomainResume(virDomainPtr dom) virDomainEventPtr event = NULL; int ret = -1; virLXCDomainObjPrivatePtr priv; - virLXCDriverConfigPtr cfg = NULL; - - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); @@ -2754,7 +2637,6 @@ cleanup: virDomainEventStateQueue(driver->domainEventState, event); if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -2774,7 +2656,6 @@ lxcDomainOpenConsole(virDomainPtr dom, virCheckFlags(0, -1); - lxcDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -2828,7 +2709,6 @@ lxcDomainOpenConsole(virDomainPtr dom, cleanup: if (vm) virObjectUnlock(vm); - lxcDriverUnlock(driver); return ret; } @@ -2855,10 +2735,8 @@ lxcDomainSendProcessSignal(virDomainPtr dom, return -1; } - lxcDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); @@ -2927,11 +2805,8 @@ lxcConnectListAllDomains(virConnectPtr conn, if (virConnectListAllDomainsEnsureACL(conn) < 0) return -1; - lxcDriverLock(driver); ret = virDomainObjListExport(driver->domains, conn, domains, virConnectListAllDomainsCheckACL, flags); - lxcDriverUnlock(driver); - return ret; } @@ -2950,9 +2825,7 @@ lxcDomainShutdownFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_SHUTDOWN_INITCTL | VIR_DOMAIN_SHUTDOWN_SIGNAL, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -3040,9 +2913,7 @@ lxcDomainReboot(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | VIR_DOMAIN_REBOOT_SIGNAL, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -4382,16 +4253,13 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4493,7 +4361,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -4518,7 +4385,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -4526,9 +4393,6 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4623,7 +4487,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -4640,16 +4503,13 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; unsigned int affect; - virLXCDriverConfigPtr cfg = NULL; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - lxcDriverLock(driver); - cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { @@ -4752,7 +4612,6 @@ cleanup: if (vm) virObjectUnlock(vm); virObjectUnref(caps); - lxcDriverUnlock(driver); virObjectUnref(cfg); return ret; } @@ -4779,9 +4638,7 @@ static int lxcDomainLxcOpenNamespace(virDomainPtr dom, *fdlist = NULL; virCheckFlags(0, -1); - lxcDriverLock(driver); vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - lxcDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 3c4a71a..91f0683 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -486,9 +486,7 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, VIR_DEBUG("mon=%p vm=%p", mon, vm); - lxcDriverLock(driver); virObjectLock(vm); - lxcDriverUnlock(driver); priv = vm->privateData; virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); @@ -526,9 +524,7 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, if (vm) virObjectUnlock(vm); if (event) { - lxcDriverLock(driver); virDomainEventStateQueue(driver->domainEventState, event); - lxcDriverUnlock(driver); } } @@ -536,12 +532,9 @@ static void virLXCProcessMonitorExitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED virLXCMonitorExitStatus status, virDomainObjPtr vm) { - virLXCDriverPtr driver = lxc_driver; virLXCDomainObjPrivatePtr priv = vm->privateData; - lxcDriverLock(driver); virObjectLock(vm); - lxcDriverUnlock(driver); switch (status) { case VIR_LXC_MONITOR_EXIT_STATUS_SHUTDOWN: @@ -600,13 +593,10 @@ static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED { virLXCDriverPtr driver = lxc_driver; virLXCDomainObjPrivatePtr priv; - virLXCDriverConfigPtr cfg; + virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); ino_t inode; - lxcDriverLock(driver); virObjectLock(vm); - cfg = virLXCDriverGetConfig(driver); - lxcDriverUnlock(driver); priv = vm->privateData; priv->initpid = initpid; @@ -1359,11 +1349,9 @@ virLXCProcessAutostartAll(virLXCDriverPtr driver) struct virLXCProcessAutostartData data = { driver, conn }; - lxcDriverLock(driver); virDomainObjListForEach(driver->domains, virLXCProcessAutostartDomain, &data); - lxcDriverUnlock(driver); if (conn) virConnectClose(conn); -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:25PM +0200, Michal Privoznik wrote:
With the majority of fields in the virLXCDriverPtr struct now immutable or self-locking, there is no need for practically any methods to be using the LXC driver lock. Only a handful of helper APIs now need it. --- src/lxc/lxc_conf.c | 14 ++++- src/lxc/lxc_conf.h | 3 - src/lxc/lxc_driver.c | 171 +++++--------------------------------------------- src/lxc/lxc_process.c | 14 +---- 4 files changed, 27 insertions(+), 175 deletions(-)
ACK
diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index f8caebe..a6208a2 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -76,9 +76,6 @@ struct _virLXCDriver { * lockless access thereafter */ virCapsPtr caps;
- /* Immutable pointer. Unsafe APIs XXX */ - virCgroupPtr cgroup; - /* Immutable pointer, Immutable object */ virDomainXMLOptionPtr xmlopt;
This ought to be in a dedicated patch. 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 :|

Similarly to qemu driver, we can use a helper function to lookup a domain instead of copying multiple lines around. --- src/lxc/lxc_driver.c | 344 +++++++++++---------------------------------------- 1 file changed, 73 insertions(+), 271 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 92f0d15..0c92de5 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -109,6 +109,35 @@ static virNWFilterCallbackDriver lxcCallbackDriver = { .vmDriverUnlock = lxcVMDriverUnlock, }; +/** + * lxcDomObjFromDomain: + * @domain: Domain pointer that has to be looked up + * + * This function looks up @domain and returns the appropriate + * virDomainObjPtr. + * + * Returns the domain object which is locked on success, NULL + * otherwise. + */ +static virDomainObjPtr +lxcDomObjFromDomain(virDomainPtr domain) +{ + virDomainObjPtr vm; + virLXCDriverPtr driver = domain->conn->privateData; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); + if (!vm) { + virUUIDFormat(domain->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, domain->name); + return NULL; + } + + return vm; +} + /* Functions */ static virDrvOpenStatus lxcConnectOpen(virConnectPtr conn, @@ -295,18 +324,11 @@ cleanup: static int lxcDomainIsActive(virDomainPtr dom) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr obj; int ret = -1; - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!obj) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(obj = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainIsActiveEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -322,18 +344,11 @@ cleanup: static int lxcDomainIsPersistent(virDomainPtr dom) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr obj; int ret = -1; - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!obj) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(obj = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainIsPersistentEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -348,18 +363,11 @@ cleanup: static int lxcDomainIsUpdated(virDomainPtr dom) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr obj; int ret = -1; - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!obj) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(obj = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainIsUpdatedEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -506,14 +514,8 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -559,20 +561,12 @@ static int lxcDomainUndefine(virDomainPtr dom) static int lxcDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1, rc; virLXCDomainObjPrivatePtr priv; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } priv = vm->privateData; @@ -618,21 +612,13 @@ lxcDomainGetState(virDomainPtr dom, int *reason, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; virCheckFlags(0, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetStateEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -648,19 +634,11 @@ cleanup: static char *lxcDomainGetOSType(virDomainPtr dom) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; char *ret = NULL; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetOSTypeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -678,19 +656,11 @@ cleanup: static unsigned long long lxcDomainGetMaxMemory(virDomainPtr dom) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; unsigned long long ret = 0; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -704,19 +674,11 @@ cleanup: } static int lxcDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainSetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -737,19 +699,13 @@ cleanup: } static int lxcDomainSetMemory(virDomainPtr dom, unsigned long newmem) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; virLXCDomainObjPrivatePtr priv; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainSetMemoryEnsureACL(dom->conn, vm->def) < 0) @@ -787,7 +743,6 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, int nparams, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; int ret = -1; @@ -805,15 +760,9 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, NULL) < 0) return -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainSetMemoryParametersEnsureACL(dom->conn, vm->def, flags) < 0) @@ -859,7 +808,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, int *nparams, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; unsigned long long val; @@ -869,15 +817,9 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, virCheckFlags(0, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainGetMemoryParametersEnsureACL(dom->conn, vm->def) < 0) @@ -950,21 +892,13 @@ cleanup: static char *lxcDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; char *ret = NULL; /* Flags checked by virDomainDefFormat */ - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -998,14 +932,8 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1146,17 +1074,10 @@ static int lxcDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr secla virDomainObjPtr vm; int ret = -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - memset(seclabel, 0, sizeof(*seclabel)); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetSecurityLabelEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1352,14 +1273,8 @@ lxcDomainDestroyFlags(virDomainPtr dom, virCheckFlags(0, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1683,18 +1598,14 @@ cleanup: static char *lxcDomainGetSchedulerType(virDomainPtr dom, int *nparams) { - virLXCDriverPtr driver = dom->conn->privateData; char *ret = NULL; int rc; virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (vm == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No such domain %s"), dom->uuid); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0) @@ -1834,13 +1745,9 @@ lxcDomainSetSchedulerParametersFlags(virDomainPtr dom, NULL) < 0) return -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No such domain %s"), dom->uuid); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags) < 0) @@ -1969,13 +1876,9 @@ lxcDomainGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No such domain %s"), dom->uuid); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0) @@ -2090,13 +1993,9 @@ lxcDomainSetBlkioParameters(virDomainPtr dom, NULL) < 0) return -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No such domain %s"), dom->uuid); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainSetBlkioParametersEnsureACL(dom->conn, vm->def, flags) < 0) @@ -2189,13 +2088,9 @@ lxcDomainGetBlkioParameters(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (vm == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No such domain %s"), dom->uuid); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainGetBlkioParametersEnsureACL(dom->conn, vm->def) < 0) @@ -2283,20 +2178,12 @@ lxcDomainInterfaceStats(virDomainPtr dom, const char *path, struct _virDomainInterfaceStats *stats) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; size_t i; int ret = -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainInterfaceStatsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2340,19 +2227,11 @@ lxcDomainInterfaceStats(virDomainPtr dom, static int lxcDomainGetAutostart(virDomainPtr dom, int *autostart) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainGetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2375,15 +2254,8 @@ static int lxcDomainSetAutostart(virDomainPtr dom, int ret = -1; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainSetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2539,15 +2411,8 @@ static int lxcDomainSuspend(virDomainPtr dom) int ret = -1; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainSuspendEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2593,15 +2458,8 @@ static int lxcDomainResume(virDomainPtr dom) virLXCDomainObjPrivatePtr priv; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } priv = vm->privateData; @@ -2647,22 +2505,15 @@ lxcDomainOpenConsole(virDomainPtr dom, virStreamPtr st, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; int ret = -1; virDomainChrDefPtr chr = NULL; size_t i; virCheckFlags(0, -1); - virUUIDFormat(dom->uuid, uuidstr); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2719,10 +2570,8 @@ lxcDomainSendProcessSignal(virDomainPtr dom, unsigned int signum, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; virLXCDomainObjPrivatePtr priv; - char uuidstr[VIR_UUID_STRING_BUFLEN]; pid_t victim; int ret = -1; @@ -2735,13 +2584,9 @@ lxcDomainSendProcessSignal(virDomainPtr dom, return -1; } - virUUIDFormat(dom->uuid, uuidstr); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainSendProcessSignalEnsureACL(dom->conn, vm->def) < 0) @@ -2815,7 +2660,6 @@ static int lxcDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; char *vroot = NULL; @@ -2825,15 +2669,8 @@ lxcDomainShutdownFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_SHUTDOWN_INITCTL | VIR_DOMAIN_SHUTDOWN_SIGNAL, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } priv = vm->privateData; @@ -2903,7 +2740,6 @@ static int lxcDomainReboot(virDomainPtr dom, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; char *vroot = NULL; @@ -2913,15 +2749,8 @@ lxcDomainReboot(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_REBOOT_INITCTL | VIR_DOMAIN_REBOOT_SIGNAL, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } priv = vm->privateData; @@ -4260,15 +4089,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -4393,15 +4215,8 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -4510,15 +4325,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -4629,7 +4437,6 @@ static int lxcDomainLxcOpenNamespace(virDomainPtr dom, int **fdlist, unsigned int flags) { - virLXCDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; int ret = -1; @@ -4638,14 +4445,9 @@ static int lxcDomainLxcOpenNamespace(virDomainPtr dom, *fdlist = NULL; virCheckFlags(0, -1); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; - } + priv = vm->privateData; if (virDomainLxcOpenNamespaceEnsureACL(dom->conn, vm->def) < 0) -- 1.8.1.5

On Wed, Jul 17, 2013 at 03:04:26PM +0200, Michal Privoznik wrote:
Similarly to qemu driver, we can use a helper function to lookup a domain instead of copying multiple lines around. --- src/lxc/lxc_driver.c | 344 +++++++++++---------------------------------------- 1 file changed, 73 insertions(+), 271 deletions(-)
ACK 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 :|

On 17.07.2013 15:04, Michal Privoznik wrote:
The first round of my patches that breaks LXC driver into smaller pieces. With my testing on 10 domains which are started up and destroyed in a loop with 10 iterations (each loop is run in a separate thread in client):
Before: real 0m45.973s user 0m0.080s sys 0m0.020s
After: real 0m14.951s user 0m0.080s sys 0m0.020s
Forgot to mention, patches are available at my gitorious too: https://gitorious.org/~zippy2/libvirt/michal-staging/commits/lxc_lock Michal

On Wed, Jul 17, 2013 at 03:04:17PM +0200, Michal Privoznik wrote:
The first round of my patches that breaks LXC driver into smaller pieces. With my testing on 10 domains which are started up and destroyed in a loop with 10 iterations (each loop is run in a separate thread in client):
Before: real 0m45.973s user 0m0.080s sys 0m0.020s
After: real 0m14.951s user 0m0.080s sys 0m0.020s
Michal Privoznik (9): qemu: Move close callbacks handling into util/virclosecallbacks.c Introduce a virLXCDriverConfigPtr object lxc: Use atomic ops for driver->nactive Introduce annotations for virLXCDriverPtr fields lxc: switch to virCloseCallbacks API Stop accessing driver->caps directly in LXC driver lxc: Make activeUsbHostdevs use locks Remove lxcDriverLock from almost everywhere Introduce lxcDomObjFromDomain
po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/libvirt_private.syms | 8 + src/lxc/lxc_conf.c | 120 ++++++-- src/lxc/lxc_conf.h | 64 ++-- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 705 +++++++++++++++---------------------------- src/lxc/lxc_hostdev.c | 8 + src/lxc/lxc_process.c | 181 ++++------- src/lxc/lxc_process.h | 1 - src/qemu/qemu_conf.c | 295 +----------------- src/qemu/qemu_conf.h | 25 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_migration.c | 18 +- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_process.c | 14 +- src/util/virclosecallbacks.c | 356 ++++++++++++++++++++++ src/util/virclosecallbacks.h | 56 ++++ 18 files changed, 900 insertions(+), 964 deletions(-) create mode 100644 src/util/virclosecallbacks.c create mode 100644 src/util/virclosecallbacks.h
Great job. It was much simpler than I thought it would be - guess the QEMU driver conversion solved all the hard bits in shared code. 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 :|
participants (2)
-
Daniel P. Berrange
-
Michal Privoznik