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

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 460f67b59031d31f7ceccb8c9094f3ecd8a6ec13..0fea61a642ae34f2a7acc2972b3733d7c6a613e9 100644 (file)
--- 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 7173f69826cc05f912f13a11149503d270f9a755..8bc9536c58949b5cd26f803a15c51a91f1820477 100644 (file)
--- 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 d7cac209026a183b7c13b8ce48468237d4c160f7..9a7a9448f060473fe32bd67babd0ce2e3130bda0 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -385,6 +385,7 @@ virDomainObjGetMetadata;
 virDomainObjGetPersistentDef;
 virDomainObjGetState;
 virDomainObjListAdd;
+virDomainObjListCollect;
 virDomainObjListExport;
 virDomainObjListFindByID;
 virDomainObjListFindByName;