[libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable

Monday, 12 February 2018

Based-on-work-of: John Ferlan <jferlan(a)redhat.com&gt;
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt;
---
 cfg.mk                         |   1 -
 src/conf/virdomainobjlist.c    |   3 +-
 src/conf/virnwfilterobj.c      | 409 +++++++++++++++++++++++++++--------------
 src/conf/virnwfilterobj.h      |   3 -
 src/libvirt_private.syms       |   1 -
 src/nwfilter/nwfilter_driver.c |   4 +-
 6 files changed, 279 insertions(+), 142 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 78f805b27..89779fb05 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -242,7 +242,6 @@ useless_free_options = \
 # y virNWFilterIncludeDefFree
 # n virNWFilterFreeName (returns int)
 # y virNWFilterObjFree
-# n virNWFilterObjListFree FIXME
 # y virNWFilterRuleDefFree
 # n virNWFilterRuleFreeInstanceData (typedef)
 # y virNWFilterRuleInstFree
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 87a742b1e..4d3cc94b3 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -206,7 +206,8 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
 
     virObjectRWLockRead(doms);
     obj = virHashLookup(doms->objsName, name);
-    virObjectRef(obj);
+    if (obj)
+        virObjectRef(obj);
     virObjectRWUnlock(doms);
     if (obj) {
         virObjectLock(obj);
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 6a54628b6..bb4d0a036 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -43,11 +43,20 @@ struct _virNWFilterObj {
 };
 
 static virClassPtr virNWFilterObjClass;
+static virClassPtr virNWFilterObjListClass;
 static void virNWFilterObjDispose(void *obj);
+static void virNWFilterObjListDispose(void *obj);
 
 struct _virNWFilterObjList {
-    size_t count;
-    virNWFilterObjPtr *objs;
+    virObjectRWLockable parent;
+
+    /* uuid string -> virNWFilterObj  mapping
+     * for O(1), lockless lookup-by-uuid */
+    virHashTable *objs;
+
+    /* name -> virNWFilterObj mapping for O(1),
+     * lockless lookup-by-name */
+    virHashTable *objsName;
 };
 
 static int virNWFilterObjOnceInit(void)
@@ -58,6 +67,12 @@ static int virNWFilterObjOnceInit(void)
                                             virNWFilterObjDispose)))
         return -1;
 
+    if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(),
+                                                "virNWFilterObjList",
+                                                sizeof(virNWFilterObjList),
+                                                virNWFilterObjListDispose)))
+        return -1;
+
     return 0;
 }
 
@@ -123,14 +138,14 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
 }
 
 
-void
-virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
+static void
+virNWFilterObjListDispose(void *obj)
 {
-    size_t i;
-    for (i = 0; i < nwfilters->count; i++)
-        virObjectUnref(nwfilters->objs[i]);
-    VIR_FREE(nwfilters->objs);
-    VIR_FREE(nwfilters);
+
+    virNWFilterObjListPtr nwfilters = obj;
+
+    virHashFree(nwfilters->objs);
+    virHashFree(nwfilters->objsName);
 }
 
 
@@ -139,8 +154,18 @@ virNWFilterObjListNew(void)
 {
     virNWFilterObjListPtr nwfilters;
 
-    if (VIR_ALLOC(nwfilters) < 0)
+    if (virNWFilterObjInitialize() < 0)
         return NULL;
+
+    if (!(nwfilters = virObjectRWLockableNew(virNWFilterObjListClass)))
+        return NULL;
+
+    if (!(nwfilters->objs = virHashCreate(10, virObjectFreeHashData)) ||
+        !(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) {
+        virObjectUnref(nwfilters);
+        return NULL;
+    }
+
     return nwfilters;
 }
 
@@ -149,20 +174,35 @@ void
 virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
                          virNWFilterObjPtr obj)
 {
-    size_t i;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
 
+    virUUIDFormat(obj->def->uuid, uuidstr);
+    virObjectRef(obj);
     virObjectUnlock(obj);
 
-    for (i = 0; i < nwfilters->count; i++) {
-        virObjectLock(nwfilters->objs[i]);
-        if (nwfilters->objs[i] == obj) {
-            virNWFilterObjEndAPI(&nwfilters->objs[i]);
+    virObjectRWLockWrite(nwfilters);
+    virObjectLock(obj);
+    virHashRemoveEntry(nwfilters->objs, uuidstr);
+    virHashRemoveEntry(nwfilters->objsName, obj->def->name);
+    virObjectUnlock(obj);
+    virObjectUnref(obj);
+    virObjectRWUnlock(nwfilters);
+}
+
+
+static virNWFilterObjPtr
+virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters,
+                                   const unsigned char *uuid)
+{
+    virNWFilterObjPtr obj = NULL;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+    virUUIDFormat(uuid, uuidstr);
 
-            VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
-            break;
-        }
-        virObjectUnlock(nwfilters->objs[i]);
-    }
+    obj = virHashLookup(nwfilters->objs, uuidstr);
+    if (obj)
+        virObjectRef(obj);
+    return obj;
 }
 
 
@@ -170,20 +210,27 @@ virNWFilterObjPtr
 virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
                              const unsigned char *uuid)
 {
-    size_t i;
     virNWFilterObjPtr obj;
-    virNWFilterDefPtr def;
 
-    for (i = 0; i < nwfilters->count; i++) {
-        obj = nwfilters->objs[i];
+    virObjectRWLockRead(nwfilters);
+    obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuid);
+    virObjectRWUnlock(nwfilters);
+    if (obj)
         virObjectLock(obj);
-        def = obj->def;
-        if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
+    return obj;
+}
 
-    return NULL;
+
+static virNWFilterObjPtr
+virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters,
+                                   const char *name)
+{
+    virNWFilterObjPtr obj;
+
+    obj = virHashLookup(nwfilters->objsName, name);
+    if (obj)
+        virObjectRef(obj);
+    return obj;
 }
 
 
@@ -191,20 +238,14 @@ virNWFilterObjPtr
 virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
                              const char *name)
 {
-    size_t i;
     virNWFilterObjPtr obj;
-    virNWFilterDefPtr def;
 
-    for (i = 0; i < nwfilters->count; i++) {
-        obj = nwfilters->objs[i];
+    virObjectRWLockRead(nwfilters);
+    obj = virNWFilterObjListFindByNameLocked(nwfilters, name);
+    virObjectRWUnlock(nwfilters);
+    if (obj)
         virObjectLock(obj);
-        def = obj->def;
-        if (STREQ_NULLABLE(def->name, name))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
-
-    return NULL;
+    return obj;
 }
 
 
@@ -253,9 +294,10 @@ _virNWFilterObjListDefLoopDetect(virNWFilterObjListPtr nwfilters,
                 break;
             }
 
-            obj = virNWFilterObjListFindByName(nwfilters,
-                                               entry->include->filterref);
+            obj = virNWFilterObjListFindByNameLocked(nwfilters,
+                                                     entry->include->filterref);
             if (obj) {
+                virObjectLock(obj);
                 rc = _virNWFilterObjListDefLoopDetect(nwfilters, obj->def,
                                                       filtername);
                 virNWFilterObjEndAPI(&obj);
@@ -325,51 +367,52 @@ virNWFilterDefEqual(const virNWFilterDef *def1,
 }
 
 
-virNWFilterObjPtr
-virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
-                            virNWFilterDefPtr def)
+static virNWFilterObjPtr
+virNWFilterObjListAssignDefLocked(virNWFilterObjListPtr nwfilters,
+                                  virNWFilterDefPtr def)
 {
-    virNWFilterObjPtr obj;
-    virNWFilterDefPtr objdef;
+    virNWFilterObjPtr obj = NULL;
+    virNWFilterObjPtr ret = NULL;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
 
-    if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
-        objdef = obj->def;
+    virUUIDFormat(def->uuid, uuidstr);
 
-        if (STRNEQ(def->name, objdef->name)) {
+    if ((obj = virNWFilterObjListFindByUUIDLocked(nwfilters, def->uuid))) {
+        virObjectLock(obj);
+
+        if (STRNEQ(def->name, obj->def->name)) {
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("filter with same UUID but different name "
                              "('%s') already exists"),
-                           objdef->name);
-            virNWFilterObjEndAPI(&obj);
-            return NULL;
+                           obj->def->name);
+            goto cleanup;
         }
-        virNWFilterObjEndAPI(&obj);
     } else {
-        if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
-            char uuidstr[VIR_UUID_STRING_BUFLEN];
+        if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
+            virObjectLock(obj);
 
-            objdef = obj->def;
-            virUUIDFormat(objdef->uuid, uuidstr);
+            virUUIDFormat(obj->def->uuid, uuidstr);
             virReportError(VIR_ERR_OPERATION_FAILED,
-                           _("filter '%s' already exists with uuid
%s"),
+                           _("filter '%s' already exists with UUID
%s"),
                            def->name, uuidstr);
-            virNWFilterObjEndAPI(&obj);
-            return NULL;
+            goto cleanup;
         }
     }
 
+    virNWFilterObjEndAPI(&obj);
+
     if (virNWFilterObjListDefLoopDetect(nwfilters, def) < 0) {
         virReportError(VIR_ERR_OPERATION_FAILED,
                        "%s", _("filter would introduce a loop"));
-        return NULL;
+        goto cleanup;
     }
 
 
-    if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
+    if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
+        virObjectLock(obj);
 
-        objdef = obj->def;
-        if (virNWFilterDefEqual(def, objdef)) {
-            virNWFilterDefFree(objdef);
+        if (virNWFilterDefEqual(def, obj->def)) {
+            virNWFilterDefFree(obj->def);
             obj->def = def;
             return obj;
         }
@@ -382,7 +425,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
             return NULL;
         }
 
-        virNWFilterDefFree(objdef);
+        virNWFilterDefFree(obj->def);
         obj->def = def;
         obj->newDef = NULL;
         return obj;
@@ -391,35 +434,114 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
     if (!(obj = virNWFilterObjNew()))
         return NULL;
 
-    if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
-                                nwfilters->count, obj) < 0) {
-        virNWFilterObjEndAPI(&obj);
-        return NULL;
+    if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0)
+        goto cleanup;
+
+    if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) {
+        virHashRemoveEntry(nwfilters->objs, uuidstr);
+        goto cleanup;
     }
     virObjectRef(obj);
+
+    /* Increase refcounter again. We need two references for the
+     * hash tables above and one to return to the caller. */
+    virObjectRef(obj);
     obj->def = def;
 
+    ret = obj;
+    obj = NULL;
+
+ cleanup:
+    virNWFilterObjEndAPI(&obj);
+    return ret;
+}
+
+
+virNWFilterObjPtr
+virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
+                            virNWFilterDefPtr def)
+{
+    virNWFilterObjPtr obj;
+
+    virObjectRWLockWrite(nwfilters);
+    obj = virNWFilterObjListAssignDefLocked(nwfilters, def);
+    virObjectRWUnlock(nwfilters);
     return obj;
 }
 
 
+struct virNWFilterObjListData {
+    virNWFilterObjListFilter filter;
+    virConnectPtr conn;
+    int count;
+};
+
+
+static int
+virNWFilterObjListCount(void *payload,
+                        const void *name ATTRIBUTE_UNUSED,
+                        void *opaque)
+{
+    virNWFilterObjPtr obj = payload;
+    struct virNWFilterObjListData *data = opaque;
+
+    virObjectLock(obj);
+    if (!data->filter ||
+        data->filter(data->conn, obj->def))
+        data->count++;
+    virObjectUnlock(obj);
+    return 0;
+}
+
+
 int
 virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
                                  virConnectPtr conn,
                                  virNWFilterObjListFilter filter)
 {
-    size_t i;
-    int nfilters = 0;
-
-    for (i = 0; i < nwfilters->count; i++) {
-        virNWFilterObjPtr obj = nwfilters->objs[i];
-        virObjectLock(obj);
-        if (!filter || filter(conn, obj->def))
-            nfilters++;
-        virObjectUnlock(obj);
+    struct virNWFilterObjListData data = { filter, conn, 0 };
+
+    virObjectRWLockRead(nwfilters);
+    virHashForEach(nwfilters->objs, virNWFilterObjListCount, &data);
+    virObjectRWUnlock(nwfilters);
+    return data.count;
+}
+
+
+struct virNWFilterNameData {
+    virNWFilterObjListFilter filter;
+    virConnectPtr conn;
+    int oom;
+    int numnames;
+    int maxnames;
+    char **const names;
+};
+
+
+static int
+virNWFilterObjListCopyNames(void *payload,
+                            const void *name ATTRIBUTE_UNUSED,
+                            void *opaque)
+{
+    virNWFilterObjPtr obj = payload;
+    struct virNWFilterNameData *data = opaque;
+
+    if (data->oom)
+        return 0;
+
+    virObjectLock(obj);
+    if (data->filter &&
+        !data->filter(data->conn, obj->def))
+        goto cleanup;
+    if (data->numnames < data->maxnames) {
+        if (VIR_STRDUP(data->names[data->numnames], obj->def->name) < 0)
+            data->oom = 1;
+        else
+            data->numnames++;
     }
-
-    return nfilters;
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
 }
 
 
@@ -430,31 +552,64 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters,
                            char **const names,
                            int maxnames)
 {
-    int nnames = 0;
+    struct virNWFilterNameData data = {filter, conn, 0, 0, maxnames, names};
     size_t i;
-    virNWFilterDefPtr def;
-
-    for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
-        virNWFilterObjPtr obj = nwfilters->objs[i];
-        virObjectLock(obj);
-        def = obj->def;
-        if (!filter || filter(conn, def)) {
-            if (VIR_STRDUP(names[nnames], def->name) < 0) {
-                virObjectUnlock(obj);
-                goto failure;
-            }
-            nnames++;
-        }
-        virObjectUnlock(obj);
+
+    virObjectRWLockRead(nwfilters);
+    virHashForEach(nwfilters->objs, virNWFilterObjListCopyNames, &data);
+    virObjectRWUnlock(nwfilters);
+    if (data.oom) {
+        for (i = 0; i < data.numnames; i++)
+            VIR_FREE(data.names[i]);
+        return -1;
+    }
+
+    return data.numnames;
+}
+
+
+struct virNWFilterListData {
+    virConnectPtr conn;
+    virNWFilterPtr *nwfilters;
+    virNWFilterObjListFilter filter;
+    int nnwfilters;
+    bool error;
+};
+
+
+static int
+virNWFilterObjListPopulate(void *payload,
+                           const void *name ATTRIBUTE_UNUSED,
+                           void *opaque)
+{
+    struct virNWFilterListData *data = opaque;
+    virNWFilterObjPtr obj = payload;
+    virNWFilterPtr nwfilter = NULL;
+
+    if (data->error)
+        return 0;
+
+    virObjectLock(obj);
+
+    if (data->filter &&
+        !data->filter(data->conn, obj->def))
+        goto cleanup;
+
+    if (!data->nwfilters) {
+        data->nnwfilters++;
+        goto cleanup;
     }
 
-    return nnames;
+    if (!(nwfilter = virGetNWFilter(data->conn, obj->def->name,
obj->def->uuid))) {
+        data->error = true;
+        goto cleanup;
+    }
 
- failure:
-    while (--nnames >= 0)
-        VIR_FREE(names[nnames]);
+    data->nwfilters[data->nnwfilters++] = nwfilter;
 
-    return -1;
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
 }
 
 
@@ -464,47 +619,33 @@ virNWFilterObjListExport(virConnectPtr conn,
                          virNWFilterPtr **filters,
                          virNWFilterObjListFilter filter)
 {
-    virNWFilterPtr *tmp_filters = NULL;
-    int nfilters = 0;
-    virNWFilterPtr nwfilter = NULL;
-    virNWFilterObjPtr obj = NULL;
-    virNWFilterDefPtr def;
-    size_t i;
     int ret = -1;
+    struct virNWFilterListData data = {.conn = conn, .nwfilters = NULL,
+        .filter = filter, .nnwfilters = 0, .error = false};
 
-    if (!filters) {
-        ret = nwfilters->count;
+    virObjectRWLockRead(nwfilters);
+    if (filters && VIR_ALLOC_N(data.nwfilters, virHashSize(nwfilters->objs) +
1) < 0)
         goto cleanup;
-    }
 
-    if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0)
+    virHashForEach(nwfilters->objs, virNWFilterObjListPopulate, &data);
+
+    if (data.error)
         goto cleanup;
 
-    for (i = 0; i < nwfilters->count; i++) {
-        obj = nwfilters->objs[i];
-        virObjectLock(obj);
-        def = obj->def;
-        if (!filter || filter(conn, def)) {
-            if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) {
-                virObjectUnlock(obj);
-                goto cleanup;
-            }
-            tmp_filters[nfilters++] = nwfilter;
-        }
-        virObjectUnlock(obj);
+    if (data.nnwfilters) {
+        /* trim the array to the final size */
+        ignore_value(VIR_REALLOC_N(data.nwfilters, data.nnwfilters + 1));
+        *filters = data.nwfilters;
+        data.nwfilters = NULL;
     }
 
-    *filters = tmp_filters;
-    tmp_filters = NULL;
-    ret = nfilters;
-
+    ret = data.nnwfilters;
  cleanup:
-    if (tmp_filters) {
-        for (i = 0; i < nfilters; i ++)
-            virObjectUnref(tmp_filters[i]);
-    }
-    VIR_FREE(tmp_filters);
+    virObjectRWUnlock(nwfilters);
+    while (data.nwfilters && data.nnwfilters)
+        virObjectUnref(data.nwfilters[--data.nnwfilters]);
 
+    VIR_FREE(data.nwfilters);
     return ret;
 }
 
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 0281bc5f5..caff76e9a 100644
--- a/src/conf/virnwfilterobj.h
+++ b/src/conf/virnwfilterobj.h
@@ -56,9 +56,6 @@ virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
 virNWFilterObjListPtr
 virNWFilterObjListNew(void);
 
-void
-virNWFilterObjListFree(virNWFilterObjListPtr nwfilters);
-
 void
 virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
                          virNWFilterObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index edda56f80..fe63defb3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1047,7 +1047,6 @@ virNWFilterObjListExport;
 virNWFilterObjListFindByName;
 virNWFilterObjListFindByUUID;
 virNWFilterObjListFindInstantiateFilter;
-virNWFilterObjListFree;
 virNWFilterObjListGetNames;
 virNWFilterObjListLoadAllConfigs;
 virNWFilterObjListNew;
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index c9bbae422..093844c9e 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -270,7 +270,7 @@ nwfilterStateInitialize(bool privileged,
     virNWFilterIPAddrMapShutdown();
 
  err_free_driverstate:
-    virNWFilterObjListFree(driver->nwfilters);
+    virObjectUnref(driver->nwfilters);
     VIR_FREE(driver);
 
     return -1;
@@ -354,7 +354,7 @@ nwfilterStateCleanup(void)
     }
 
     /* free inactive nwfilters */
-    virNWFilterObjListFree(driver->nwfilters);
+    virObjectUnref(driver->nwfilters);
 
     virMutexDestroy(&driver->lock);
     VIR_FREE(driver);
-- 
2.13.6


    

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable