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

2018-02-15 Thread Stefan Berger 

On 02/12/2018 05:52 AM, Michal Privoznik wrote:



@@ -430,31 +552,64 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr 
nwfilters,
 char **const names,
 int maxnames)
  {
-int nnames = 0;
+struct virNWFilterNameData data = {filter, conn, 0, 0, maxnames, names};



Here you don't user .filter = filter while further below you.

The rest looks all good to me.


  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, );
+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, );
+
+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 @@

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

2018-02-12 Thread Michal Privoznik 
Based-on-work-of: John Ferlan 
Signed-off-by: Michal Privoznik 
---
 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(>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