[libvirt] [PATCH 0/5] Misc changes to domain driver stuff
The patches that follow make some changes to the domain driver infrastructure, and the QEMU driver. The important goal is to improve the concurrency of the QEMU driver, by allowing multiple readers on the driver (change mutex to a rwlock) and removing the O(n) locks on the virDomainObjPtr instances These have not been heavily tested yet, beyond checking the TCK passes with QEMU driver. I just send them now for early review, while I continue to work on the rest of the QEMU monitor related patches which this series supports. The CIL object locking test also needs to be extended to understand the idea of RWlocks instead of just mutexes so it can properly validate this code Daniel
Most of the hash iterators need to modify either payload of data args. The const annotation prevents this. * src/util/hash.h, src/util/hash.c: Remove const-ness from virHashForEach/Iterator * src/xen/xm_internal.c: Remove bogus casts --- src/util/hash.c | 2 +- src/util/hash.h | 4 ++-- src/xen/xm_internal.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 42a28d1..40df2c6 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -482,7 +482,7 @@ virHashRemoveEntry(virHashTablePtr table, const char *name, * * Returns number of items iterated over upon completion, -1 on failure */ -int virHashForEach(virHashTablePtr table, virHashIterator iter, const void *data) { +int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { int i, count = 0; if (table == NULL || iter == NULL) diff --git a/src/util/hash.h b/src/util/hash.h index 7778909..a163f10 100644 --- a/src/util/hash.h +++ b/src/util/hash.h @@ -38,7 +38,7 @@ typedef void (*virHashDeallocator) (void *payload, const char *name); * * Callback to process a hash entry during iteration */ -typedef void (*virHashIterator) (const void *payload, const char *name, const void *data); +typedef void (*virHashIterator) (void *payload, const char *name, void *data); /** * virHashSearcher * @payload: the data in the hash @@ -82,7 +82,7 @@ void *virHashLookup(virHashTablePtr table, const char *name); /* * Iterators */ -int virHashForEach(virHashTablePtr table, virHashIterator iter, const void *data); +int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data); int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, virHashDeallocator f, const void *data); void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index ebbaae8..732b2d3 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -2750,8 +2750,8 @@ struct xenXMListIteratorContext { char ** names; }; -static void xenXMListIterator(const void *payload ATTRIBUTE_UNUSED, const char *name, const void *data) { - struct xenXMListIteratorContext *ctx = (struct xenXMListIteratorContext *)data; +static void xenXMListIterator(void *payload ATTRIBUTE_UNUSED, const char *name, void *data) { + struct xenXMListIteratorContext *ctx = data; virDomainPtr dom = NULL; if (ctx->count == ctx->max) -- 1.6.2.5
On Wed, Oct 14, 2009 at 11:48:50AM +0100, Daniel P. Berrange wrote:
Most of the hash iterators need to modify either payload of data args. The const annotation prevents this.
* src/util/hash.h, src/util/hash.c: Remove const-ness from virHashForEach/Iterator * src/xen/xm_internal.c: Remove bogus casts
ACK, that looks completely independant of other parts so let's push this now. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
The current virDomainObjListPtr object stores domain objects in an array. This means that to find a particular objects requires O(n) time, and more critically acquiring O(n) mutex locks. The new impl replaces the array with a virHashTable, keyed off UUID. Finding a object based on UUID is now O(1) time, and only requires a single mutex lock. Finding by name/id is unchanged in complexity. In changing this, all code which iterates over the array had to be updated to use a hash table iterator function callback. Several of the functions which were identically duplicating across all drivers were pulled into domain_conf.c * src/conf/domain_conf.h, src/conf/domain_conf.c: Change virDomainObjListPtr to use virHashTable. Add a initializer method virDomainObjListInit, and rename virDomainObjListFree to virDomainObjListDeinit, since its not actually freeing the container, only its contents. Also add some convenient methods virDomainObjListGetInactiveNames, virDomainObjListGetActiveIDs and virDomainObjListNumOfDomains which can be used to implement the correspondingly named public API entry points in drivers * src/libvirt_private.syms: Export new methods from domain_conf.h * src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_conf.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/test/test_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c: Update all code to deal with hash tables instead of arrays for domains --- src/conf/domain_conf.c | 268 ++++++++++++++++++++++++++++++------------- src/conf/domain_conf.h | 19 +++- src/libvirt_private.syms | 6 +- src/lxc/lxc_driver.c | 242 +++++++++++++++++---------------------- src/opennebula/one_driver.c | 68 +++-------- src/openvz/openvz_conf.c | 7 +- src/openvz/openvz_driver.c | 25 ++--- src/qemu/qemu_driver.c | 216 ++++++++++++++--------------------- src/test/test_driver.c | 60 +++------- src/uml/uml_driver.c | 125 +++++++++----------- src/vbox/vbox_tmpl.c | 2 - 11 files changed, 505 insertions(+), 533 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f50a8ef..7700d6a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -193,51 +193,93 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST, #ifndef PROXY -virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms, - int id) +int virDomainObjListInit(virDomainObjListPtr doms) { - unsigned int i; - - for (i = 0 ; i < doms->count ; i++) { - virDomainObjLock(doms->objs[i]); - if (virDomainIsActive(doms->objs[i]) && - doms->objs[i]->def->id == id) - return doms->objs[i]; - virDomainObjUnlock(doms->objs[i]); + doms->objs = virHashCreate(50); + if (!doms->objs) { + virReportOOMError(NULL); + return -1; } + return 0; +} - return NULL; + +static void virDomainObjListDeallocator(void *payload, const char *name ATTRIBUTE_UNUSED) +{ + virDomainObjPtr obj = payload; + virDomainObjFree(obj); +} + +void virDomainObjListDeinit(virDomainObjListPtr doms) +{ + if (doms->objs) + virHashFree(doms->objs, virDomainObjListDeallocator); +} + + +static int virDomainObjListSearchID(const void *payload, + const char *name ATTRIBUTE_UNUSED, + const void *data) +{ + virDomainObjPtr obj = (virDomainObjPtr)payload; + const int *id = data; + int want = 0; + + virDomainObjLock(obj); + if (virDomainIsActive(obj) && + obj->def->id == *id) + want = 1; + virDomainObjUnlock(obj); + return want; +} + +virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms, + int id) +{ + virDomainObjPtr obj; + obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); + if (obj) + virDomainObjLock(obj); + return obj; } virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms, const unsigned char *uuid) { - unsigned int i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr obj; - for (i = 0 ; i < doms->count ; i++) { - virDomainObjLock(doms->objs[i]); - if (!memcmp(doms->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return doms->objs[i]; - virDomainObjUnlock(doms->objs[i]); - } + virUUIDFormat(uuid, uuidstr); - return NULL; + obj = virHashLookup(doms->objs, uuidstr); + if (obj) + virDomainObjLock(obj); + return obj; +} + +static int virDomainObjListSearchName(const void *payload, + const char *name ATTRIBUTE_UNUSED, + const void *data) +{ + virDomainObjPtr obj = (virDomainObjPtr)payload; + int want = 0; + + virDomainObjLock(obj); + if (STREQ(obj->def->name, (const char *)data)) + want = 1; + virDomainObjUnlock(obj); + return want; } virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, const char *name) { - unsigned int i; - - for (i = 0 ; i < doms->count ; i++) { - virDomainObjLock(doms->objs[i]); - if (STREQ(doms->objs[i]->def->name, name)) - return doms->objs[i]; - virDomainObjUnlock(doms->objs[i]); - } - - return NULL; + virDomainObjPtr obj; + obj = virHashSearch(doms->objs, virDomainObjListSearchName, name); + if (obj) + virDomainObjLock(obj); + return obj; } #endif /* !PROXY */ @@ -532,21 +574,6 @@ void virDomainObjFree(virDomainObjPtr dom) VIR_FREE(dom); } -void virDomainObjListFree(virDomainObjListPtr vms) -{ - unsigned int i; - - if (!vms) - return; - - for (i = 0 ; i < vms->count ; i++) - virDomainObjFree(vms->objs[i]); - - VIR_FREE(vms->objs); - vms->count = 0; -} - - static virDomainObjPtr virDomainObjNew(virConnectPtr conn) { virDomainObjPtr domain; @@ -576,6 +603,7 @@ virDomainObjPtr virDomainAssignDef(virConnectPtr conn, const virDomainDefPtr def) { virDomainObjPtr domain; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((domain = virDomainFindByUUID(doms, def->uuid))) { if (!virDomainIsActive(domain)) { @@ -590,49 +618,34 @@ virDomainObjPtr virDomainAssignDef(virConnectPtr conn, return domain; } - if (VIR_REALLOC_N(doms->objs, doms->count + 1) < 0) { - virReportOOMError(conn); - return NULL; - } - if (!(domain = virDomainObjNew(conn))) return NULL; - domain->def = def; - doms->objs[doms->count] = domain; - doms->count++; + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) { + VIR_FREE(domain); + virReportOOMError(conn); + return NULL; + } return domain; } +/* + * The caller must hold a lock on the driver owning 'doms', + * and must also have locked 'dom', to ensure no one else + * is either waiting for 'dom' or still usingn it + */ void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainObjPtr dom) { - unsigned int i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->def->uuid, uuidstr); virDomainObjUnlock(dom); - for (i = 0 ; i < doms->count ; i++) { - virDomainObjLock(doms->objs[i]); - if (doms->objs[i] == dom) { - virDomainObjUnlock(doms->objs[i]); - virDomainObjFree(doms->objs[i]); - - if (i < (doms->count - 1)) - memmove(doms->objs + i, doms->objs + i + 1, - sizeof(*(doms->objs)) * (doms->count - (i + 1))); - - if (VIR_REALLOC_N(doms->objs, doms->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - doms->count--; - - break; - } - virDomainObjUnlock(doms->objs[i]); - } - + virHashRemoveEntry(doms->objs, uuidstr, virDomainObjListDeallocator); } @@ -4650,7 +4663,7 @@ static virDomainObjPtr virDomainLoadStatus(virConnectPtr conn, { char *statusFile = NULL; virDomainObjPtr obj = NULL; - virDomainObjPtr tmp = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((statusFile = virDomainConfigFile(conn, statusDir, name)) == NULL) goto error; @@ -4658,23 +4671,20 @@ static virDomainObjPtr virDomainLoadStatus(virConnectPtr conn, if (!(obj = virDomainObjParseFile(conn, caps, statusFile))) goto error; - tmp = virDomainFindByName(doms, obj->def->name); - if (tmp) { - virDomainObjUnlock(obj); + virUUIDFormat(obj->def->uuid, uuidstr); + + if (virHashLookup(doms->objs, uuidstr) != NULL) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, _("unexpected domain %s already exists"), obj->def->name); goto error; } - if (VIR_REALLOC_N(doms->objs, doms->count + 1) < 0) { + if (virHashAddEntry(doms->objs, uuidstr, obj) < 0) { virReportOOMError(conn); goto error; } - doms->objs[doms->count] = obj; - doms->count++; - if (notify) (*notify)(obj, 1, opaque); @@ -4861,4 +4871,100 @@ void virDomainObjUnlock(virDomainObjPtr obj) virMutexUnlock(&obj->lock); } + +static void virDomainObjListCountActive(void *payload, const char *name ATTRIBUTE_UNUSED, void *data) +{ + virDomainObjPtr obj = payload; + int *count = data; + virDomainObjLock(obj); + if (virDomainIsActive(obj)) + (*count)++; + virDomainObjUnlock(obj); +} + +static void virDomainObjListCountInactive(void *payload, const char *name ATTRIBUTE_UNUSED, void *data) +{ + virDomainObjPtr obj = payload; + int *count = data; + virDomainObjLock(obj); + if (!virDomainIsActive(obj)) + (*count)++; + virDomainObjUnlock(obj); +} + +int virDomainObjListNumOfDomains(virDomainObjListPtr doms, int active) +{ + int count = 0; + if (active) + virHashForEach(doms->objs, virDomainObjListCountActive, &count); + else + virHashForEach(doms->objs, virDomainObjListCountInactive, &count); + return count; +} + +struct virDomainIDData { + int numids; + int maxids; + int *ids; +}; + +static void virDomainObjListCopyActiveIDs(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr obj = payload; + struct virDomainIDData *data = opaque; + virDomainObjLock(obj); + if (virDomainIsActive(obj) && data->numids < data->maxids) + data->ids[data->numids++] = obj->def->id; + virDomainObjUnlock(obj); +} + +int virDomainObjListGetActiveIDs(virDomainObjListPtr doms, + int *ids, + int maxids) +{ + struct virDomainIDData data = { 0, maxids, ids }; + virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data); + return data.numids; +} + +struct virDomainNameData { + int oom; + int numnames; + int maxnames; + char **const names; +}; + +static void virDomainObjListCopyInactiveNames(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr obj = payload; + struct virDomainNameData *data = opaque; + virDomainObjLock(obj); + if (!virDomainIsActive(obj) && data->numnames < data->maxnames) { + if (!(data->names[data->numnames] = strdup(obj->def->name))) + data->oom = 1; + else + data->numnames++; + } + virDomainObjUnlock(obj); +} + + +int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, + char **const names, + int maxnames) +{ + struct virDomainNameData data = { 0, 0, maxnames, names }; + int i; + virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); + if (data.oom) + goto cleanup; + + return data.numnames; + +cleanup: + for (i = 0 ; i < data.numnames ; i++) + VIR_FREE(data.names[i]); + return -1; +} + #endif /* ! PROXY */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4b3646e..11f059c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -33,6 +33,7 @@ #include "storage_encryption_conf.h" #include "util.h" #include "threads.h" +#include "hash.h" /* Private component of virDomainXMLFlags */ typedef enum { @@ -613,8 +614,9 @@ struct _virDomainObj { typedef struct _virDomainObjList virDomainObjList; typedef virDomainObjList *virDomainObjListPtr; struct _virDomainObjList { - unsigned int count; - virDomainObjPtr *objs; + /* uuid string -> virDomainObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; }; static inline int @@ -623,6 +625,8 @@ virDomainIsActive(virDomainObjPtr dom) return dom->def->id != -1; } +int virDomainObjListInit(virDomainObjListPtr objs); +void virDomainObjListDeinit(virDomainObjListPtr objs); virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms, int id); @@ -644,7 +648,6 @@ void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); void virDomainDefFree(virDomainDefPtr vm); void virDomainObjFree(virDomainObjPtr vm); -void virDomainObjListFree(virDomainObjListPtr vms); virDomainObjPtr virDomainAssignDef(virConnectPtr conn, virDomainObjListPtr doms, @@ -756,6 +759,16 @@ int virDomainVideoDefaultRAM(virDomainDefPtr def, int type); void virDomainObjLock(virDomainObjPtr obj); void virDomainObjUnlock(virDomainObjPtr obj); +int virDomainObjListNumOfDomains(virDomainObjListPtr doms, int active); + +int virDomainObjListGetActiveIDs(virDomainObjListPtr doms, + int *ids, + int maxids); +int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, + char **const names, + int maxnames); + + VIR_ENUM_DECL(virDomainVirt) VIR_ENUM_DECL(virDomainBoot) VIR_ENUM_DECL(virDomainFeature) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 37395ab..f17d260 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -124,7 +124,6 @@ virDomainLoadAllConfigs; virDomainNetDefFree; virDomainNetTypeToString; virDomainObjFree; -virDomainObjListFree; virDomainRemoveInactive; virDomainSaveXML; virDomainSaveConfig; @@ -143,6 +142,11 @@ virDomainObjLock; virDomainObjUnlock; virDomainStateTypeToString; virDomainStateTypeFromString; +virDomainObjListGetInactiveNames; +virDomainObjListGetActiveIDs; +virDomainObjListNumOfDomains; +virDomainObjListInit; +virDomainObjListDeinit; # domain_event.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0b614e3..077e970 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -220,31 +220,21 @@ cleanup: static int lxcListDomains(virConnectPtr conn, int *ids, int nids) { lxc_driver_t *driver = conn->privateData; - int got = 0, i; + int n; lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nids ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - ids[got++] = driver->domains.objs[i]->def->id; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetActiveIDs(&driver->domains, ids, nids); lxcDriverUnlock(driver); - return got; + return n; } static int lxcNumDomains(virConnectPtr conn) { lxc_driver_t *driver = conn->privateData; - int n = 0, i; + int n; lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 1); lxcDriverUnlock(driver); return n; @@ -253,43 +243,22 @@ static int lxcNumDomains(virConnectPtr conn) { static int lxcListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { lxc_driver_t *driver = conn->privateData; - int got = 0, i; + int n; lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nnames ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) { - if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) { - virReportOOMError(conn); - virDomainObjUnlock(driver->domains.objs[i]); - goto cleanup; - } - } - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetInactiveNames(&driver->domains, names, nnames); lxcDriverUnlock(driver); - return got; - - cleanup: - for (i = 0 ; i < got ; i++) - VIR_FREE(names[i]); - lxcDriverUnlock(driver); - return -1; + return n; } static int lxcNumDefinedDomains(virConnectPtr conn) { lxc_driver_t *driver = conn->privateData; - int n = 0, i; + int n; lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 0); lxcDriverUnlock(driver); return n; @@ -887,27 +856,15 @@ static void lxcMonitorEvent(int watch, int events ATTRIBUTE_UNUSED, void *data) { - lxc_driver_t *driver = data; - virDomainObjPtr vm = NULL; + lxc_driver_t *driver = lxc_driver; + virDomainObjPtr vm = data; virDomainEventPtr event = NULL; - unsigned int i; lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr tmpvm = driver->domains.objs[i]; - virDomainObjLock(tmpvm); - if (tmpvm->monitorWatch == watch) { - vm = tmpvm; - break; - } - virDomainObjUnlock(tmpvm); - } - if (!vm) { - virEventRemoveHandle(watch); - goto cleanup; - } + virDomainObjLock(vm); + lxcDriverUnlock(driver); - if (vm->monitor != fd) { + if (vm->monitor != fd || vm->monitorWatch != watch) { virEventRemoveHandle(watch); goto cleanup; } @@ -925,11 +882,9 @@ static void lxcMonitorEvent(int watch, } cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); if (event) lxcDomainEventQueue(driver, event); - lxcDriverUnlock(driver); } @@ -1548,9 +1503,40 @@ static int lxcCheckNetNsSupport(void) } +struct lxcAutostartData { + lxc_driver_t *driver; + virConnectPtr conn; +}; + +static void +lxcAutostartDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr vm = payload; + const struct lxcAutostartData *data = opaque; + + virDomainObjLock(vm); + if (vm->autostart && + !virDomainIsActive(vm)) { + int ret = lxcVmStart(data->conn, data->driver, vm); + if (ret < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to autostart VM '%s': %s\n"), + vm->def->name, + err ? err->message : ""); + } else { + virDomainEventPtr event = + virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_BOOTED); + if (event) + lxcDomainEventQueue(data->driver, event); + } + } + virDomainObjUnlock(vm); +} + static void lxcAutostartConfigs(lxc_driver_t *driver) { - unsigned int i; /* XXX: Figure out a better way todo this. The domain * startup code needs a connection handle in order * to lookup the bridge associated with a virtual @@ -1559,39 +1545,65 @@ lxcAutostartConfigs(lxc_driver_t *driver) { virConnectPtr conn = virConnectOpen("lxc:///"); /* Ignoring NULL conn which is mostly harmless here */ + struct lxcAutostartData data = { driver, conn }; + lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr vm = driver->domains.objs[i]; - virDomainObjLock(vm); - if (vm->autostart && - !virDomainIsActive(vm)) { - int ret = lxcVmStart(conn, driver, vm); - if (ret < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to autostart VM '%s': %s\n"), - vm->def->name, - err ? err->message : ""); - } else { - virDomainEventPtr event = - virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_BOOTED); - if (event) - lxcDomainEventQueue(driver, event); - } - } - virDomainObjUnlock(vm); - } + virHashForEach(driver->domains.objs, lxcAutostartDomain, &data); lxcDriverUnlock(driver); if (conn) virConnectClose(conn); } +static void +lxcReconnectVM(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr vm = payload; + lxc_driver_t *driver = opaque; + char *config = NULL; + virDomainDefPtr tmp; + + virDomainObjLock(vm); + if ((vm->monitor = lxcMonitorClient(NULL, driver, vm)) < 0) { + goto cleanup; + } + + /* Read pid from controller */ + if ((virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) { + close(vm->monitor); + vm->monitor = -1; + goto cleanup; + } + + if ((config = virDomainConfigFile(NULL, + driver->stateDir, + vm->def->name)) == NULL) + goto cleanup; + + /* Try and load the live config */ + tmp = virDomainDefParseFile(NULL, driver->caps, config, 0); + VIR_FREE(config); + if (tmp) { + vm->newDef = vm->def; + vm->def = tmp; + } + + if (vm->pid != 0) { + vm->def->id = vm->pid; + vm->state = VIR_DOMAIN_RUNNING; + } else { + vm->def->id = -1; + close(vm->monitor); + vm->monitor = -1; + } + +cleanup: + virDomainObjUnlock(vm); +} + static int lxcStartup(int privileged) { - unsigned int i; char *ld; int rc; @@ -1623,6 +1635,9 @@ static int lxcStartup(int privileged) goto cleanup; } + if (virDomainObjListInit(&lxc_driver->domains) < 0) + goto cleanup; + if (VIR_ALLOC(lxc_driver->domainEventCallbacks) < 0) goto cleanup; if (!(lxc_driver->domainEventQueue = virDomainEventQueueNew())) @@ -1657,50 +1672,7 @@ static int lxcStartup(int privileged) 0, NULL, NULL) < 0) goto cleanup; - for (i = 0 ; i < lxc_driver->domains.count ; i++) { - virDomainObjPtr vm = lxc_driver->domains.objs[i]; - char *config = NULL; - virDomainDefPtr tmp; - - virDomainObjLock(vm); - if ((vm->monitor = lxcMonitorClient(NULL, lxc_driver, vm)) < 0) { - virDomainObjUnlock(vm); - continue; - } - - /* Read pid from controller */ - if ((rc = virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) { - close(vm->monitor); - vm->monitor = -1; - virDomainObjUnlock(vm); - continue; - } - - if ((config = virDomainConfigFile(NULL, - lxc_driver->stateDir, - vm->def->name)) == NULL) { - virDomainObjUnlock(vm); - continue; - } - - /* Try and load the live config */ - tmp = virDomainDefParseFile(NULL, lxc_driver->caps, config, 0); - VIR_FREE(config); - if (tmp) { - vm->newDef = vm->def; - vm->def = tmp; - } - - if (vm->pid != 0) { - vm->def->id = vm->pid; - vm->state = VIR_DOMAIN_RUNNING; - } else { - vm->def->id = -1; - close(vm->monitor); - vm->monitor = -1; - } - virDomainObjUnlock(vm); - } + virHashForEach(lxc_driver->domains.objs, lxcReconnectVM, lxc_driver); lxcDriverUnlock(lxc_driver); return 0; @@ -1756,7 +1728,7 @@ static int lxcShutdown(void) return(-1); lxcDriverLock(lxc_driver); - virDomainObjListFree(&lxc_driver->domains); + virDomainObjListDeinit(&lxc_driver->domains); virDomainEventCallbackListFree(lxc_driver->domainEventCallbacks); virDomainEventQueueFree(lxc_driver->domainEventQueue); @@ -1785,19 +1757,13 @@ static int lxcShutdown(void) */ static int lxcActive(void) { - unsigned int i; - int active = 0; + int active; if (lxc_driver == NULL) return(0); lxcDriverLock(lxc_driver); - for (i = 0 ; i < lxc_driver->domains.count ; i++) { - virDomainObjLock(lxc_driver->domains.objs[i]); - if (virDomainIsActive(lxc_driver->domains.objs[i])) - active = 1; - virDomainObjUnlock(lxc_driver->domains.objs[i]); - } + active = virDomainObjListNumOfDomains(&lxc_driver->domains, 1); lxcDriverUnlock(lxc_driver); return active; diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index 9bcd5c3..0b807ad 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -178,32 +178,22 @@ return_point: static int oneListDomains(virConnectPtr conn, int *ids, int nids) { one_driver_t *driver = (one_driver_t *)conn->privateData; - int got = 0, i; + int n; oneDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nids ; i++){ - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - ids[got++] = driver->domains.objs[i]->def->id; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetActiveIDs(&driver->domains, ids, nids); oneDriverUnlock(driver); - return got; + return n; } static int oneNumDomains(virConnectPtr conn) { one_driver_t *driver = (one_driver_t *)conn->privateData; - int n = 0, i; + int n; oneDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++){ - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 1); oneDriverUnlock(driver); return n; @@ -212,44 +202,22 @@ static int oneNumDomains(virConnectPtr conn) static int oneListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { one_driver_t *driver = (one_driver_t *)conn->privateData; - int got = 0, i; + int n; oneDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nnames ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) { - if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) { - virReportOOMError(conn); - virDomainObjUnlock(driver->domains.objs[i]); - goto cleanup; - } - } - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetInactiveNames(&driver->domains, names, nnames); oneDriverUnlock(driver); - return got; - -cleanup: - for (i = 0 ; i < got ; i++) - VIR_FREE(names[i]); - oneDriverUnlock(driver); - - return -1; + return n; } static int oneNumDefinedDomains(virConnectPtr conn) { one_driver_t *driver = (one_driver_t *)conn->privateData; - int n = 0, i; + int n; oneDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++){ - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 0); oneDriverUnlock(driver); return n; @@ -647,6 +615,12 @@ static int oneStartup(int privileged ATTRIBUTE_UNUSED){ return -1; } + if (virDomainObjListInit(&one_driver->domains) < 0) { + virMutexDestroy(&one_driver->lock); + VIR_FREE(one_driver); + return -1; + } + c_oneStart(); oneDriverLock(one_driver); one_driver->nextid=1; @@ -665,7 +639,7 @@ static int oneShutdown(void){ return(-1); oneDriverLock(one_driver); - virDomainObjListFree(&one_driver->domains); + virDomainObjListDeinit(&one_driver->domains); virCapabilitiesFree(one_driver->caps); oneDriverUnlock(one_driver); @@ -677,19 +651,13 @@ static int oneShutdown(void){ } static int oneActive(void){ - unsigned int i; int active = 0; if (one_driver == NULL) return(0); oneDriverLock(one_driver); - for (i = 0 ; i < one_driver->domains.count ; i++) { - virDomainObjLock(one_driver->domains.objs[i]); - if (virDomainIsActive(one_driver->domains.objs[i])) - active = 1; - virDomainObjUnlock(one_driver->domains.objs[i]); - } + active = virDomainObjListNumOfDomains(&one_driver->domains, 1); oneDriverUnlock(one_driver); return active; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 403f537..c928afb 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -421,7 +421,7 @@ openvzFreeDriver(struct openvz_driver *driver) if (!driver) return; - virDomainObjListFree(&driver->domains); + virDomainObjListDeinit(&driver->domains); virCapabilitiesFree(driver->caps); } @@ -509,11 +509,10 @@ int openvzLoadDomains(struct openvz_driver *driver) { openvzReadNetworkConf(NULL, dom->def, veid); openvzReadFSConf(NULL, dom->def, veid); - if (VIR_REALLOC_N(driver->domains.objs, - driver->domains.count + 1) < 0) + virUUIDFormat(dom->def->uuid, uuidstr); + if (virHashAddEntry(driver->domains.objs, uuidstr, dom) < 0) goto no_memory; - driver->domains.objs[driver->domains.count++] = dom; dom = NULL; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index f64ad1e..b0092cd 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1168,6 +1168,9 @@ static virDrvOpenStatus openvzOpen(virConnectPtr conn, return VIR_DRV_OPEN_ERROR; } + if (virDomainObjListInit(&driver->domains) < 0) + goto cleanup; + if (!(driver->caps = openvzCapsInit())) goto cleanup; @@ -1247,18 +1250,13 @@ static int openvzListDomains(virConnectPtr conn, int *ids, int nids) { static int openvzNumDomains(virConnectPtr conn) { struct openvz_driver *driver = conn->privateData; - int nactive = 0, i; + int n; openvzDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - nactive++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 1); openvzDriverUnlock(driver); - return nactive; + return n; } static int openvzListDefinedDomains(virConnectPtr conn, @@ -1350,18 +1348,13 @@ Version: 2.2 static int openvzNumDefinedDomains(virConnectPtr conn) { struct openvz_driver *driver = conn->privateData; - int ninactive = 0, i; + int n; openvzDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) - ninactive++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 0); openvzDriverUnlock(driver); - return ninactive; + return n; } static virDriver openvzDriver = { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d37b184..1eb7797 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -201,9 +201,42 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir, const char* name, off_t p } +struct qemuAutostartData { + struct qemud_driver *driver; + virConnectPtr conn; +}; +static void +qemuAutostartDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr vm = payload; + struct qemuAutostartData *data = opaque; + + virDomainObjLock(vm); + if (vm->autostart && + !virDomainIsActive(vm)) { + int ret; + + virResetLastError(); + ret = qemudStartVMDaemon(data->conn, data->driver, vm, NULL, -1); + if (ret < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to autostart VM '%s': %s\n"), + vm->def->name, + err ? err->message : ""); + } else { + virDomainEventPtr event = + virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_BOOTED); + if (event) + qemuDomainEventQueue(data->driver, event); + } + } + virDomainObjUnlock(vm); +} + static void qemudAutostartConfigs(struct qemud_driver *driver) { - unsigned int i; /* XXX: Figure out a better way todo this. The domain * startup code needs a connection handle in order * to lookup the bridge associated with a virtual @@ -213,33 +246,10 @@ qemudAutostartConfigs(struct qemud_driver *driver) { "qemu:///system" : "qemu:///session"); /* Ignoring NULL conn which is mostly harmless here */ + struct qemuAutostartData data = { driver, conn }; qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr vm = driver->domains.objs[i]; - virDomainObjLock(vm); - if (vm->autostart && - !virDomainIsActive(vm)) { - int ret; - - virResetLastError(); - ret = qemudStartVMDaemon(conn, driver, vm, NULL, -1); - if (ret < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to autostart VM '%s': %s\n"), - vm->def->name, - err ? err->message : ""); - } else { - virDomainEventPtr event = - virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_BOOTED); - if (event) - qemuDomainEventQueue(driver, event); - } - } - virDomainObjUnlock(vm); - } + virHashForEach(driver->domains.objs, qemuAutostartDomain, &data); qemuDriverUnlock(driver); if (conn) @@ -284,7 +294,6 @@ cleanup: static int qemudOpenMonitor(virConnectPtr conn, - struct qemud_driver* driver, virDomainObjPtr vm, int reconnect); @@ -293,13 +302,16 @@ static int qemudOpenMonitor(virConnectPtr conn, * Open an existing VM's monitor, re-detect VCPU threads * and re-reserve the security labels in use */ -static int -qemuReconnectDomain(struct qemud_driver *driver, - virDomainObjPtr obj) +static void +qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) { int rc; + virDomainObjPtr obj = payload; + struct qemud_driver *driver = opaque; - if ((rc = qemudOpenMonitor(NULL, driver, obj, 1)) != 0) { + virDomainObjLock(obj); + + if ((rc = qemudOpenMonitor(NULL, obj, 1)) != 0) { VIR_ERROR(_("Failed to reconnect monitor for %s: %d\n"), obj->def->name, rc); goto error; @@ -313,15 +325,20 @@ qemuReconnectDomain(struct qemud_driver *driver, driver->securityDriver && driver->securityDriver->domainReserveSecurityLabel && driver->securityDriver->domainReserveSecurityLabel(NULL, obj) < 0) - return -1; + goto error; if (obj->def->id >= driver->nextvmid) driver->nextvmid = obj->def->id + 1; - return 0; + virDomainObjUnlock(obj); + return; error: - return -1; + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later */ + qemudShutdownVMDaemon(NULL, driver, obj); + virDomainObjUnlock(obj); } /** @@ -333,20 +350,7 @@ error: static void qemuReconnectDomains(struct qemud_driver *driver) { - int i; - - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr obj = driver->domains.objs[i]; - - virDomainObjLock(obj); - if (qemuReconnectDomain(driver, obj) < 0) { - /* If we can't get the monitor back, then kill the VM - * so user has ability to start it again later without - * danger of ending up running twice */ - qemudShutdownVMDaemon(NULL, driver, obj); - } - virDomainObjUnlock(obj); - } + virHashForEach(driver->domains.objs, qemuReconnectDomain, driver); } @@ -438,8 +442,11 @@ qemudStartup(int privileged) { /* Don't have a dom0 so start from 1 */ qemu_driver->nextvmid = 1; + if (virDomainObjListInit(&qemu_driver->domains) < 0) + goto out_of_memory; + /* Init callback list */ - if(VIR_ALLOC(qemu_driver->domainEventCallbacks) < 0) + if (VIR_ALLOC(qemu_driver->domainEventCallbacks) < 0) goto out_of_memory; if (!(qemu_driver->domainEventQueue = virDomainEventQueueNew())) goto out_of_memory; @@ -679,21 +686,14 @@ qemudReload(void) { */ static int qemudActive(void) { - unsigned int i; int active = 0; if (!qemu_driver) return 0; + /* XXX having to iterate here is not great because it requires many locks */ qemuDriverLock(qemu_driver); - for (i = 0 ; i < qemu_driver->domains.count ; i++) { - virDomainObjPtr vm = qemu_driver->domains.objs[i]; - virDomainObjLock(vm); - if (virDomainIsActive(vm)) - active = 1; - virDomainObjUnlock(vm); - } - + active = virDomainObjListNumOfDomains(&qemu_driver->domains, 1); qemuDriverUnlock(qemu_driver); return active; } @@ -713,7 +713,7 @@ qemudShutdown(void) { pciDeviceListFree(NULL, qemu_driver->activePciHostdevs); virCapabilitiesFree(qemu_driver->caps); - virDomainObjListFree(&qemu_driver->domains); + virDomainObjListDeinit(&qemu_driver->domains); VIR_FREE(qemu_driver->securityDriverName); VIR_FREE(qemu_driver->logDir); @@ -913,7 +913,6 @@ qemudCheckMonitorPrompt(virConnectPtr conn ATTRIBUTE_UNUSED, static int qemudOpenMonitorCommon(virConnectPtr conn, - struct qemud_driver* driver, virDomainObjPtr vm, int monfd, int reconnect) @@ -952,7 +951,7 @@ qemudOpenMonitorCommon(virConnectPtr conn, if ((vm->monitorWatch = virEventAddHandle(vm->monitor, VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR, qemudDispatchVMEvent, - driver, NULL)) < 0) + vm, NULL)) < 0) return -1; return 0; @@ -960,7 +959,6 @@ qemudOpenMonitorCommon(virConnectPtr conn, static int qemudOpenMonitorUnix(virConnectPtr conn, - struct qemud_driver* driver, virDomainObjPtr vm, const char *monitor, int reconnect) @@ -1008,7 +1006,7 @@ qemudOpenMonitorUnix(virConnectPtr conn, goto error; } - if (qemudOpenMonitorCommon(conn, driver, vm, monfd, reconnect) < 0) + if (qemudOpenMonitorCommon(conn, vm, monfd, reconnect) < 0) goto error; return 0; @@ -1020,7 +1018,6 @@ error: static int qemudOpenMonitorPty(virConnectPtr conn, - struct qemud_driver* driver, virDomainObjPtr vm, const char *monitor, int reconnect) @@ -1033,7 +1030,7 @@ qemudOpenMonitorPty(virConnectPtr conn, return -1; } - if (qemudOpenMonitorCommon(conn, driver, vm, monfd, reconnect) < 0) + if (qemudOpenMonitorCommon(conn, vm, monfd, reconnect) < 0) goto error; return 0; @@ -1045,17 +1042,16 @@ error: static int qemudOpenMonitor(virConnectPtr conn, - struct qemud_driver *driver, virDomainObjPtr vm, int reconnect) { switch (vm->monitor_chr->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: - return qemudOpenMonitorUnix(conn, driver, vm, + return qemudOpenMonitorUnix(conn, vm, vm->monitor_chr->data.nix.path, reconnect); case VIR_DOMAIN_CHR_TYPE_PTY: - return qemudOpenMonitorPty(conn, driver, vm, + return qemudOpenMonitorPty(conn, vm, vm->monitor_chr->data.file.path, reconnect); default: @@ -1177,7 +1173,7 @@ qemudWaitForMonitor(virConnectPtr conn, return -1; } - if (qemudOpenMonitor(conn, driver, vm, 0) < 0) + if (qemudOpenMonitor(conn, vm, 0) < 0) return -1; return 0; @@ -2255,28 +2251,22 @@ retry: static void qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { - struct qemud_driver *driver = opaque; - virDomainObjPtr vm = NULL; + struct qemud_driver *driver = qemu_driver; + virDomainObjPtr vm = opaque; virDomainEventPtr event = NULL; - unsigned int i; int quit = 0, failed = 0; + /* XXX Normally we have to lock the driver first, to protect + * against someone adding/removing the domain. We know, + * however, then if we're getting data in this callback + * the VM must be running. Nowhere is allowed to remove + * a domain while it is running, so it is safe to not + * lock the driver here... */ qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr tmpvm = driver->domains.objs[i]; - virDomainObjLock(tmpvm); - if (virDomainIsActive(tmpvm) && - tmpvm->monitorWatch == watch) { - vm = tmpvm; - break; - } - virDomainObjUnlock(tmpvm); - } - - if (!vm) - goto cleanup; + virDomainObjLock(vm); + qemuDriverUnlock(driver); - if (vm->monitor != fd) { + if (vm->monitor != fd || vm->monitorWatch != watch) { failed = 1; } else { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) @@ -2302,12 +2292,9 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { } } -cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); } @@ -2637,31 +2624,21 @@ qemudGetHostname (virConnectPtr conn) static int qemudListDomains(virConnectPtr conn, int *ids, int nids) { struct qemud_driver *driver = conn->privateData; - int got = 0, i; + int n; qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nids ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - ids[got++] = driver->domains.objs[i]->def->id; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetActiveIDs(&driver->domains, ids, nids); qemuDriverUnlock(driver); - return got; + return n; } static int qemudNumDomains(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - int n = 0, i; + int n; qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 1); qemuDriverUnlock(driver); return n; @@ -4059,39 +4036,20 @@ cleanup: static int qemudListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { struct qemud_driver *driver = conn->privateData; - int got = 0, i; + int n; qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nnames ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) { - if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) { - virReportOOMError(conn); - virDomainObjUnlock(driver->domains.objs[i]); - goto cleanup; - } - } - virDomainObjUnlock(driver->domains.objs[i]); - } - + n = virDomainObjListGetInactiveNames(&driver->domains, names, nnames); qemuDriverUnlock(driver); - return got; - - cleanup: - for (i = 0 ; i < got ; i++) - VIR_FREE(names[i]); - qemuDriverUnlock(driver); - return -1; + return n; } static int qemudNumDefinedDomains(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; - int n = 0, i; + int n; qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) - if (!virDomainIsActive(driver->domains.objs[i])) - n++; + n = virDomainObjListNumOfDomains(&driver->domains, 0); qemuDriverUnlock(driver); return n; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0541a73..919a2f6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -338,6 +338,9 @@ static int testOpenDefault(virConnectPtr conn) { goto error; } + if (virDomainObjListInit(&privconn->domains) < 0) + goto error; + memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); // Numa setup @@ -419,7 +422,7 @@ static int testOpenDefault(virConnectPtr conn) { return VIR_DRV_OPEN_SUCCESS; error: - virDomainObjListFree(&privconn->domains); + virDomainObjListDeinit(&privconn->domains); virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); @@ -569,6 +572,9 @@ static int testOpenFromFile(virConnectPtr conn, testDriverLock(privconn); conn->privateData = privconn; + if (virDomainObjListInit(&privconn->domains) < 0) + goto error; + if (!(privconn->caps = testBuildCapabilities(conn))) goto error; @@ -897,7 +903,7 @@ static int testOpenFromFile(virConnectPtr conn, VIR_FREE(pools); if (fd != -1) close(fd); - virDomainObjListFree(&privconn->domains); + virDomainObjListDeinit(&privconn->domains); virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); @@ -966,7 +972,7 @@ static int testClose(virConnectPtr conn) testConnPtr privconn = conn->privateData; testDriverLock(privconn); virCapabilitiesFree(privconn->caps); - virDomainObjListFree(&privconn->domains); + virDomainObjListDeinit(&privconn->domains); virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); @@ -1036,15 +1042,13 @@ static char *testGetCapabilities (virConnectPtr conn) static int testNumOfDomains(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - unsigned int numActive = 0, i; + int count; testDriverLock(privconn); - for (i = 0 ; i < privconn->domains.count ; i++) - if (virDomainIsActive(privconn->domains.objs[i])) - numActive++; + count = virDomainObjListNumOfDomains(&privconn->domains, 1); testDriverUnlock(privconn); - return numActive; + return count; } static virDomainPtr @@ -1173,15 +1177,10 @@ static int testListDomains (virConnectPtr conn, int maxids) { testConnPtr privconn = conn->privateData; - unsigned int n = 0, i; + int n; testDriverLock(privconn); - for (i = 0 ; i < privconn->domains.count && n < maxids ; i++) { - virDomainObjLock(privconn->domains.objs[i]); - if (virDomainIsActive(privconn->domains.objs[i])) - ids[n++] = privconn->domains.objs[i]->def->id; - virDomainObjUnlock(privconn->domains.objs[i]); - } + n = virDomainObjListGetActiveIDs(&privconn->domains, ids, maxids); testDriverUnlock(privconn); return n; @@ -1852,47 +1851,28 @@ cleanup: static int testNumOfDefinedDomains(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - unsigned int numInactive = 0, i; + int count; testDriverLock(privconn); - for (i = 0 ; i < privconn->domains.count ; i++) { - virDomainObjLock(privconn->domains.objs[i]); - if (!virDomainIsActive(privconn->domains.objs[i])) - numInactive++; - virDomainObjUnlock(privconn->domains.objs[i]); - } + count = virDomainObjListNumOfDomains(&privconn->domains, 0); testDriverUnlock(privconn); - return numInactive; + return count; } static int testListDefinedDomains(virConnectPtr conn, char **const names, int maxnames) { + testConnPtr privconn = conn->privateData; - unsigned int n = 0, i; + int n; testDriverLock(privconn); memset(names, 0, sizeof(*names)*maxnames); - for (i = 0 ; i < privconn->domains.count && n < maxnames ; i++) { - virDomainObjLock(privconn->domains.objs[i]); - if (!virDomainIsActive(privconn->domains.objs[i]) && - !(names[n++] = strdup(privconn->domains.objs[i]->def->name))) { - virDomainObjUnlock(privconn->domains.objs[i]); - goto no_memory; - } - virDomainObjUnlock(privconn->domains.objs[i]); - } + n = virDomainObjListGetInactiveNames(&privconn->domains, names, maxnames); testDriverUnlock(privconn); return n; - -no_memory: - virReportOOMError(conn); - for (n = 0 ; n < maxnames ; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static virDomainPtr testDomainDefineXML(virConnectPtr conn, diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 9a7fe42..80cf477 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -114,10 +114,32 @@ static int umlMonitorCommand (virConnectPtr conn, static struct uml_driver *uml_driver = NULL; +struct umlAutostartData { + struct uml_driver *driver; + virConnectPtr conn; +}; + +static void +umlAutostartDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr vm = payload; + const struct umlAutostartData *data = opaque; + + virDomainObjLock(vm); + if (vm->autostart && + !virDomainIsActive(vm)) { + virResetLastError(); + if (umlStartVMDaemon(data->conn, data->driver, vm) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to autostart VM '%s': %s"), + vm->def->name, err->message); + } + } + virDomainObjUnlock(vm); +} static void umlAutostartConfigs(struct uml_driver *driver) { - unsigned int i; /* XXX: Figure out a better way todo this. The domain * startup code needs a connection handle in order * to lookup the bridge associated with a virtual @@ -128,15 +150,9 @@ umlAutostartConfigs(struct uml_driver *driver) { "uml:///session"); /* Ignoring NULL conn which is mostly harmless here */ - for (i = 0 ; i < driver->domains.count ; i++) { - if (driver->domains.objs[i]->autostart && - !virDomainIsActive(driver->domains.objs[i]) && - umlStartVMDaemon(conn, driver, driver->domains.objs[i]) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to autostart VM '%s': %s"), - driver->domains.objs[i]->def->name, err->message); - } - } + struct umlAutostartData data = { driver, conn }; + + virHashForEach(driver->domains.objs, umlAutostartDomain, &data); if (conn) virConnectClose(conn); @@ -320,6 +336,9 @@ umlStartup(int privileged) { uml_driver->nextvmid = 1; uml_driver->inotifyWatch = -1; + if (virDomainObjListInit(¨_driver->domains) < 0) + goto error; + userdir = virGetUserDirectory(NULL, uid); if (!userdir) goto error; @@ -449,24 +468,30 @@ umlReload(void) { */ static int umlActive(void) { - unsigned int i; int active = 0; if (!uml_driver) return 0; umlDriverLock(uml_driver); - for (i = 0 ; i < uml_driver->domains.count ; i++) { - virDomainObjLock(uml_driver->domains.objs[i]); - if (virDomainIsActive(uml_driver->domains.objs[i])) - active = 1; - virDomainObjUnlock(uml_driver->domains.objs[i]); - } + active = virDomainObjListNumOfDomains(¨_driver->domains, 1); umlDriverUnlock(uml_driver); return active; } +static void +umlShutdownOneVM(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr dom = payload; + struct uml_driver *driver = opaque; + + virDomainObjLock(dom); + if (virDomainIsActive(dom)) + umlShutdownVMDaemon(NULL, driver, dom); + virDomainObjUnlock(dom); +} + /** * umlShutdown: * @@ -474,8 +499,6 @@ umlActive(void) { */ static int umlShutdown(void) { - unsigned int i; - if (!uml_driver) return -1; @@ -485,16 +508,11 @@ umlShutdown(void) { close(uml_driver->inotifyFD); virCapabilitiesFree(uml_driver->caps); - /* shutdown active VMs */ - for (i = 0 ; i < uml_driver->domains.count ; i++) { - virDomainObjPtr dom = uml_driver->domains.objs[i]; - virDomainObjLock(dom); - if (virDomainIsActive(dom)) - umlShutdownVMDaemon(NULL, uml_driver, dom); - virDomainObjUnlock(dom); - } + /* shutdown active VMs + * XXX allow them to stay around & reconnect */ + virHashForEach(uml_driver->domains.objs, umlShutdownOneVM, uml_driver); - virDomainObjListFree(¨_driver->domains); + virDomainObjListDeinit(¨_driver->domains); VIR_FREE(uml_driver->logDir); VIR_FREE(uml_driver->configDir); @@ -1145,30 +1163,20 @@ umlGetHostname (virConnectPtr conn) static int umlListDomains(virConnectPtr conn, int *ids, int nids) { struct uml_driver *driver = conn->privateData; - int got = 0, i; + int n; umlDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nids ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - ids[got++] = driver->domains.objs[i]->def->id; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetActiveIDs(&driver->domains, ids, nids); umlDriverUnlock(driver); - return got; + return n; } static int umlNumDomains(virConnectPtr conn) { struct uml_driver *driver = conn->privateData; - int n = 0, i; + int n; umlDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 1); umlDriverUnlock(driver); return n; @@ -1481,42 +1489,21 @@ cleanup: static int umlListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { struct uml_driver *driver = conn->privateData; - int got = 0, i; + int n; umlDriverLock(driver); - for (i = 0 ; i < driver->domains.count && got < nnames ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) { - if (!(names[got++] = strdup(driver->domains.objs[i]->def->name))) { - virReportOOMError(conn); - virDomainObjUnlock(driver->domains.objs[i]); - goto cleanup; - } - } - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListGetInactiveNames(&driver->domains, names, nnames); umlDriverUnlock(driver); - return got; - - cleanup: - for (i = 0 ; i < got ; i++) - VIR_FREE(names[i]); - umlDriverUnlock(driver); - return -1; + return n; } static int umlNumDefinedDomains(virConnectPtr conn) { struct uml_driver *driver = conn->privateData; - int n = 0, i; + int n; umlDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjLock(driver->domains.objs[i]); - if (!virDomainIsActive(driver->domains.objs[i])) - n++; - virDomainObjUnlock(driver->domains.objs[i]); - } + n = virDomainObjListNumOfDomains(&driver->domains, 0); umlDriverUnlock(driver); return n; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4f43901..4741c57 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -103,7 +103,6 @@ typedef struct { virMutex lock; int version; - virDomainObjList domains; virCapsPtr caps; IVirtualBox *vboxObj; @@ -485,7 +484,6 @@ static void vboxUninitialize(vboxGlobalData *data) { if (data->pFuncs) data->pFuncs->pfnComUninitialize(); - virDomainObjListFree(&data->domains); virCapabilitiesFree(data->caps); #if VBOX_API_VERSION == 2002 /* No domainEventCallbacks in 2.2.* version */ -- 1.6.2.5
On Wed, Oct 14, 2009 at 11:48:51AM +0100, Daniel P. Berrange wrote:
The current virDomainObjListPtr object stores domain objects in an array. This means that to find a particular objects requires O(n) time, and more critically acquiring O(n) mutex locks.
The new impl replaces the array with a virHashTable, keyed off UUID. Finding a object based on UUID is now O(1) time, and only requires a single mutex lock. Finding by name/id is unchanged in complexity.
In changing this, all code which iterates over the array had to be updated to use a hash table iterator function callback. Several of the functions which were identically duplicating across all drivers were pulled into domain_conf.c
So we went from list to array, and now to hash table :-) <nitpick> I'm afraid the O(1) is an exageration since as the number grows you would still need to lock each object on the clash list for that hash. But for normal use we should end up in constant time, but in theory I think it's still O(n) ... </nitpick> I expect that the gain will come from the fact all lookups intenally are done though the UUID, the Name/ID being only convenience API. I'm also wondering about the symetry for other kind of objects, we probably don't need this for networks (less objects, less operation and operation usually short), you really expect that to be a single optimization and just for domains, right ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Oct 14, 2009 at 04:04:50PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 11:48:51AM +0100, Daniel P. Berrange wrote:
The current virDomainObjListPtr object stores domain objects in an array. This means that to find a particular objects requires O(n) time, and more critically acquiring O(n) mutex locks.
The new impl replaces the array with a virHashTable, keyed off UUID. Finding a object based on UUID is now O(1) time, and only requires a single mutex lock. Finding by name/id is unchanged in complexity.
In changing this, all code which iterates over the array had to be updated to use a hash table iterator function callback. Several of the functions which were identically duplicating across all drivers were pulled into domain_conf.c
So we went from list to array, and now to hash table :-)
Yeah, I very much regret switching from the linked list to an array. I don't know why I didn't go straight to a hash table when I changed it last time :-(
<nitpick> I'm afraid the O(1) is an exageration since as the number grows you would still need to lock each object on the clash list for that hash. But for normal use we should end up in constant time, but in theory I think it's still O(n) ... </nitpick>
If that ever becomes a problem, which I very much doubt, then we can easily enhance the hash table impl to automatically add extra buckets once the number of elements passes some threshold so you guarentee its always very close to O(1) The important change here is really removal of the O(n) mutex locks, whcih will always be O(1), because even if there are many entries in each hash bucket you're still doing the lookup based on the key, not the entry. So you'll only ever need to lock the object you eventually find.
I expect that the gain will come from the fact all lookups intenally are done though the UUID, the Name/ID being only convenience API. I'm also wondering about the symetry for other kind of objects, we probably don't need this for networks (less objects, less operation and operation usually short), you really expect that to be a single optimization and just for domains, right ?
I think it will probably be worth doing it for storage pools too. And once we've done that we might as well do it for networks too just to be consistent everywhere, but its not critical for now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 03:16:19PM +0100, Daniel P. Berrange wrote:
On Wed, Oct 14, 2009 at 04:04:50PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 11:48:51AM +0100, Daniel P. Berrange wrote: So we went from list to array, and now to hash table :-)
Yeah, I very much regret switching from the linked list to an array. I don't know why I didn't go straight to a hash table when I changed it last time :-(
point is we switched everything (well nearly) to arrays, but we are only changing domain lookup to hash table.
<nitpick> I'm afraid the O(1) is an exageration since as the number grows you would still need to lock each object on the clash list for that hash. But for normal use we should end up in constant time, but in theory I think it's still O(n) ... </nitpick>
If that ever becomes a problem, which I very much doubt, then we can easily enhance the hash table impl to automatically add extra buckets once the number of elements passes some threshold so you guarentee its always very close to O(1)
The important change here is really removal of the O(n) mutex locks, whcih will always be O(1), because even if there are many entries in each hash bucket you're still doing the lookup based on the key, not the entry. So you'll only ever need to lock the object you eventually find.
I was nitpicking on the formalism :-) Independantly of O(whatever) I'm sure it's way faster to jump directly to the hash list. But I'm afraid we need to check something, looking quickly at the patch I don't see any locking of the hash table itself during the lookups, maybe I missed it but there is 2 things I need to raise - the obvious that another thread may modify the hash table, I assume it's covered by the new R/W locks but just double checking - the less obvious point that the object hash can't be cached because the hash table code allows to grow as needed (up to some limit) that's inherited from libxml2 design where I wanted to keep small hashes for non-complex document. Since UUID are immutable we could drop that virHashGrow() code, go for the larger from the start, and avoid recomputing UUID strings and hash everytime by caching them in the domain object. Crude but might save some cycle.
I expect that the gain will come from the fact all lookups intenally are done though the UUID, the Name/ID being only convenience API. I'm also wondering about the symetry for other kind of objects, we probably don't need this for networks (less objects, less operation and operation usually short), you really expect that to be a single optimization and just for domains, right ?
I think it will probably be worth doing it for storage pools too. And once we've done that we might as well do it for networks too just to be consistent everywhere, but its not critical for now.
Yup, let's implement and evaluate for domains, then we will see if consistency really matters. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2009/10/14 Daniel P. Berrange <berrange@redhat.com>:
The current virDomainObjListPtr object stores domain objects in an array. This means that to find a particular objects requires O(n) time, and more critically acquiring O(n) mutex locks.
The new impl replaces the array with a virHashTable, keyed off UUID. Finding a object based on UUID is now O(1) time, and only requires a single mutex lock. Finding by name/id is unchanged in complexity.
In changing this, all code which iterates over the array had to be updated to use a hash table iterator function callback. Several of the functions which were identically duplicating across all drivers were pulled into domain_conf.c
* src/conf/domain_conf.h, src/conf/domain_conf.c: Change virDomainObjListPtr to use virHashTable. Add a initializer method virDomainObjListInit, and rename virDomainObjListFree to virDomainObjListDeinit, since its not actually freeing the container, only its contents. Also add some convenient methods virDomainObjListGetInactiveNames, virDomainObjListGetActiveIDs and virDomainObjListNumOfDomains which can be used to implement the correspondingly named public API entry points in drivers * src/libvirt_private.syms: Export new methods from domain_conf.h * src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_conf.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/test/test_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c: Update all code to deal with hash tables instead of arrays for domains ---
+ +static void virDomainObjListCopyInactiveNames(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ + virDomainObjPtr obj = payload; + struct virDomainNameData *data = opaque;
You could check if already hit OOM and return before trying to call strdup() again, because that'll probably fail again in an OOM situation: if (data->oom) return;
+ virDomainObjLock(obj); + if (!virDomainIsActive(obj) && data->numnames < data->maxnames) { + if (!(data->names[data->numnames] = strdup(obj->def->name))) + data->oom = 1; + else + data->numnames++; + } + virDomainObjUnlock(obj); +} + + +int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, + char **const names, + int maxnames) +{ + struct virDomainNameData data = { 0, 0, maxnames, names }; + int i; + virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); + if (data.oom)
virReportOOMError(NULL) is missing here.
+ goto cleanup; + + return data.numnames; + +cleanup: + for (i = 0 ; i < data.numnames ; i++) + VIR_FREE(data.names[i]); + return -1; +} +
@@ -887,27 +856,15 @@ static void lxcMonitorEvent(int watch, int events ATTRIBUTE_UNUSED, void *data) { - lxc_driver_t *driver = data; - virDomainObjPtr vm = NULL; + lxc_driver_t *driver = lxc_driver; + virDomainObjPtr vm = data; virDomainEventPtr event = NULL; - unsigned int i;
lxcDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr tmpvm = driver->domains.objs[i]; - virDomainObjLock(tmpvm); - if (tmpvm->monitorWatch == watch) { - vm = tmpvm; - break; - } - virDomainObjUnlock(tmpvm); - } - if (!vm) { - virEventRemoveHandle(watch); - goto cleanup; - } + virDomainObjLock(vm); + lxcDriverUnlock(driver);
- if (vm->monitor != fd) { + if (vm->monitor != fd || vm->monitorWatch != watch) { virEventRemoveHandle(watch); goto cleanup; } @@ -925,11 +882,9 @@ static void lxcMonitorEvent(int watch, }
cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); if (event) lxcDomainEventQueue(driver, event); - lxcDriverUnlock(driver); }
Before this change the lxcDomainEventQueue() was made while the driver lock was held, now its made without the lock being held. I'm pretty sure that this is not intended because all other calls to lxcDomainEventQueue() are made while the driver lock is being held.
static void qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { - struct qemud_driver *driver = opaque; - virDomainObjPtr vm = NULL; + struct qemud_driver *driver = qemu_driver; + virDomainObjPtr vm = opaque; virDomainEventPtr event = NULL; - unsigned int i; int quit = 0, failed = 0;
+ /* XXX Normally we have to lock the driver first, to protect + * against someone adding/removing the domain. We know, + * however, then if we're getting data in this callback + * the VM must be running. Nowhere is allowed to remove + * a domain while it is running, so it is safe to not + * lock the driver here... */ qemuDriverLock(driver); - for (i = 0 ; i < driver->domains.count ; i++) { - virDomainObjPtr tmpvm = driver->domains.objs[i]; - virDomainObjLock(tmpvm); - if (virDomainIsActive(tmpvm) && - tmpvm->monitorWatch == watch) { - vm = tmpvm; - break; - } - virDomainObjUnlock(tmpvm); - } - - if (!vm) - goto cleanup; + virDomainObjLock(vm); + qemuDriverUnlock(driver);
- if (vm->monitor != fd) { + if (vm->monitor != fd || vm->monitorWatch != watch) { failed = 1; } else { if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) @@ -2302,12 +2292,9 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { } }
-cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); }
Same as with lxcDomainEventQueue() above, the driver lock is no longer being held while calling qemuDomainEventQueue(), but it should be. Besides the remarks, ACK. Matthias
This implements a thin wrapper around the pthread_rwlock primitives. No impl is provided for Win32 at this time since it is rather hard, and none of our code yet requires it on Win32 * src/util/threads.h: Add virRWLockInit, virRWLockDestroy, virRWLockRead, virRWLockWrite, virRWLockUnlock APIs * src/util/threads-pthread.h: define virRWLock struct * src/util/threads-pthread.c: Implement RWLock APIs --- src/libvirt_private.syms | 6 ++++++ src/util/threads-pthread.c | 30 ++++++++++++++++++++++++++++++ src/util/threads-pthread.h | 4 ++++ src/util/threads.h | 10 ++++++++++ 4 files changed, 50 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f17d260..7b1e3c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -420,6 +420,12 @@ virCondWait; virCondSignal; virCondBroadcast; +virRWLockInit; +virRWLockDestroy; +virRWLockRead; +virRWLockWrite; +virRWLockUnlock; + # util.h virFileReadAll; virFileWriteStr; diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index 4e00bc5..2052c0a 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -57,6 +57,36 @@ void virMutexUnlock(virMutexPtr m) } +int virRWLockInit(virRWLockPtr m) +{ + if (pthread_rwlock_init(&m->lock, NULL) != 0) { + errno = EINVAL; + return -1; + } + return 0; +} + +void virRWLockDestroy(virRWLockPtr m) +{ + pthread_rwlock_destroy(&m->lock); +} + +void virRWLockRead(virRWLockPtr m) +{ + pthread_rwlock_rdlock(&m->lock); +} + +void virRWLockWrite(virRWLockPtr m) +{ + pthread_rwlock_wrlock(&m->lock); +} + +void virRWLockUnlock(virRWLockPtr m) +{ + pthread_rwlock_unlock(&m->lock); +} + + int virCondInit(virCondPtr c) { diff --git a/src/util/threads-pthread.h b/src/util/threads-pthread.h index 6404d1d..f2b546e 100644 --- a/src/util/threads-pthread.h +++ b/src/util/threads-pthread.h @@ -27,6 +27,10 @@ struct virMutex { pthread_mutex_t lock; }; +struct virRWLock { + pthread_rwlock_t lock; +}; + struct virCond { pthread_cond_t cond; }; diff --git a/src/util/threads.h b/src/util/threads.h index 62239b7..be65031 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -27,6 +27,9 @@ typedef struct virMutex virMutex; typedef virMutex *virMutexPtr; +typedef struct virRWLock virRWLock; +typedef virRWLock *virRWLockPtr; + typedef struct virCond virCond; typedef virCond *virCondPtr; @@ -44,6 +47,13 @@ void virMutexLock(virMutexPtr m); void virMutexUnlock(virMutexPtr m); +int virRWLockInit(virRWLockPtr m) ATTRIBUTE_RETURN_CHECK; +void virRWLockDestroy(virRWLockPtr m); + +void virRWLockRead(virRWLockPtr m); +void virRWLockWrite(virRWLockPtr m); +void virRWLockUnlock(virRWLockPtr m); + int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c) ATTRIBUTE_RETURN_CHECK; -- 1.6.2.5
2009/10/14 Daniel P. Berrange <berrange@redhat.com>:
This implements a thin wrapper around the pthread_rwlock primitives. No impl is provided for Win32 at this time since it is rather hard, and none of our code yet requires it on Win32
* src/util/threads.h: Add virRWLockInit, virRWLockDestroy, virRWLockRead, virRWLockWrite, virRWLockUnlock APIs * src/util/threads-pthread.h: define virRWLock struct * src/util/threads-pthread.c: Implement RWLock APIs --- src/libvirt_private.syms | 6 ++++++ src/util/threads-pthread.c | 30 ++++++++++++++++++++++++++++++ src/util/threads-pthread.h | 4 ++++ src/util/threads.h | 10 ++++++++++ 4 files changed, 50 insertions(+), 0 deletions(-)
ACK Matthias
A number of driver API methods which acquire the driver mutex only ever used the driver object in a read-only fashion. All these uses are converted to call qemuDriverLockRO() allowing for greater concurrency. * src/qemu/qemu_conf.h: s/Mutex/RWLock/ * src/qemu/qemu_driver.c: Add a qemuDriverLockRO() method and use it anywhere that doesn't require a write lock on the driver --- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 100 +++++++++++++++++++++++++++-------------------- 2 files changed, 58 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index f9a970f..a6e68f8 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -75,7 +75,7 @@ enum qemud_cmd_flags { /* Main driver state */ struct qemud_driver { - virMutex lock; + virRWLock lock; int privileged; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1eb7797..19187ac 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -78,13 +78,19 @@ static int qemudShutdown(void); +/* Obtains a full write lock */ static void qemuDriverLock(struct qemud_driver *driver) { - virMutexLock(&driver->lock); + virRWLockWrite(&driver->lock); +} +/* Obtains a read-only lock */ +static void qemuDriverLockRO(struct qemud_driver *driver) +{ + virRWLockRead(&driver->lock); } static void qemuDriverUnlock(struct qemud_driver *driver) { - virMutexUnlock(&driver->lock); + virRWLockUnlock(&driver->lock); } static void qemuDomainEventFlush(int timer, void *opaque); @@ -431,8 +437,8 @@ qemudStartup(int privileged) { if (VIR_ALLOC(qemu_driver) < 0) return -1; - if (virMutexInit(&qemu_driver->lock) < 0) { - VIR_ERROR("%s", _("cannot initialize mutex")); + if (virRWLockInit(&qemu_driver->lock) < 0) { + VIR_ERROR("%s", _("cannot initialize rwlock")); VIR_FREE(qemu_driver); return -1; } @@ -743,7 +749,7 @@ qemudShutdown(void) { virCgroupFree(&qemu_driver->cgroup); qemuDriverUnlock(qemu_driver); - virMutexDestroy(&qemu_driver->lock); + virRWLockDestroy(&qemu_driver->lock); VIR_FREE(qemu_driver); return 0; @@ -2262,7 +2268,7 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { * the VM must be running. Nowhere is allowed to remove * a domain while it is running, so it is safe to not * lock the driver here... */ - qemuDriverLock(driver); + qemuDriverLockRO(driver); virDomainObjLock(vm); qemuDriverUnlock(driver); @@ -2520,7 +2526,7 @@ static virDomainPtr qemudDomainLookupByID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByID(&driver->domains, id); qemuDriverUnlock(driver); @@ -2545,7 +2551,7 @@ static virDomainPtr qemudDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, uuid); qemuDriverUnlock(driver); @@ -2572,7 +2578,7 @@ static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByName(&driver->domains, name); qemuDriverUnlock(driver); @@ -2626,7 +2632,7 @@ static int qemudListDomains(virConnectPtr conn, int *ids, int nids) { struct qemud_driver *driver = conn->privateData; int n; - qemuDriverLock(driver); + qemuDriverLockRO(driver); n = virDomainObjListGetActiveIDs(&driver->domains, ids, nids); qemuDriverUnlock(driver); @@ -2637,7 +2643,7 @@ static int qemudNumDomains(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; int n; - qemuDriverLock(driver); + qemuDriverLockRO(driver); n = virDomainObjListNumOfDomains(&driver->domains, 1); qemuDriverUnlock(driver); @@ -2731,7 +2737,7 @@ static int qemudDomainSuspend(virDomainPtr dom) { int ret = -1; virDomainEventPtr event = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { @@ -2761,7 +2767,9 @@ static int qemudDomainSuspend(virDomainPtr dom) { cleanup: if (vm) virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + qemuDriverLock(driver); if (event) qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); @@ -2775,7 +2783,7 @@ static int qemudDomainResume(virDomainPtr dom) { int ret = -1; virDomainEventPtr event = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { @@ -2809,6 +2817,8 @@ static int qemudDomainResume(virDomainPtr dom) { cleanup: if (vm) virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + qemuDriverLock(driver); if (event) qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); @@ -2821,7 +2831,7 @@ static int qemudDomainShutdown(virDomainPtr dom) { virDomainObjPtr vm; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -2898,7 +2908,7 @@ static char *qemudDomainGetOSType(virDomainPtr dom) { virDomainObjPtr vm; char *type = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { @@ -2924,7 +2934,7 @@ static unsigned long qemudDomainGetMaxMemory(virDomainPtr dom) { virDomainObjPtr vm; unsigned long ret = 0; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -2949,7 +2959,7 @@ static int qemudDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) { virDomainObjPtr vm; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -2982,7 +2992,7 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) { virDomainObjPtr vm; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { @@ -3029,7 +3039,7 @@ static int qemudDomainGetInfo(virDomainPtr dom, int err; unsigned long balloon; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { @@ -3257,7 +3267,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, NULL, }; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3314,7 +3324,7 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { int ret = -1; const char *type; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3375,7 +3385,7 @@ qemudDomainPinVcpu(virDomainPtr dom, virNodeInfo nodeinfo; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3444,7 +3454,7 @@ qemudDomainGetVcpus(virDomainPtr dom, int i, v, maxcpu; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3533,7 +3543,7 @@ static int qemudDomainGetMaxVcpus(virDomainPtr dom) { const char *type; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3567,7 +3577,7 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec const char *type; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); memset(seclabel, 0, sizeof(*seclabel)); @@ -3627,7 +3637,7 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn, char *p; int ret = 0; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (!driver->securityDriver) { memset(secmodel, 0, sizeof (*secmodel)); goto cleanup; @@ -3843,7 +3853,7 @@ static char *qemudDomainDumpXML(virDomainPtr dom, unsigned long balloon; int err; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3891,7 +3901,7 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn, goto cleanup; } - qemuDriverLock(driver); + qemuDriverLockRO(driver); def = qemuParseCommandLineString(conn, driver->caps, config); qemuDriverUnlock(driver); if (!def) @@ -3921,7 +3931,7 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, char *ret = NULL; int i; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (STRNEQ(format, QEMU_CONFIG_FORMAT_ARGV)) { qemudReportError(conn, NULL, NULL, VIR_ERR_INVALID_ARG, @@ -4038,7 +4048,7 @@ static int qemudListDefinedDomains(virConnectPtr conn, struct qemud_driver *driver = conn->privateData; int n; - qemuDriverLock(driver); + qemuDriverLockRO(driver); n = virDomainObjListGetInactiveNames(&driver->domains, names, nnames); qemuDriverUnlock(driver); return n; @@ -4048,7 +4058,7 @@ static int qemudNumDefinedDomains(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; int n; - qemuDriverLock(driver); + qemuDriverLockRO(driver); n = virDomainObjListNumOfDomains(&driver->domains, 0); qemuDriverUnlock(driver); @@ -4062,7 +4072,7 @@ static int qemudDomainStart(virDomainPtr dom) { int ret = -1; virDomainEventPtr event = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { @@ -4082,6 +4092,8 @@ static int qemudDomainStart(virDomainPtr dom) { cleanup: if (vm) virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + qemuDriverLock(driver); if (event) qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); @@ -5151,7 +5163,7 @@ static int qemudDomainGetAutostart(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -5179,7 +5191,7 @@ static int qemudDomainSetAutostart(virDomainPtr dom, char *configFile = NULL, *autostartLink = NULL; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { @@ -5249,7 +5261,7 @@ static char *qemuGetSchedulerType(virDomainPtr dom, struct qemud_driver *driver = dom->conn->privateData; char *ret = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -5278,7 +5290,7 @@ static int qemuSetSchedulerParameters(virDomainPtr dom, virDomainObjPtr vm = NULL; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -5343,7 +5355,7 @@ static int qemuGetSchedulerParameters(virDomainPtr dom, int ret = -1; int rc; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -5410,7 +5422,7 @@ qemudDomainBlockStats (virDomainPtr dom, virDomainObjPtr vm; virDomainDiskDefPtr disk = NULL; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); if (!vm) { @@ -5471,7 +5483,7 @@ qemudDomainInterfaceStats (virDomainPtr dom, int i; int ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -5531,7 +5543,7 @@ qemudDomainBlockPeek (virDomainPtr dom, virDomainObjPtr vm; int fd = -1, ret = -1, i; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -5604,7 +5616,7 @@ qemudDomainMemoryPeek (virDomainPtr dom, char *tmp = NULL; int fd = -1, ret = -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -6759,7 +6771,7 @@ qemudDomainMigratePerform (virDomainPtr dom, int ret = -1; int paused = 0; - qemuDriverLock(driver); + qemuDriverLockRO(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -6831,6 +6843,8 @@ cleanup: if (vm) virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + qemuDriverLock(driver); if (event) qemuDomainEventQueue(driver, event); qemuDriverUnlock(driver); @@ -7038,7 +7052,7 @@ qemudNodeDeviceReset (virNodeDevicePtr dev) if (!pci) return -1; - qemuDriverLock(driver); + qemuDriverLockRO(driver); if (pciResetDevice(dev->conn, pci, driver->activePciHostdevs) < 0) goto out; -- 1.6.2.5
2009/10/14 Daniel P. Berrange <berrange@redhat.com>:
A number of driver API methods which acquire the driver mutex only ever used the driver object in a read-only fashion. All these uses are converted to call qemuDriverLockRO() allowing for greater concurrency.
* src/qemu/qemu_conf.h: s/Mutex/RWLock/ * src/qemu/qemu_driver.c: Add a qemuDriverLockRO() method and use it anywhere that doesn't require a write lock on the driver ---
ACK. Matthias
The virDomainObjPtr object stores state about a running domain. This object is shared across all drivers so it is not appropriate to include driver specific state here. This patch adds the ability to request a blob of private data per domain object instance. The driver must provide a allocator & deallocator for this purpose THis patch abuses the virCapabilitiesPtr structure for storing the allocator/deallocator callbacks, since it is already being abused for other internal things relating to parsing. This should be moved out into a separate object at some point. * src/conf/capabilities.h: Add privateDataAllocFunc and privateDataFreeFunc fields * src/conf/domain_conf.c: Invoke the driver allocators / deallocators when creating/freeing virDomainObjPtr instances. * src/conf/domain_conf.h: Pass virCapsPtr into virDomainAssignDef to allow access to the driver specific allocator function * src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/test/test_driver.c, src/uml/uml_driver.c: Update for change in virDomainAssignDef contract --- src/conf/capabilities.h | 2 ++ src/conf/domain_conf.c | 23 +++++++++++++++++++---- src/conf/domain_conf.h | 4 ++++ src/lxc/lxc_driver.c | 6 ++++-- src/opennebula/one_driver.c | 6 ++++-- src/openvz/openvz_driver.c | 6 ++++-- src/qemu/qemu_driver.c | 5 +++++ src/test/test_driver.c | 15 ++++++++++----- src/uml/uml_driver.c | 2 ++ 9 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 2f24605..7234cf4 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -115,6 +115,8 @@ struct _virCaps { virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; unsigned int emulatorRequired : 1; + void *(*privateDataAllocFunc)(void); + void (*privateDataFreeFunc)(void *); }; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7700d6a..b168c83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -569,12 +569,16 @@ void virDomainObjFree(virDomainObjPtr dom) VIR_FREE(dom->vcpupids); + if (dom->privateDataFreeFunc) + (dom->privateDataFreeFunc)(dom->privateData); + virMutexDestroy(&dom->lock); VIR_FREE(dom); } -static virDomainObjPtr virDomainObjNew(virConnectPtr conn) +static virDomainObjPtr virDomainObjNew(virConnectPtr conn, + virCapsPtr caps) { virDomainObjPtr domain; @@ -583,9 +587,19 @@ static virDomainObjPtr virDomainObjNew(virConnectPtr conn) return NULL; } + if (caps->privateDataAllocFunc && + !(domain->privateData = (caps->privateDataAllocFunc)())) { + virReportOOMError(conn); + VIR_FREE(domain); + return NULL; + } + domain->privateDataFreeFunc = caps->privateDataFreeFunc; + if (virMutexInit(&domain->lock) < 0) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); + if (domain->privateDataFreeFunc) + (domain->privateDataFreeFunc)(domain->privateData); VIR_FREE(domain); return NULL; } @@ -599,6 +613,7 @@ static virDomainObjPtr virDomainObjNew(virConnectPtr conn) } virDomainObjPtr virDomainAssignDef(virConnectPtr conn, + virCapsPtr caps, virDomainObjListPtr doms, const virDomainDefPtr def) { @@ -618,7 +633,7 @@ virDomainObjPtr virDomainAssignDef(virConnectPtr conn, return domain; } - if (!(domain = virDomainObjNew(conn))) + if (!(domain = virDomainObjNew(conn, caps))) return NULL; domain->def = def; @@ -3083,7 +3098,7 @@ static virDomainObjPtr virDomainObjParseXML(virConnectPtr conn, xmlNodePtr *nodes = NULL; int n, i; - if (!(obj = virDomainObjNew(conn))) + if (!(obj = virDomainObjNew(conn, caps))) return NULL; if (!(config = virXPathNode(conn, "./domain", ctxt))) { @@ -4634,7 +4649,7 @@ virDomainObjPtr virDomainLoadConfig(virConnectPtr conn, newVM = 0; } - if (!(dom = virDomainAssignDef(conn, doms, def))) + if (!(dom = virDomainAssignDef(conn, caps, doms, def))) goto error; dom->autostart = autostart; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 11f059c..1e53385 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -609,6 +609,9 @@ struct _virDomainObj { virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */ + + void *privateData; + void (*privateDataFreeFunc)(void *); }; typedef struct _virDomainObjList virDomainObjList; @@ -650,6 +653,7 @@ void virDomainDefFree(virDomainDefPtr vm); void virDomainObjFree(virDomainObjPtr vm); virDomainObjPtr virDomainAssignDef(virConnectPtr conn, + virCapsPtr caps, virDomainObjListPtr doms, const virDomainDefPtr def); void virDomainRemoveInactive(virDomainObjListPtr doms, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 077e970..3e0d24d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -315,7 +315,8 @@ static virDomainPtr lxcDomainDefine(virConnectPtr conn, const char *xml) goto cleanup; } - if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, def))) goto cleanup; def = NULL; vm->persistent = 1; @@ -1285,7 +1286,8 @@ lxcDomainCreateAndStart(virConnectPtr conn, } - if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, def))) goto cleanup; def = NULL; diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index 0b807ad..beb48ce 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -235,7 +235,8 @@ static virDomainPtr oneDomainDefine(virConnectPtr conn, const char *xml) VIR_DOMAIN_XML_INACTIVE))) goto return_point; - if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) { + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, def))) { virDomainDefFree(def); goto return_point; } @@ -439,7 +440,8 @@ oneDomainCreateAndStart(virConnectPtr conn, goto return_point; } - if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) { + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, def))) { virDomainDefFree(def); goto return_point; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b0092cd..d334235 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -774,7 +774,8 @@ openvzDomainDefineXML(virConnectPtr conn, const char *xml) vmdef->name); goto cleanup; } - if (!(vm = virDomainAssignDef(conn, &driver->domains, vmdef))) + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, vmdef))) goto cleanup; vmdef = NULL; @@ -841,7 +842,8 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, vmdef->name); goto cleanup; } - if (!(vm = virDomainAssignDef(conn, &driver->domains, vmdef))) + if (!(vm = virDomainAssignDef(conn, driver->caps, + &driver->domains, vmdef))) goto cleanup; vmdef = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 19187ac..1fe789a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2700,6 +2700,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, } if (!(vm = virDomainAssignDef(conn, + driver->caps, &driver->domains, def))) goto cleanup; @@ -3763,6 +3764,7 @@ static int qemudDomainRestore(virConnectPtr conn, } if (!(vm = virDomainAssignDef(conn, + driver->caps, &driver->domains, def))) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -4251,6 +4253,7 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { goto cleanup; if (!(vm = virDomainAssignDef(conn, + driver->caps, &driver->domains, def))) { goto cleanup; @@ -6109,6 +6112,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, } if (!(vm = virDomainAssignDef(dconn, + driver->caps, &driver->domains, def))) { qemudReportError(dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, @@ -6330,6 +6334,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, } if (!(vm = virDomainAssignDef(dconn, + driver->caps, &driver->domains, def))) { qemudReportError (dconn, NULL, NULL, VIR_ERR_OPERATION_FAILED, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 919a2f6..f08fe74 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -364,7 +364,8 @@ static int testOpenDefault(virConnectPtr conn) { goto error; if (testDomainGenerateIfnames(conn, domdef) < 0) goto error; - if (!(domobj = virDomainAssignDef(conn, &privconn->domains, domdef))) + if (!(domobj = virDomainAssignDef(conn, privconn->caps, + &privconn->domains, domdef))) goto error; domdef = NULL; domobj->def->id = privconn->nextDomID++; @@ -716,7 +717,8 @@ static int testOpenFromFile(virConnectPtr conn, } if (testDomainGenerateIfnames(conn, def) < 0 || - !(dom = virDomainAssignDef(conn, &privconn->domains, def))) { + !(dom = virDomainAssignDef(conn, privconn->caps, + &privconn->domains, def))) { virDomainDefFree(def); goto error; } @@ -1068,7 +1070,8 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; - if (!(dom = virDomainAssignDef(conn, &privconn->domains, def))) + if (!(dom = virDomainAssignDef(conn, privconn->caps, + &privconn->domains, def))) goto cleanup; def = NULL; dom->state = VIR_DOMAIN_RUNNING; @@ -1616,7 +1619,8 @@ static int testDomainRestore(virConnectPtr conn, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; - if (!(dom = virDomainAssignDef(conn, &privconn->domains, def))) + if (!(dom = virDomainAssignDef(conn, privconn->caps, + &privconn->domains, def))) goto cleanup; def = NULL; @@ -1890,7 +1894,8 @@ static virDomainPtr testDomainDefineXML(virConnectPtr conn, if (testDomainGenerateIfnames(conn, def) < 0) goto cleanup; - if (!(dom = virDomainAssignDef(conn, &privconn->domains, def))) + if (!(dom = virDomainAssignDef(conn, privconn->caps, + &privconn->domains, def))) goto cleanup; def = NULL; dom->persistent = 1; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 80cf477..4fb04d9 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1212,6 +1212,7 @@ static virDomainPtr umlDomainCreate(virConnectPtr conn, const char *xml, } if (!(vm = virDomainAssignDef(conn, + driver->caps, &driver->domains, def))) goto cleanup; @@ -1546,6 +1547,7 @@ static virDomainPtr umlDomainDefine(virConnectPtr conn, const char *xml) { goto cleanup; if (!(vm = virDomainAssignDef(conn, + driver->caps, &driver->domains, def))) goto cleanup; -- 1.6.2.5
2009/10/14 Daniel P. Berrange <berrange@redhat.com>:
The virDomainObjPtr object stores state about a running domain. This object is shared across all drivers so it is not appropriate to include driver specific state here. This patch adds the ability to request a blob of private data per domain object instance. The driver must provide a allocator & deallocator for this purpose
THis patch abuses the virCapabilitiesPtr structure for storing the allocator/deallocator callbacks, since it is already being abused for other internal things relating to parsing. This should be moved out into a separate object at some point.
* src/conf/capabilities.h: Add privateDataAllocFunc and privateDataFreeFunc fields * src/conf/domain_conf.c: Invoke the driver allocators / deallocators when creating/freeing virDomainObjPtr instances. * src/conf/domain_conf.h: Pass virCapsPtr into virDomainAssignDef to allow access to the driver specific allocator function * src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/test/test_driver.c, src/uml/uml_driver.c: Update for change in virDomainAssignDef contract --- src/conf/capabilities.h | 2 ++ src/conf/domain_conf.c | 23 +++++++++++++++++++---- src/conf/domain_conf.h | 4 ++++ src/lxc/lxc_driver.c | 6 ++++-- src/opennebula/one_driver.c | 6 ++++-- src/openvz/openvz_driver.c | 6 ++++-- src/qemu/qemu_driver.c | 5 +++++ src/test/test_driver.c | 15 ++++++++++----- src/uml/uml_driver.c | 2 ++ 9 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 2f24605..7234cf4 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -115,6 +115,8 @@ struct _virCaps { virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; unsigned int emulatorRequired : 1; + void *(*privateDataAllocFunc)(void); + void (*privateDataFreeFunc)(void *); };
Maybe add a comment here that this should be moved out into a separate object at some point. ACK. PS: What's the specific use case for this? Matthias
On Thu, Oct 15, 2009 at 11:41:56PM +0200, Matthias Bolte wrote:
2009/10/14 Daniel P. Berrange <berrange@redhat.com>:
The virDomainObjPtr object stores state about a running domain. This object is shared across all drivers so it is not appropriate to include driver specific state here. This patch adds the ability to request a blob of private data per domain object instance. The driver must provide a allocator & deallocator for this purpose
THis patch abuses the virCapabilitiesPtr structure for storing the allocator/deallocator callbacks, since it is already being abused for other internal things relating to parsing. This should be moved out into a separate object at some point.
* src/conf/capabilities.h: Add privateDataAllocFunc and privateDataFreeFunc fields * src/conf/domain_conf.c: Invoke the driver allocators / deallocators when creating/freeing virDomainObjPtr instances. * src/conf/domain_conf.h: Pass virCapsPtr into virDomainAssignDef to allow access to the driver specific allocator function * src/lxc/lxc_driver.c, src/opennebula/one_driver.c, src/openvz/openvz_driver.c, src/qemu/qemu_driver.c, src/test/test_driver.c, src/uml/uml_driver.c: Update for change in virDomainAssignDef contract --- src/conf/capabilities.h | 2 ++ src/conf/domain_conf.c | 23 +++++++++++++++++++---- src/conf/domain_conf.h | 4 ++++ src/lxc/lxc_driver.c | 6 ++++-- src/opennebula/one_driver.c | 6 ++++-- src/openvz/openvz_driver.c | 6 ++++-- src/qemu/qemu_driver.c | 5 +++++ src/test/test_driver.c | 15 ++++++++++----- src/uml/uml_driver.c | 2 ++ 9 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 2f24605..7234cf4 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -115,6 +115,8 @@ struct _virCaps { virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; unsigned int emulatorRequired : 1; + void *(*privateDataAllocFunc)(void); + void (*privateDataFreeFunc)(void *); };
Maybe add a comment here that this should be moved out into a separate object at some point.
ACK.
PS: What's the specific use case for this?
It going to be used by the QEMU monitor re-write I'm working on. That's not ready to send out for review yet, but hopefully in a day or two Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Matthias Bolte