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 c2b7d6a..66fe470 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22993,98 +22993,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 bbfe96e..ac11a20 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3048,6 +3048,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 d7cac20..9a7a944 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -385,6 +385,7 @@ virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; +virDomainObjListCollect; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName; -- 2.3.5 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list