conf: use a hash table for storing nwfilter object list

The current use of an array for nwfilter objects requires
the caller to iterate over all elements to find a filter,
and also requires locking each filter.

Switching to a pair of hash tables enables O(1) lookups
both by name and uuid, with no locking required.

Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

src/conf/virnwfilterobj.c

@@ -43,10 +43,14 @@ struct _virNWFilterObj {
 };
 
 struct _virNWFilterObjList {
-    size_t count;
+    /* uuid string -> virNWFilterObj  mapping
-    virNWFilterObj **objs;
+     * for O(1), lookup-by-uuid */
-};
+    GHashTable *objs;
 
+    /* name -> virNWFilterObj mapping for O(1),
+     * lookup-by-name */
+    GHashTable *objsName;
+};
 
 static virNWFilterObj *
 virNWFilterObjNew(void)
@@ -106,10 +110,8 @@ virNWFilterObjFree(virNWFilterObj *obj)
 void
 virNWFilterObjListFree(virNWFilterObjList *nwfilters)
 {
-    size_t i;
+    g_hash_table_unref(nwfilters->objs);
-    for (i = 0; i < nwfilters->count; i++)
+    g_hash_table_unref(nwfilters->objsName);
-        virNWFilterObjFree(nwfilters->objs[i]);
-    g_free(nwfilters->objs);
     g_free(nwfilters);
 }
 
@@ -117,7 +119,17 @@ virNWFilterObjListFree(virNWFilterObjList *nwfilters)
 virNWFilterObjList *
 virNWFilterObjListNew(void)
 {
-    return g_new0(virNWFilterObjList, 1);
+    virNWFilterObjList *nwfilters = g_new0(virNWFilterObjList, 1);
+
+    /* virNWFilterObj is not ref counted, so we rely fact that
+     * an instance will always exist in both hash tables, or
+     * neither hash table. Thus we only need to have a destroy
+     * callback for one of the two hash tables.
+     */
+    nwfilters->objs = virHashNew((GDestroyNotify)virNWFilterObjFree);
+    nwfilters->objsName = virHashNew(NULL);
+
+    return nwfilters;
 }
 
 
@@ -125,21 +137,14 @@ void
 virNWFilterObjListRemove(virNWFilterObjList *nwfilters,
                          virNWFilterObj *obj)
 {
-    size_t i;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+    virUUIDFormat(obj->def->uuid, uuidstr);
 
     virNWFilterObjUnlock(obj);
 
-    for (i = 0; i < nwfilters->count; i++) {
+    g_hash_table_remove(nwfilters->objsName, obj->def->name);
-        virNWFilterObjLock(nwfilters->objs[i]);
+    g_hash_table_remove(nwfilters->objs, uuidstr);
-        if (nwfilters->objs[i] == obj) {
-            virNWFilterObjUnlock(nwfilters->objs[i]);
-            virNWFilterObjFree(nwfilters->objs[i]);
-
-            VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
-            break;
-        }
-        virNWFilterObjUnlock(nwfilters->objs[i]);
-    }
 }
 
 
@@ -147,20 +152,16 @@ virNWFilterObj *
 virNWFilterObjListFindByUUID(virNWFilterObjList *nwfilters,
                              const unsigned char *uuid)
 {
-    size_t i;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
     virNWFilterObj *obj;
-    virNWFilterDef *def;
 
-    for (i = 0; i < nwfilters->count; i++) {
+    virUUIDFormat(uuid, uuidstr);
-        obj = nwfilters->objs[i];
+
+    obj = g_hash_table_lookup(nwfilters->objs, uuidstr);
+    if (obj)
         virNWFilterObjLock(obj);
-        def = obj->def;
-        if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
-            return obj;
-        virNWFilterObjUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
 }
 
 
@@ -168,20 +169,13 @@ virNWFilterObj *
 virNWFilterObjListFindByName(virNWFilterObjList *nwfilters,
                              const char *name)
 {
-    size_t i;
     virNWFilterObj *obj;
-    virNWFilterDef *def;
 
-    for (i = 0; i < nwfilters->count; i++) {
+    obj = g_hash_table_lookup(nwfilters->objsName, name);
-        obj = nwfilters->objs[i];
+    if (obj)
         virNWFilterObjLock(obj);
-        def = obj->def;
-        if (STREQ_NULLABLE(def->name, name))
-            return obj;
-        virNWFilterObjUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
 }
 
 
@@ -308,6 +302,7 @@ virNWFilterObjListAssignDef(virNWFilterObjList *nwfilters,
 {
     virNWFilterObj *obj;
     virNWFilterDef *objdef;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
 
     if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
         objdef = obj->def;
@@ -323,8 +318,6 @@ virNWFilterObjListAssignDef(virNWFilterObjList *nwfilters,
         virNWFilterObjUnlock(obj);
     } else {
         if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
-            char uuidstr[VIR_UUID_STRING_BUFLEN];
-
             objdef = obj->def;
             virUUIDFormat(objdef->uuid, uuidstr);
             virReportError(VIR_ERR_OPERATION_FAILED,
@@ -368,7 +361,10 @@ virNWFilterObjListAssignDef(virNWFilterObjList *nwfilters,
     if (!(obj = virNWFilterObjNew()))
         return NULL;
 
-    VIR_APPEND_ELEMENT_COPY(nwfilters->objs, nwfilters->count, obj);
+    virUUIDFormat(def->uuid, uuidstr);
+
+    g_hash_table_insert(nwfilters->objs, g_strdup(uuidstr), obj);
+    g_hash_table_insert(nwfilters->objsName, g_strdup(def->name), obj);
 
     obj->def = def;
 
@@ -376,25 +372,68 @@ virNWFilterObjListAssignDef(virNWFilterObjList *nwfilters,
 }
 
 
+struct virNWFilterObjListData {
+    virNWFilterObjListFilter filter;
+    virConnectPtr conn;
+    int count;
+};
+
+
+static void
+virNWFilterObjListCount(void *payload,
+                        void *key G_GNUC_UNUSED,
+                        void *opaque)
+{
+    virNWFilterObj *obj = payload;
+    struct virNWFilterObjListData *data = opaque;
+    VIR_LOCK_GUARD lock = virObjectLockGuard(obj);
+
+    if (data->filter(data->conn, obj->def))
+        data->count++;
+}
+
+
 int
 virNWFilterObjListNumOfNWFilters(virNWFilterObjList *nwfilters,
                                  virConnectPtr conn,
                                  virNWFilterObjListFilter filter)
 {
-    size_t i;
+    struct virNWFilterObjListData data = { filter, conn, 0 };
-    int nfilters = 0;
 
-    for (i = 0; i < nwfilters->count; i++) {
+    g_hash_table_foreach(nwfilters->objs,
-        virNWFilterObj *obj = nwfilters->objs[i];
+                         virNWFilterObjListCount,
-        virNWFilterObjLock(obj);
+                         &data);
-        if (!filter || filter(conn, obj->def))
+    return data.count;
-            nfilters++;
-        virNWFilterObjUnlock(obj);
 }
 
-    return nfilters;
-}
 
+struct virNWFilterNameData {
+    virNWFilterObjListFilter filter;
+    virConnectPtr conn;
+    int numnames;
+    int maxnames;
+    char **const names;
+};
+
+
+static void
+virNWFilterObjListCopyNames(void *payload,
+                            void *key G_GNUC_UNUSED,
+                            void *opaque)
+{
+    virNWFilterObj *obj = payload;
+    struct virNWFilterNameData *data = opaque;
+    VIR_LOCK_GUARD lock = virObjectLockGuard(obj);
+
+    if (data->filter &&
+        !data->filter(data->conn, obj->def))
+        return;
+
+    if (data->numnames < data->maxnames) {
+        data->names[data->numnames] = g_strdup(obj->def->name);
+        data->numnames++;
+    }
+}
 
 int
 virNWFilterObjListGetNames(virNWFilterObjList *nwfilters,
@@ -403,22 +442,80 @@ virNWFilterObjListGetNames(virNWFilterObjList *nwfilters,
                            char **const names,
                            int maxnames)
 {
-    int nnames = 0;
+    struct virNWFilterNameData data =
-    size_t i;
+        { filter, conn, 0, maxnames, names };
-    virNWFilterDef *def;
+
+    g_hash_table_foreach(nwfilters->objs,
+                         virNWFilterObjListCopyNames,
+                         &data);
+
+    return data.numnames;
+}
+
+
+struct virNWFilterListData {
+    virNWFilterObj **filters;
+    size_t nfilters;
+};
+
+
+static void
+virNWFilterObjListCollectIterator(void *payload,
+                                  void *key G_GNUC_UNUSED,
+                                  void *opaque)
+{
+    struct virNWFilterListData *data = opaque;
+    virNWFilterObj *obj = payload;
 
-    for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
-        virNWFilterObj *obj = nwfilters->objs[i];
     virNWFilterObjLock(obj);
-        def = obj->def;
+    data->filters[data->nfilters++] = payload;
-        if (!filter || filter(conn, def)) {
-            names[nnames] = g_strdup(def->name);
-            nnames++;
-        }
-        virNWFilterObjUnlock(obj);
 }
 
-    return nnames;
+
+static void
+virNWFilterObjListFilterApply(virNWFilterObj ***list,
+                              size_t *nfilters,
+                              virConnectPtr conn,
+                              virNWFilterObjListFilter filter)
+{
+    size_t i = 0;
+
+    while (i < *nfilters) {
+        virNWFilterObj *obj = (*list)[i];
+
+        if (filter && !filter(conn, obj->def)) {
+            VIR_DELETE_ELEMENT(*list, i, *nfilters);
+            virNWFilterObjUnlock(obj);
+            continue;
+        }
+
+        i++;
+    }
+}
+
+
+static int
+virNWFilterObjListCollect(virNWFilterObjList *nwfilters,
+                          virConnectPtr conn,
+                          virNWFilterObj ***filters,
+                          size_t *nfilters,
+                          virNWFilterObjListFilter filter)
+{
+    struct virNWFilterListData data = { NULL, 0 };
+
+    data.filters = g_new0(virNWFilterObj *,
+                          g_hash_table_size(nwfilters->objs));
+
+    g_hash_table_foreach(nwfilters->objs,
+                         virNWFilterObjListCollectIterator,
+                         &data);
+
+    virNWFilterObjListFilterApply(&data.filters, &data.nfilters, conn, filter);
+
+    *nfilters = data.nfilters;
+    *filters = data.filters;
+
+    return 0;
 }
 
 
@@ -429,33 +526,27 @@ virNWFilterObjListExport(virConnectPtr conn,
                          virNWFilterObjListFilter filter)
 {
     virNWFilterPtr *tmp_filters = NULL;
-    int nfilters = 0;
+    virNWFilterObj **objs = NULL;
-    virNWFilterPtr nwfilter = NULL;
+    size_t nfilters = 0;
-    virNWFilterObj *obj = NULL;
-    virNWFilterDef *def;
     size_t i;
     int ret = -1;
 
+    if (virNWFilterObjListCollect(nwfilters, conn, &objs, &nfilters, filter) < 0)
+        return -1;
+
     if (!filters) {
-        ret = nwfilters->count;
+        ret = nfilters;
         goto cleanup;
     }
 
-    tmp_filters = g_new0(virNWFilterPtr, nwfilters->count + 1);
+    tmp_filters = g_new0(virNWFilterPtr, nfilters + 1);
 
-    for (i = 0; i < nwfilters->count; i++) {
+    for (i = 0; i < nfilters; i++) {
-        obj = nwfilters->objs[i];
+        tmp_filters[i] = virGetNWFilter(conn, objs[i]->def->name, objs[i]->def->uuid);
-        virNWFilterObjLock(obj);
+
-        def = obj->def;
+        if (!tmp_filters[i])
-        if (!filter || filter(conn, def)) {
-            if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) {
-                virNWFilterObjUnlock(obj);
             goto cleanup;
     }
-            tmp_filters[nfilters++] = nwfilter;
-        }
-        virNWFilterObjUnlock(obj);
-    }
 
     *filters = g_steal_pointer(&tmp_filters);
     ret = nfilters;
@@ -466,7 +557,8 @@ virNWFilterObjListExport(virConnectPtr conn,
             virObjectUnref(tmp_filters[i]);
     }
     VIR_FREE(tmp_filters);
-
+    for (i = 0; i < nfilters; i++)
+        virNWFilterObjUnlock(objs[i]);
     return ret;
 }
 

src/nwfilter/nwfilter_gentech_driver.c

@@ -59,13 +59,7 @@ static virNWFilterTechDriver *filter_tech_drivers[] = {
  * Serializes instantiation of filters.
  *
  * When instantiating a filter, we need to resolve references
- * to other filters and acquire locks on them. The act of
+ * to other filters and acquire locks on them.
- * looking up a filter requires traversing an array, locking
- * each filter in turn until we find the one we want.
- *
- * The mere act of finding a filter by name, while holding
- * a lock on another filter, introduces deadlocks due to
- * varying lock ordering.
  *
  * We retain a lock on the referenced filter once found.
  * The order in which the locks are acquired depends on