From cbe7bbf722a4c5b276238d4cc50c2ac5d407f800 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Wed, 29 Apr 2015 14:11:09 +0200 Subject: [PATCH] conf: Refactor domain list collection critical section Until now the virDomainListAllDomains API would lock the domain list and then every single domain object to access and filter it. This would potentially allow a unresponsive VM to block the whole daemon if a *listAllDomains call would get stuck. To avoid this problem this patch collects a list of referenced domain objects first from the list and then unlocks it right away. The expensive operation requiring locking of the domain object is executed after the list lock is dropped. While a single blocked domain will still lock up a listAllDomains call, the domain list won't be held locked and thus other APIs won't be blocked. Additionally this patch also fixes the lookup code, where we'd ignore the vm->removing flag and thus potentially return domain objects that would be deleted very soon so calling any API wouldn't make sense. As other clients also could benefit from operating on a list of domain objects rather than the public domain descriptors a new intermediate API - virDomainObjListCollect - is introduced by this patch. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1181074 --- src/conf/domain_conf.c | 142 ++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 6 ++ src/libvirt_private.syms | 1 + 3 files changed, 95 insertions(+), 54 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 460f67b590..0fea61a642 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23031,98 +23031,132 @@ virDomainObjMatchFilter(virDomainObjPtr vm, struct virDomainListData { - virConnectPtr conn; - virDomainPtr *domains; - virDomainObjListACLFilter filter; - unsigned int flags; - int ndomains; - bool error; + virDomainObjPtr *vms; + size_t nvms; }; + static void -virDomainListPopulate(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +virDomainObjListCollectIterator(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { struct virDomainListData *data = opaque; - virDomainObjPtr vm = payload; - virDomainPtr dom; - if (data->error) - return; + data->vms[data->nvms++] = virObjectRef(payload); +} - virObjectLock(vm); - /* check if the domain matches the filter */ - /* filter by the callback function (access control checks) */ - if (data->filter != NULL && - !data->filter(data->conn, vm->def)) - goto cleanup; +static void +virDomainObjListFilter(virDomainObjPtr **list, + size_t *nvms, + virConnectPtr conn, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + size_t i = 0; - if (!virDomainObjMatchFilter(vm, data->flags)) - goto cleanup; + while (i < *nvms) { + virDomainObjPtr vm = (*list)[i]; - /* just count the machines */ - if (!data->domains) { - data->ndomains++; - goto cleanup; + virObjectLock(vm); + + /* do not list the object if: + * 1) it's being removed. + * 2) connection does not have ACL to see it + * 3) it doesn't match the filter + */ + if (vm->removing || + (filter && !filter(conn, vm->def)) || + !virDomainObjMatchFilter(vm, flags)) { + virObjectUnlock(vm); + virObjectUnref(vm); + VIR_DELETE_ELEMENT(*list, i, *nvms); + continue; + } + + virObjectUnlock(vm); + i++; } +} - if (!(dom = virGetDomain(data->conn, vm->def->name, vm->def->uuid))) { - data->error = true; - goto cleanup; + +int +virDomainObjListCollect(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectLock(domlist); + if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) { + virObjectUnlock(domlist); + return -1; } - dom->id = vm->def->id; + virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); + virObjectUnlock(domlist); - data->domains[data->ndomains++] = dom; + virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags); - cleanup: - virObjectUnlock(vm); - return; + *nvms = data.nvms; + *vms = data.vms; + + return 0; } int -virDomainObjListExport(virDomainObjListPtr doms, +virDomainObjListExport(virDomainObjListPtr domlist, virConnectPtr conn, virDomainPtr **domains, virDomainObjListACLFilter filter, unsigned int flags) { + virDomainObjPtr *vms = NULL; + virDomainPtr *doms = NULL; + size_t nvms = 0; + size_t i; int ret = -1; - struct virDomainListData data = { - conn, NULL, - filter, - flags, 0, false - }; + if (virDomainObjListCollect(domlist, conn, &vms, &nvms, filter, flags) < 0) + return -1; - virObjectLock(doms); - if (domains && - VIR_ALLOC_N(data.domains, virHashSize(doms->objs) + 1) < 0) - goto cleanup; + if (domains) { + if (VIR_ALLOC_N(doms, nvms + 1) < 0) + goto cleanup; - virHashForEach(doms->objs, virDomainListPopulate, &data); + for (i = 0; i < nvms; i++) { + virDomainObjPtr vm = vms[i]; - if (data.error) - goto cleanup; + virObjectLock(vm); + + if (!(doms[i] = virGetDomain(conn, vm->def->name, vm->def->uuid))) { + virObjectUnlock(vm); + goto cleanup; + } + + doms[i]->id = vm->def->id; + + virObjectUnlock(vm); + } - if (data.domains) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(data.domains, data.ndomains + 1)); - *domains = data.domains; - data.domains = NULL; + *domains = doms; + doms = NULL; } - ret = data.ndomains; + ret = nvms; cleanup: - virObjectListFree(data.domains); - virObjectUnlock(doms); + virObjectListFree(doms); + virObjectListFreeCount(vms, nvms); return ret; } + virSecurityLabelDefPtr virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7173f69826..8bc9536c58 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3051,6 +3051,12 @@ VIR_ENUM_DECL(virDomainStartupPolicy) VIR_CONNECT_LIST_DOMAINS_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT) +int virDomainObjListCollect(virDomainObjListPtr doms, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags); int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, virDomainPtr **domains, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d7cac20902..9a7a9448f0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -385,6 +385,7 @@ virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; +virDomainObjListCollect; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName; -- GitLab