Re: [libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
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, &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 @@ virNWFilte
[libvirt] [PATCH 5/6] nwfilter: Convert _virNWFilterObjList to use virObjectRWLockable
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(&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,