Implement the self locking object list for nwfilter object lists
that uses two hash tables to store the nwfilter object by UUID or
by Name.
As part of this alter the uuid argument to virNWFilterObjLookupByUUID
to expect an already formatted uuidstr.
Alter the existing list traversal code to implement the hash table
find/lookup/search functionality.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/conf/virnwfilterobj.c | 402 ++++++++++++++++++++++++++++-------------
src/conf/virnwfilterobj.h | 2 +-
src/nwfilter/nwfilter_driver.c | 5 +-
3 files changed, 282 insertions(+), 127 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 6b4758656..a4e6a03d2 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -43,12 +43,21 @@ struct _virNWFilterObj {
};
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 virClassPtr virNWFilterObjClass;
+static virClassPtr virNWFilterObjListClass;
static void virNWFilterObjDispose(void *opaque);
+static void virNWFilterObjListDispose(void *opaque);
static int
@@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void)
virNWFilterObjDispose)))
return -1;
+ if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(),
+ "virNWFilterObjList",
+ sizeof(virNWFilterObjList),
+ virNWFilterObjListDispose)))
+ return -1;
+
return 0;
}
@@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque)
}
+static void
+virNWFilterObjListDispose(void *opaque)
+{
+ virNWFilterObjListPtr nwfilters = opaque;
+
+ virHashFree(nwfilters->objs);
+ virHashFree(nwfilters->objsName);
+}
+
+
void
virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
{
- size_t i;
- for (i = 0; i < nwfilters->count; i++)
- virObjectUnref(nwfilters->objs[i]);
- VIR_FREE(nwfilters->objs);
- VIR_FREE(nwfilters);
+ virObjectUnref(nwfilters);
}
@@ -160,8 +181,23 @@ 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))) {
+ virObjectUnref(nwfilters);
+ return NULL;
+ }
+
+ if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) {
+ virHashFree(nwfilters->objs);
+ virObjectUnref(nwfilters);
return NULL;
+ }
+
return nwfilters;
}
@@ -170,83 +206,105 @@ void
virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
virNWFilterObjPtr obj)
{
- size_t i;
-
- virObjectRWUnlock(obj);
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virNWFilterDefPtr def;
- for (i = 0; i < nwfilters->count; i++) {
- virObjectRWLockWrite(nwfilters->objs[i]);
- if (nwfilters->objs[i] == obj) {
- virObjectRWUnlock(nwfilters->objs[i]);
- virObjectUnref(nwfilters->objs[i]);
+ if (!obj)
+ return;
+ def = obj->def;
- VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
- break;
- }
- virObjectRWUnlock(nwfilters->objs[i]);
- }
+ virUUIDFormat(def->uuid, uuidstr);
+ virObjectRef(obj);
+ virObjectRWUnlock(obj);
+ virObjectRWLockWrite(nwfilters);
+ virObjectRWLockWrite(obj);
+ virHashRemoveEntry(nwfilters->objs, uuidstr);
+ virHashRemoveEntry(nwfilters->objsName, def->name);
+ virObjectRWUnlock(obj);
+ virObjectUnref(obj);
+ virObjectRWUnlock(nwfilters);
}
/**
- * virNWFilterObjListFindByUUID
+ * virNWFilterObjListFindByUUID[Locked]
* @nwfilters: Pointer to filter list
- * @uuid: UUID to use to lookup the object
+ * @uuidstr: UUID to use to lookup the object
+ *
+ * The static [Locked] version would only be used when the Object List is
+ * already locked, such as is the case during virNWFilterObjListAssignDef.
+ * The caller is thus responsible for locking the object.
*
* Search for the object by uuidstr in the hash table and return a read
* locked copy of the object.
*
+ * Returns: A reffed object or NULL on error
+ */
+static virNWFilterObjPtr
+virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters,
+ const char *uuidstr)
+{
+ return virObjectRef(virHashLookup(nwfilters->objs, uuidstr));
+}
+
+
+/*
* Returns: A reffed and read locked object or NULL on error
*/
virNWFilterObjPtr
virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
- const unsigned char *uuid)
+ const char *uuidstr)
{
- size_t i;
virNWFilterObjPtr obj;
- virNWFilterDefPtr def;
- for (i = 0; i < nwfilters->count; i++) {
- obj = nwfilters->objs[i];
+ virObjectRWLockRead(nwfilters);
+ obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuidstr);
+ virObjectRWUnlock(nwfilters);
+ if (obj)
virObjectRWLockRead(obj);
- def = obj->def;
- if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
- return virObjectRef(obj);
- virObjectRWUnlock(obj);
- }
- return NULL;
+ return obj;
}
/**
- * virNWFilterObjListFindByName
+ * virNWFilterObjListFindByName[Locked]
* @nwfilters: Pointer to filter list
* @name: filter name to use to lookup the object
*
+ * The static [Locked] version would only be used when the Object List is
+ * already locked, such as is the case during virNWFilterObjListAssignDef.
+ * The caller is thus responsible for locking the object.
+ *
* Search for the object by name in the hash table and return a read
* locked copy of the object.
*
+ * Returns: A reffed object or NULL on error
+ */
+static virNWFilterObjPtr
+virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters,
+ const char *name)
+{
+ return virObjectRef(virHashLookup(nwfilters->objsName, name));
+}
+
+
+/*
* Returns: A reffed and read locked object or NULL on error
*/
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)
virObjectRWLockRead(obj);
- def = obj->def;
- if (STREQ_NULLABLE(def->name, name))
- return virObjectRef(obj);
- virObjectRWUnlock(obj);
- }
- return NULL;
+ return obj;
}
@@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
{
virNWFilterObjPtr obj;
virNWFilterDefPtr objdef;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
- if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
+ virUUIDFormat(def->uuid, uuidstr);
+
+ if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) {
objdef = obj->def;
if (STRNEQ(def->name, objdef->name)) {
@@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
virNWFilterObjEndAPI(&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,
_("filter '%s' already exists with uuid %s"),
def->name, uuidstr);
@@ -424,11 +482,13 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
return NULL;
}
-
- /* Get a READ lock and immediately promote to WRITE while we adjust
- * data within. */
- if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
- virNWFilterObjPromoteToWrite(obj);
+ /* We're about to make some changes to objects on the list - so get
+ * the list READ lock in order to Find the object and WRITE lock it
+ * while we adjust data within. */
+ virObjectRWLockRead(nwfilters);
+ if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
+ virObjectRWUnlock(nwfilters);
+ virObjectRWLockWrite(obj);
objdef = obj->def;
if (virNWFilterDefEqual(def, objdef)) {
@@ -458,37 +518,112 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
return obj;
}
+ /* Promote the nwfilters to add a new object */
+ virObjectRWUnlock(nwfilters);
+ virObjectRWLockWrite(nwfilters);
if (!(obj = virNWFilterObjNew()))
- return NULL;
+ goto cleanup;
- if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
- nwfilters->count, obj) < 0) {
- virNWFilterObjEndAPI(&obj);
- return NULL;
+ if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0)
+ goto error;
+ virObjectRef(obj);
+
+ if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) {
+ virHashRemoveEntry(nwfilters->objs, uuidstr);
+ goto error;
}
+ virObjectRef(obj);
+
obj->def = def;
- return virObjectRef(obj);
+ cleanup:
+ virObjectRWUnlock(nwfilters);
+ return obj;
+
+ error:
+ virObjectRWUnlock(obj);
+ virObjectUnref(obj);
+ virObjectRWUnlock(nwfilters);
+ return NULL;
}
+struct virNWFilterCountData {
+ virConnectPtr conn;
+ virNWFilterObjListFilter filter;
+ int nelems;
+};
+
+static int
+virNWFilterObjListNumOfNWFiltersCallback(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+ virNWFilterObjPtr obj = payload;
+ struct virNWFilterCountData *data = opaque;
+
+ virObjectRWLockRead(obj);
+ if (!data->filter || data->filter(data->conn, obj->def))
+ data->nelems++;
+ virObjectRWUnlock(obj);
+ return 0;
+}
+
int
virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
virConnectPtr conn,
virNWFilterObjListFilter filter)
{
- size_t i;
- int nfilters = 0;
+ struct virNWFilterCountData data = { .conn = conn,
+ .filter = filter, .nelems = 0 };
- for (i = 0; i < nwfilters->count; i++) {
- virNWFilterObjPtr obj = nwfilters->objs[i];
- virObjectRWLockRead(obj);
- if (!filter || filter(conn, obj->def))
- nfilters++;
- virObjectRWUnlock(obj);
+ virObjectRWLockRead(nwfilters);
+ virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback,
+ &data);
+ virObjectRWUnlock(nwfilters);
+
+ return data.nelems;
+}
+
+
+struct virNWFilterListData {
+ virConnectPtr conn;
+ virNWFilterObjListFilter filter;
+ int nelems;
+ char **elems;
+ int maxelems;
+ bool error;
+};
+
+static int
+virNWFilterObjListGetNamesCallback(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+ virNWFilterObjPtr obj = payload;
+ virNWFilterDefPtr def;
+ struct virNWFilterListData *data = opaque;
+
+ if (data->error)
+ return 0;
+
+ if (data->maxelems >= 0 && data->nelems == data->maxelems)
+ return 0;
+
+ virObjectRWLockRead(obj);
+ def = obj->def;
+
+ if (!data->filter || data->filter(data->conn, def)) {
+ if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) {
+ data->error = true;
+ goto cleanup;
+ }
+ data->nelems++;
}
- return nfilters;
+ cleanup:
+ virObjectRWUnlock(obj);
+ return 0;
}
@@ -499,82 +634,103 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters,
char **const names,
int maxnames)
{
- int nnames = 0;
- size_t i;
- virNWFilterDefPtr def;
+ struct virNWFilterListData data = { .conn = conn, .filter = filter,
+ .nelems = 0, .elems = names, .maxelems = maxnames, .error = false };
- for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
- virNWFilterObjPtr obj = nwfilters->objs[i];
- virObjectRWLockRead(obj);
- def = obj->def;
- if (!filter || filter(conn, def)) {
- if (VIR_STRDUP(names[nnames], def->name) < 0) {
- virObjectRWUnlock(obj);
- goto failure;
- }
- nnames++;
- }
- virObjectRWUnlock(obj);
- }
+ virObjectRWLockRead(nwfilters);
+ virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data);
+ virObjectRWUnlock(nwfilters);
- return nnames;
+ if (data.error)
+ goto error;
- failure:
- while (--nnames >= 0)
- VIR_FREE(names[nnames]);
+ return data.nelems;
+ error:
+ while (--data.nelems >= 0)
+ VIR_FREE(data.elems[data.nelems]);
return -1;
}
+struct virNWFilterExportData {
+ virConnectPtr conn;
+ virNWFilterObjListFilter filter;
+ virNWFilterPtr *filters;
+ int nfilters;
+ bool error;
+};
+
+static int
+virNWFilterObjListExportCallback(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+ virNWFilterObjPtr obj = payload;
+ virNWFilterDefPtr def;
+
+ struct virNWFilterExportData *data = opaque;
+ virNWFilterPtr nwfilter;
+
+ if (data->error)
+ return 0;
+
+ virObjectRWLockRead(obj);
+ def = obj->def;
+
+ if (data->filter && !data->filter(data->conn, def))
+ goto cleanup;
+
+ if (!data->filters) {
+ data->nfilters++;
+ goto cleanup;
+ }
+
+ if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) {
+ data->error = true;
+ goto cleanup;
+ }
+ data->filters[data->nfilters++] = nwfilter;
+
+ cleanup:
+ virObjectRWUnlock(obj);
+ return 0;
+}
+
+
int
virNWFilterObjListExport(virConnectPtr conn,
virNWFilterObjListPtr nwfilters,
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 virNWFilterExportData data = { .conn = conn, .filter = filter,
+ .filters = NULL, .nfilters = 0, .error = false };
- if (!filters) {
- ret = nwfilters->count;
- goto cleanup;
+ virObjectRWLockRead(nwfilters);
+ if (filters &&
+ VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) {
+ virObjectRWUnlock(nwfilters);
+ return -1;
}
- if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0)
- goto cleanup;
+ virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data);
+ virObjectRWUnlock(nwfilters);
- for (i = 0; i < nwfilters->count; i++) {
- obj = nwfilters->objs[i];
- virObjectRWLockRead(obj);
- def = obj->def;
- if (!filter || filter(conn, def)) {
- if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) {
- virObjectRWUnlock(obj);
- goto cleanup;
- }
- tmp_filters[nfilters++] = nwfilter;
- }
- virObjectRWUnlock(obj);
+ if (data.error)
+ goto cleanup;
+
+ if (data.filters) {
+ /* trim the array to the final size */
+ ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1));
+ *filters = data.filters;
}
- *filters = tmp_filters;
- tmp_filters = NULL;
- ret = nfilters;
+ return data.nfilters;
cleanup:
- if (tmp_filters) {
- for (i = 0; i < nfilters; i ++)
- virObjectUnref(tmp_filters[i]);
- }
- VIR_FREE(tmp_filters);
-
- return ret;
+ virObjectListFree(data.filters);
+ return -1;
}
diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
index 0281bc5f5..cabb42a71 100644
--- a/src/conf/virnwfilterobj.h
+++ b/src/conf/virnwfilterobj.h
@@ -65,7 +65,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
virNWFilterObjPtr
virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
- const unsigned char *uuid);
+ const char *uuidstr);
virNWFilterObjPtr
virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index b38740b15..b24f59379 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -369,11 +369,10 @@ nwfilterObjFromNWFilter(const unsigned char *uuid)
virNWFilterObjPtr obj;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid))) {
- virUUIDFormat(uuid, uuidstr);
+ virUUIDFormat(uuid, uuidstr);
+ if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuidstr)))
virReportError(VIR_ERR_NO_NWFILTER,
_("no nwfilter with matching uuid '%s'"), uuidstr);
- }
return obj;
}
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/08/2017 09:01 AM, John Ferlan wrote:
> Implement the self locking object list for nwfilter object lists
> that uses two hash tables to store the nwfilter object by UUID or
> by Name.
>
> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
> to expect an already formatted uuidstr.
>
> Alter the existing list traversal code to implement the hash table
> find/lookup/search functionality.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
> src/conf/virnwfilterobj.c | 402 ++++++++++++++++++++++++++++-------------
> src/conf/virnwfilterobj.h | 2 +-
> src/nwfilter/nwfilter_driver.c | 5 +-
> 3 files changed, 282 insertions(+), 127 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 6b4758656..a4e6a03d2 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -43,12 +43,21 @@ struct _virNWFilterObj {
> };
>
> 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 virClassPtr virNWFilterObjClass;
> +static virClassPtr virNWFilterObjListClass;
> static void virNWFilterObjDispose(void *opaque);
> +static void virNWFilterObjListDispose(void *opaque);
>
>
> static int
> @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void)
> virNWFilterObjDispose)))
> return -1;
>
> + if (!(virNWFilterObjListClass = virClassNew(virClassForObjectRWLockable(),
> + "virNWFilterObjList",
> + sizeof(virNWFilterObjList),
> + virNWFilterObjListDispose)))
> + return -1;
> +
> return 0;
> }
>
> @@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque)
> }
>
>
> +static void
> +virNWFilterObjListDispose(void *opaque)
> +{
> + virNWFilterObjListPtr nwfilters = opaque;
> +
> + virHashFree(nwfilters->objs);
> + virHashFree(nwfilters->objsName);
> +}
> +
> +
> void
> virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
> {
> - size_t i;
> - for (i = 0; i < nwfilters->count; i++)
> - virObjectUnref(nwfilters->objs[i]);
> - VIR_FREE(nwfilters->objs);
> - VIR_FREE(nwfilters);
> + virObjectUnref(nwfilters);
> }
>
>
> @@ -160,8 +181,23 @@ 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))) {
> + virObjectUnref(nwfilters);
> + return NULL;
> + }
> +
> + if (!(nwfilters->objsName = virHashCreate(10, virObjectFreeHashData))) {
> + virHashFree(nwfilters->objs);
> + virObjectUnref(nwfilters);
> return NULL;
> + }
> +
> return nwfilters;
> }
>
> @@ -170,83 +206,105 @@ void
> virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
> virNWFilterObjPtr obj)
> {
> - size_t i;
> -
> - virObjectRWUnlock(obj);
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virNWFilterDefPtr def;
>
> - for (i = 0; i < nwfilters->count; i++) {
> - virObjectRWLockWrite(nwfilters->objs[i]);
> - if (nwfilters->objs[i] == obj) {
> - virObjectRWUnlock(nwfilters->objs[i]);
> - virObjectUnref(nwfilters->objs[i]);
> + if (!obj)
> + return;
> + def = obj->def;
>
> - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
> - break;
> - }
> - virObjectRWUnlock(nwfilters->objs[i]);
> - }
> + virUUIDFormat(def->uuid, uuidstr);
> + virObjectRef(obj);
> + virObjectRWUnlock(obj);
> + virObjectRWLockWrite(nwfilters);
> + virObjectRWLockWrite(obj);
> + virHashRemoveEntry(nwfilters->objs, uuidstr);
Unref here after successful removal from hash bucket?
> + virHashRemoveEntry(nwfilters->objsName, def->name);
Again unref here after successful removal ?
> + virObjectRWUnlock(obj);
> + virObjectUnref(obj);
> + virObjectRWUnlock(nwfilters);
> }
>
>
> /**
> - * virNWFilterObjListFindByUUID
> + * virNWFilterObjListFindByUUID[Locked]
> * @nwfilters: Pointer to filter list
> - * @uuid: UUID to use to lookup the object
> + * @uuidstr: UUID to use to lookup the object
> + *
> + * The static [Locked] version would only be used when the Object List is
> + * already locked, such as is the case during virNWFilterObjListAssignDef.
> + * The caller is thus responsible for locking the object.
> *
> * Search for the object by uuidstr in the hash table and return a read
> * locked copy of the object.
> *
> + * Returns: A reffed object or NULL on error
> + */
> +static virNWFilterObjPtr
> +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters,
> + const char *uuidstr)
> +{
> + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr));
> +}
> +
> +
> +/*
> * Returns: A reffed and read locked object or NULL on error
> */
> virNWFilterObjPtr
> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
> - const unsigned char *uuid)
> + const char *uuidstr)
> {
> - size_t i;
> virNWFilterObjPtr obj;
> - virNWFilterDefPtr def;
>
> - for (i = 0; i < nwfilters->count; i++) {
> - obj = nwfilters->objs[i];
> + virObjectRWLockRead(nwfilters);
> + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuidstr);
> + virObjectRWUnlock(nwfilters);
> + if (obj)
> virObjectRWLockRead(obj);
> - def = obj->def;
> - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
> - return virObjectRef(obj);
> - virObjectRWUnlock(obj);
> - }
>
> - return NULL;
> + return obj;
> }
>
>
> /**
> - * virNWFilterObjListFindByName
> + * virNWFilterObjListFindByName[Locked]
> * @nwfilters: Pointer to filter list
> * @name: filter name to use to lookup the object
> *
> + * The static [Locked] version would only be used when the Object List is
> + * already locked, such as is the case during virNWFilterObjListAssignDef.
> + * The caller is thus responsible for locking the object.
> + *
> * Search for the object by name in the hash table and return a read
> * locked copy of the object.
> *
> + * Returns: A reffed object or NULL on error
> + */
> +static virNWFilterObjPtr
> +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters,
> + const char *name)
> +{
> + return virObjectRef(virHashLookup(nwfilters->objsName, name));
> +}
> +
> +
> +/*
> * Returns: A reffed and read locked object or NULL on error
> */
> 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)
> virObjectRWLockRead(obj);
> - def = obj->def;
> - if (STREQ_NULLABLE(def->name, name))
> - return virObjectRef(obj);
> - virObjectRWUnlock(obj);
> - }
>
> - return NULL;
> + return obj;
> }
>
>
> @@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> {
> virNWFilterObjPtr obj;
> virNWFilterDefPtr objdef;
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
> + virUUIDFormat(def->uuid, uuidstr);
> +
> + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) {
> objdef = obj->def;
>
> if (STRNEQ(def->name, objdef->name)) {
> @@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> virNWFilterObjEndAPI(&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,
> _("filter '%s' already exists with uuid %s"),
> def->name, uuidstr);
> @@ -424,11 +482,13 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> return NULL;
> }
>
> -
> - /* Get a READ lock and immediately promote to WRITE while we adjust
> - * data within. */
> - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
> - virNWFilterObjPromoteToWrite(obj);
> + /* We're about to make some changes to objects on the list - so get
> + * the list READ lock in order to Find the object and WRITE lock it
> + * while we adjust data within. */
> + virObjectRWLockRead(nwfilters);
> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters, def->name))) {
> + virObjectRWUnlock(nwfilters);
> + virObjectRWLockWrite(obj);
>
> objdef = obj->def;
> if (virNWFilterDefEqual(def, objdef)) {
I think you should have left the above PromoteToWrite(obj) rather than
doing a virObjectRWLockWrite(obj) now and would then adapt the code here
as mentioned in review of 2/4.
> @@ -458,37 +518,112 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
> return obj;
> }
>
> + /* Promote the nwfilters to add a new object */
> + virObjectRWUnlock(nwfilters);
> + virObjectRWLockWrite(nwfilters);
> if (!(obj = virNWFilterObjNew()))
> - return NULL;
> + goto cleanup;
>
> - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
> - nwfilters->count, obj) < 0) {
> - virNWFilterObjEndAPI(&obj);
> - return NULL;
> + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0)
> + goto error;
> + virObjectRef(obj);
good, here you take a reference because obj ended in the hash bucket.
Therefore, further above, you have to unref when taking it out of the
hash bucket.
> +
> + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) {
> + virHashRemoveEntry(nwfilters->objs, uuidstr);
> + goto error;
> }
> + virObjectRef(obj);
> +
Same comment here. Looks also better to me since taking the reference is
done 'closer' to virHashAddEntry() call.
> obj->def = def;
>
> - return virObjectRef(obj);
> + cleanup:
> + virObjectRWUnlock(nwfilters);
> + return obj;
> +
> + error:
> + virObjectRWUnlock(obj);
> + virObjectUnref(obj);
> + virObjectRWUnlock(nwfilters);
> + return NULL;
> }
>
>
> +struct virNWFilterCountData {
> + virConnectPtr conn;
> + virNWFilterObjListFilter filter;
> + int nelems;
> +};
> +
> +static int
> +virNWFilterObjListNumOfNWFiltersCallback(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virNWFilterObjPtr obj = payload;
> + struct virNWFilterCountData *data = opaque;
> +
> + virObjectRWLockRead(obj);
> + if (!data->filter || data->filter(data->conn, obj->def))
> + data->nelems++;
> + virObjectRWUnlock(obj);
> + return 0;
> +}
> +
> int
> virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
> virConnectPtr conn,
> virNWFilterObjListFilter filter)
> {
> - size_t i;
> - int nfilters = 0;
> + struct virNWFilterCountData data = { .conn = conn,
> + .filter = filter, .nelems = 0 };
style: one of these per line ?
>
> - for (i = 0; i < nwfilters->count; i++) {
> - virNWFilterObjPtr obj = nwfilters->objs[i];
> - virObjectRWLockRead(obj);
> - if (!filter || filter(conn, obj->def))
> - nfilters++;
> - virObjectRWUnlock(obj);
> + virObjectRWLockRead(nwfilters);
> + virHashForEach(nwfilters->objs, virNWFilterObjListNumOfNWFiltersCallback,
> + &data);
> + virObjectRWUnlock(nwfilters);
> +
> + return data.nelems;
> +}
> +
> +
> +struct virNWFilterListData {
> + virConnectPtr conn;
> + virNWFilterObjListFilter filter;
> + int nelems;
> + char **elems;
> + int maxelems;
> + bool error;
> +};
> +
> +static int
> +virNWFilterObjListGetNamesCallback(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virNWFilterObjPtr obj = payload;
> + virNWFilterDefPtr def;
> + struct virNWFilterListData *data = opaque;
> +
> + if (data->error)
> + return 0;
> +
> + if (data->maxelems >= 0 && data->nelems == data->maxelems)
> + return 0;
> +
> + virObjectRWLockRead(obj);
> + def = obj->def;
> +
> + if (!data->filter || data->filter(data->conn, def)) {
> + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) {
> + data->error = true;
> + goto cleanup;
> + }
> + data->nelems++;
> }
>
> - return nfilters;
> + cleanup:
> + virObjectRWUnlock(obj);
> + return 0;
> }
>
>
> @@ -499,82 +634,103 @@ virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters,
> char **const names,
> int maxnames)
> {
> - int nnames = 0;
> - size_t i;
> - virNWFilterDefPtr def;
> + struct virNWFilterListData data = { .conn = conn, .filter = filter,
> + .nelems = 0, .elems = names, .maxelems = maxnames, .error = false };
>
> - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
> - virNWFilterObjPtr obj = nwfilters->objs[i];
> - virObjectRWLockRead(obj);
> - def = obj->def;
> - if (!filter || filter(conn, def)) {
> - if (VIR_STRDUP(names[nnames], def->name) < 0) {
> - virObjectRWUnlock(obj);
> - goto failure;
> - }
> - nnames++;
> - }
> - virObjectRWUnlock(obj);
> - }
> + virObjectRWLockRead(nwfilters);
> + virHashForEach(nwfilters->objs, virNWFilterObjListGetNamesCallback, &data);
> + virObjectRWUnlock(nwfilters);
>
> - return nnames;
> + if (data.error)
> + goto error;
>
> - failure:
> - while (--nnames >= 0)
> - VIR_FREE(names[nnames]);
> + return data.nelems;
>
> + error:
> + while (--data.nelems >= 0)
> + VIR_FREE(data.elems[data.nelems]);
> return -1;
> }
>
>
> +struct virNWFilterExportData {
> + virConnectPtr conn;
> + virNWFilterObjListFilter filter;
> + virNWFilterPtr *filters;
> + int nfilters;
> + bool error;
> +};
> +
> +static int
> +virNWFilterObjListExportCallback(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virNWFilterObjPtr obj = payload;
> + virNWFilterDefPtr def;
> +
nit: indentation error
> + struct virNWFilterExportData *data = opaque;
> + virNWFilterPtr nwfilter;
> +
> + if (data->error)
> + return 0;
> +
> + virObjectRWLockRead(obj);
> + def = obj->def;
> +
> + if (data->filter && !data->filter(data->conn, def))
> + goto cleanup;
> +
> + if (!data->filters) {
> + data->nfilters++;
> + goto cleanup;
> + }
> +
> + if (!(nwfilter = virGetNWFilter(data->conn, def->name, def->uuid))) {
> + data->error = true;
> + goto cleanup;
> + }
> + data->filters[data->nfilters++] = nwfilter;
> +
> + cleanup:
> + virObjectRWUnlock(obj);
> + return 0;
> +}
> +
> +
> int
> virNWFilterObjListExport(virConnectPtr conn,
> virNWFilterObjListPtr nwfilters,
> 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 virNWFilterExportData data = { .conn = conn, .filter = filter,
> + .filters = NULL, .nfilters = 0, .error = false };
>
> - if (!filters) {
> - ret = nwfilters->count;
> - goto cleanup;
> + virObjectRWLockRead(nwfilters);
> + if (filters &&
> + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) < 0) {
> + virObjectRWUnlock(nwfilters);
> + return -1;
> }
>
> - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0)
> - goto cleanup;
> + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback, &data);
> + virObjectRWUnlock(nwfilters);
>
> - for (i = 0; i < nwfilters->count; i++) {
> - obj = nwfilters->objs[i];
> - virObjectRWLockRead(obj);
> - def = obj->def;
> - if (!filter || filter(conn, def)) {
> - if (!(nwfilter = virGetNWFilter(conn, def->name, def->uuid))) {
> - virObjectRWUnlock(obj);
> - goto cleanup;
> - }
> - tmp_filters[nfilters++] = nwfilter;
> - }
> - virObjectRWUnlock(obj);
> + if (data.error)
> + goto cleanup;
> +
> + if (data.filters) {
> + /* trim the array to the final size */
> + ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1));
can we ignore errors here and not return -1 in case an error occurs ?
> + *filters = data.filters;
> }
>
> - *filters = tmp_filters;
> - tmp_filters = NULL;
> - ret = nfilters;
> + return data.nfilters;
>
> cleanup:
> - if (tmp_filters) {
> - for (i = 0; i < nfilters; i ++)
> - virObjectUnref(tmp_filters[i]);
> - }
> - VIR_FREE(tmp_filters);
> -
> - return ret;
> + virObjectListFree(data.filters);
> + return -1;
> }
>
>
> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
> index 0281bc5f5..cabb42a71 100644
> --- a/src/conf/virnwfilterobj.h
> +++ b/src/conf/virnwfilterobj.h
> @@ -65,7 +65,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
>
> virNWFilterObjPtr
> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
> - const unsigned char *uuid);
> + const char *uuidstr);
>
> virNWFilterObjPtr
> virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index b38740b15..b24f59379 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -369,11 +369,10 @@ nwfilterObjFromNWFilter(const unsigned char *uuid)
> virNWFilterObjPtr obj;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
>
> - if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuid))) {
> - virUUIDFormat(uuid, uuidstr);
> + virUUIDFormat(uuid, uuidstr);
> + if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters, uuidstr)))
> virReportError(VIR_ERR_NO_NWFILTER,
> _("no nwfilter with matching uuid '%s'"), uuidstr);
> - }
> return obj;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 01/31/2018 10:10 PM, Stefan Berger wrote:
> On 12/08/2017 09:01 AM, John Ferlan wrote:
>> Implement the self locking object list for nwfilter object lists
>> that uses two hash tables to store the nwfilter object by UUID or
>> by Name.
>>
>> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
>> to expect an already formatted uuidstr.
>>
>> Alter the existing list traversal code to implement the hash table
>> find/lookup/search functionality.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>> src/conf/virnwfilterobj.c | 402
>> ++++++++++++++++++++++++++++-------------
>> src/conf/virnwfilterobj.h | 2 +-
>> src/nwfilter/nwfilter_driver.c | 5 +-
>> 3 files changed, 282 insertions(+), 127 deletions(-)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 6b4758656..a4e6a03d2 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -43,12 +43,21 @@ struct _virNWFilterObj {
>> };
>>
>> 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 virClassPtr virNWFilterObjClass;
>> +static virClassPtr virNWFilterObjListClass;
>> static void virNWFilterObjDispose(void *opaque);
>> +static void virNWFilterObjListDispose(void *opaque);
>>
>>
>> static int
>> @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void)
>> virNWFilterObjDispose)))
>> return -1;
>>
>> + if (!(virNWFilterObjListClass =
>> virClassNew(virClassForObjectRWLockable(),
>> + "virNWFilterObjList",
>> +
>> sizeof(virNWFilterObjList),
>> +
>> virNWFilterObjListDispose)))
>> + return -1;
>> +
>> return 0;
>> }
>>
>> @@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque)
>> }
>>
>>
>> +static void
>> +virNWFilterObjListDispose(void *opaque)
>> +{
>> + virNWFilterObjListPtr nwfilters = opaque;
>> +
>> + virHashFree(nwfilters->objs);
>> + virHashFree(nwfilters->objsName);
>> +}
>> +
>> +
>> void
>> virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
>> {
>> - size_t i;
>> - for (i = 0; i < nwfilters->count; i++)
>> - virObjectUnref(nwfilters->objs[i]);
>> - VIR_FREE(nwfilters->objs);
>> - VIR_FREE(nwfilters);
>> + virObjectUnref(nwfilters);
>> }
>>
>>
>> @@ -160,8 +181,23 @@ 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))) {
[1] As part of the magic of hash tables, when an element is removed from
the hash table the virObjectFreeHashData is called which does a
virObjectUnref...
>> + virObjectUnref(nwfilters);
>> + return NULL;
>> + }
>> +
>> + if (!(nwfilters->objsName = virHashCreate(10,
>> virObjectFreeHashData))) {
>> + virHashFree(nwfilters->objs);
>> + virObjectUnref(nwfilters);
>> return NULL;
>> + }
>> +
>> return nwfilters;
>> }
>>
>> @@ -170,83 +206,105 @@ void
>> virNWFilterObjListRemove(virNWFilterObjListPtr nwfilters,
>> virNWFilterObjPtr obj)
>> {
>> - size_t i;
>> -
>> - virObjectRWUnlock(obj);
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virNWFilterDefPtr def;
>>
>> - for (i = 0; i < nwfilters->count; i++) {
>> - virObjectRWLockWrite(nwfilters->objs[i]);
>> - if (nwfilters->objs[i] == obj) {
>> - virObjectRWUnlock(nwfilters->objs[i]);
>> - virObjectUnref(nwfilters->objs[i]);
>> + if (!obj)
>> + return;
>> + def = obj->def;
>>
>> - VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count);
>> - break;
>> - }
>> - virObjectRWUnlock(nwfilters->objs[i]);
>> - }
>> + virUUIDFormat(def->uuid, uuidstr);
>> + virObjectRef(obj);
>> + virObjectRWUnlock(obj);
>> + virObjectRWLockWrite(nwfilters);
>> + virObjectRWLockWrite(obj);
>> + virHashRemoveEntry(nwfilters->objs, uuidstr);
> Unref here after successful removal from hash bucket?
>> + virHashRemoveEntry(nwfilters->objsName, def->name);
> Again unref here after successful removal ?
[1] So when we RemoveEntry, the data->tableFree (from our Create) is
called. So, yes, that Unref happens automagically. Once the last ref is
removed the object's disposal API (virNWFilterObjDispose) is called
before being completely reaped.
>> + virObjectRWUnlock(obj);
>> + virObjectUnref(obj);
>> + virObjectRWUnlock(nwfilters);
>> }
>>
>>
>> /**
>> - * virNWFilterObjListFindByUUID
>> + * virNWFilterObjListFindByUUID[Locked]
>> * @nwfilters: Pointer to filter list
>> - * @uuid: UUID to use to lookup the object
>> + * @uuidstr: UUID to use to lookup the object
>> + *
>> + * The static [Locked] version would only be used when the Object
>> List is
>> + * already locked, such as is the case during
>> virNWFilterObjListAssignDef.
>> + * The caller is thus responsible for locking the object.
>> *
>> * Search for the object by uuidstr in the hash table and return a read
>> * locked copy of the object.
>> *
>> + * Returns: A reffed object or NULL on error
>> + */
>> +static virNWFilterObjPtr
>> +virNWFilterObjListFindByUUIDLocked(virNWFilterObjListPtr nwfilters,
>> + const char *uuidstr)
>> +{
>> + return virObjectRef(virHashLookup(nwfilters->objs, uuidstr));
>> +}
>> +
>> +
>> +/*
>> * Returns: A reffed and read locked object or NULL on error
>> */
>> virNWFilterObjPtr
>> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
>> - const unsigned char *uuid)
>> + const char *uuidstr)
>> {
>> - size_t i;
>> virNWFilterObjPtr obj;
>> - virNWFilterDefPtr def;
>>
>> - for (i = 0; i < nwfilters->count; i++) {
>> - obj = nwfilters->objs[i];
>> + virObjectRWLockRead(nwfilters);
>> + obj = virNWFilterObjListFindByUUIDLocked(nwfilters, uuidstr);
>> + virObjectRWUnlock(nwfilters);
>> + if (obj)
>> virObjectRWLockRead(obj);
>> - def = obj->def;
>> - if (!memcmp(def->uuid, uuid, VIR_UUID_BUFLEN))
>> - return virObjectRef(obj);
>> - virObjectRWUnlock(obj);
>> - }
>>
>> - return NULL;
>> + return obj;
>> }
>>
>>
>> /**
>> - * virNWFilterObjListFindByName
>> + * virNWFilterObjListFindByName[Locked]
>> * @nwfilters: Pointer to filter list
>> * @name: filter name to use to lookup the object
>> *
>> + * The static [Locked] version would only be used when the Object
>> List is
>> + * already locked, such as is the case during
>> virNWFilterObjListAssignDef.
>> + * The caller is thus responsible for locking the object.
>> + *
>> * Search for the object by name in the hash table and return a read
>> * locked copy of the object.
>> *
>> + * Returns: A reffed object or NULL on error
>> + */
>> +static virNWFilterObjPtr
>> +virNWFilterObjListFindByNameLocked(virNWFilterObjListPtr nwfilters,
>> + const char *name)
>> +{
>> + return virObjectRef(virHashLookup(nwfilters->objsName, name));
>> +}
>> +
>> +
>> +/*
>> * Returns: A reffed and read locked object or NULL on error
>> */
>> 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)
>> virObjectRWLockRead(obj);
>> - def = obj->def;
>> - if (STREQ_NULLABLE(def->name, name))
>> - return virObjectRef(obj);
>> - virObjectRWUnlock(obj);
>> - }
>>
>> - return NULL;
>> + return obj;
>> }
>>
>>
>> @@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>> nwfilters,
>> {
>> virNWFilterObjPtr obj;
>> virNWFilterDefPtr objdef;
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>>
>> - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
>> + virUUIDFormat(def->uuid, uuidstr);
>> +
>> + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) {
>> objdef = obj->def;
>>
>> if (STRNEQ(def->name, objdef->name)) {
>> @@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>> nwfilters,
>> virNWFilterObjEndAPI(&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,
>> _("filter '%s' already exists with uuid
>> %s"),
>> def->name, uuidstr);
>> @@ -424,11 +482,13 @@
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>> return NULL;
>> }
>>
>> -
>> - /* Get a READ lock and immediately promote to WRITE while we adjust
>> - * data within. */
>> - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>> - virNWFilterObjPromoteToWrite(obj);
>> + /* We're about to make some changes to objects on the list - so get
>> + * the list READ lock in order to Find the object and WRITE lock it
>> + * while we adjust data within. */
>> + virObjectRWLockRead(nwfilters);
>> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
>> def->name))) {
>> + virObjectRWUnlock(nwfilters);
>> + virObjectRWLockWrite(obj);
>>
>> objdef = obj->def;
>> if (virNWFilterDefEqual(def, objdef)) {
>
> I think you should have left the above PromoteToWrite(obj) rather than
> doing a virObjectRWLockWrite(obj) now and would then adapt the code here
> as mentioned in review of 2/4.
>
Hmm... true... I think that's because originally I had done the list
patch before the object patch... So it's probably one of those merge
things...
Good catch.
>> @@ -458,37 +518,112 @@
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>> return obj;
>> }
>>
>> + /* Promote the nwfilters to add a new object */
>> + virObjectRWUnlock(nwfilters);
>> + virObjectRWLockWrite(nwfilters);
>> if (!(obj = virNWFilterObjNew()))
>> - return NULL;
>> + goto cleanup;
>>
>> - if (VIR_APPEND_ELEMENT_COPY(nwfilters->objs,
>> - nwfilters->count, obj) < 0) {
>> - virNWFilterObjEndAPI(&obj);
>> - return NULL;
>> + if (virHashAddEntry(nwfilters->objs, uuidstr, obj) < 0)
>> + goto error;
>> + virObjectRef(obj);
>
> good, here you take a reference because obj ended in the hash bucket.
> Therefore, further above, you have to unref when taking it out of the
> hash bucket.
>
>> +
>> + if (virHashAddEntry(nwfilters->objsName, def->name, obj) < 0) {
>> + virHashRemoveEntry(nwfilters->objs, uuidstr);
>> + goto error;
>> }
>> + virObjectRef(obj);
>> +
> Same comment here. Looks also better to me since taking the reference is
> done 'closer' to virHashAddEntry() call.
>
Right. That was the "end goal"...
>> obj->def = def;
>>
>> - return virObjectRef(obj);
>> + cleanup:
>> + virObjectRWUnlock(nwfilters);
>> + return obj;
>> +
>> + error:
>> + virObjectRWUnlock(obj);
>> + virObjectUnref(obj);
>> + virObjectRWUnlock(nwfilters);
>> + return NULL;
>> }
>>
>>
>> +struct virNWFilterCountData {
>> + virConnectPtr conn;
>> + virNWFilterObjListFilter filter;
>> + int nelems;
>> +};
>> +
>> +static int
>> +virNWFilterObjListNumOfNWFiltersCallback(void *payload,
>> + const void *name
>> ATTRIBUTE_UNUSED,
>> + void *opaque)
>> +{
>> + virNWFilterObjPtr obj = payload;
>> + struct virNWFilterCountData *data = opaque;
>> +
>> + virObjectRWLockRead(obj);
>> + if (!data->filter || data->filter(data->conn, obj->def))
>> + data->nelems++;
>> + virObjectRWUnlock(obj);
>> + return 0;
>> +}
>> +
>> int
>> virNWFilterObjListNumOfNWFilters(virNWFilterObjListPtr nwfilters,
>> virConnectPtr conn,
>> virNWFilterObjListFilter filter)
>> {
>> - size_t i;
>> - int nfilters = 0;
>> + struct virNWFilterCountData data = { .conn = conn,
>> + .filter = filter, .nelems = 0 };
>
> style: one of these per line ?
>
I've never seen a consistent style for these things, but I can move the
.conn to the same line as .filter which is done in other initializers
where there is only one line.
>>
>> - for (i = 0; i < nwfilters->count; i++) {
>> - virNWFilterObjPtr obj = nwfilters->objs[i];
>> - virObjectRWLockRead(obj);
>> - if (!filter || filter(conn, obj->def))
>> - nfilters++;
>> - virObjectRWUnlock(obj);
>> + virObjectRWLockRead(nwfilters);
>> + virHashForEach(nwfilters->objs,
>> virNWFilterObjListNumOfNWFiltersCallback,
>> + &data);
>> + virObjectRWUnlock(nwfilters);
>> +
>> + return data.nelems;
>> +}
>> +
>> +
>> +struct virNWFilterListData {
>> + virConnectPtr conn;
>> + virNWFilterObjListFilter filter;
>> + int nelems;
>> + char **elems;
>> + int maxelems;
>> + bool error;
>> +};
>> +
>> +static int
>> +virNWFilterObjListGetNamesCallback(void *payload,
>> + const void *name ATTRIBUTE_UNUSED,
>> + void *opaque)
>> +{
>> + virNWFilterObjPtr obj = payload;
>> + virNWFilterDefPtr def;
>> + struct virNWFilterListData *data = opaque;
>> +
>> + if (data->error)
>> + return 0;
>> +
>> + if (data->maxelems >= 0 && data->nelems == data->maxelems)
>> + return 0;
>> +
>> + virObjectRWLockRead(obj);
>> + def = obj->def;
>> +
>> + if (!data->filter || data->filter(data->conn, def)) {
>> + if (VIR_STRDUP(data->elems[data->nelems], def->name) < 0) {
>> + data->error = true;
>> + goto cleanup;
>> + }
>> + data->nelems++;
>> }
>>
>> - return nfilters;
>> + cleanup:
>> + virObjectRWUnlock(obj);
>> + return 0;
>> }
>>
>>
>> @@ -499,82 +634,103 @@
>> virNWFilterObjListGetNames(virNWFilterObjListPtr nwfilters,
>> char **const names,
>> int maxnames)
>> {
>> - int nnames = 0;
>> - size_t i;
>> - virNWFilterDefPtr def;
>> + struct virNWFilterListData data = { .conn = conn, .filter = filter,
>> + .nelems = 0, .elems = names, .maxelems = maxnames, .error =
>> false };
>>
>> - for (i = 0; i < nwfilters->count && nnames < maxnames; i++) {
>> - virNWFilterObjPtr obj = nwfilters->objs[i];
>> - virObjectRWLockRead(obj);
>> - def = obj->def;
>> - if (!filter || filter(conn, def)) {
>> - if (VIR_STRDUP(names[nnames], def->name) < 0) {
>> - virObjectRWUnlock(obj);
>> - goto failure;
>> - }
>> - nnames++;
>> - }
>> - virObjectRWUnlock(obj);
>> - }
>> + virObjectRWLockRead(nwfilters);
>> + virHashForEach(nwfilters->objs,
>> virNWFilterObjListGetNamesCallback, &data);
>> + virObjectRWUnlock(nwfilters);
>>
>> - return nnames;
>> + if (data.error)
>> + goto error;
>>
>> - failure:
>> - while (--nnames >= 0)
>> - VIR_FREE(names[nnames]);
>> + return data.nelems;
>>
>> + error:
>> + while (--data.nelems >= 0)
>> + VIR_FREE(data.elems[data.nelems]);
>> return -1;
>> }
>>
>>
>> +struct virNWFilterExportData {
>> + virConnectPtr conn;
>> + virNWFilterObjListFilter filter;
>> + virNWFilterPtr *filters;
>> + int nfilters;
>> + bool error;
>> +};
>> +
>> +static int
>> +virNWFilterObjListExportCallback(void *payload,
>> + const void *name ATTRIBUTE_UNUSED,
>> + void *opaque)
>> +{
>> + virNWFilterObjPtr obj = payload;
>> + virNWFilterDefPtr def;
>> +
> nit: indentation error
>
Thanks!
>> + struct virNWFilterExportData *data = opaque;
>> + virNWFilterPtr nwfilter;
>> +
>> + if (data->error)
>> + return 0;
>> +
>> + virObjectRWLockRead(obj);
>> + def = obj->def;
>> +
>> + if (data->filter && !data->filter(data->conn, def))
>> + goto cleanup;
>> +
>> + if (!data->filters) {
>> + data->nfilters++;
>> + goto cleanup;
>> + }
>> +
>> + if (!(nwfilter = virGetNWFilter(data->conn, def->name,
>> def->uuid))) {
>> + data->error = true;
>> + goto cleanup;
>> + }
>> + data->filters[data->nfilters++] = nwfilter;
>> +
>> + cleanup:
>> + virObjectRWUnlock(obj);
>> + return 0;
>> +}
>> +
>> +
>> int
>> virNWFilterObjListExport(virConnectPtr conn,
>> virNWFilterObjListPtr nwfilters,
>> 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 virNWFilterExportData data = { .conn = conn, .filter =
>> filter,
>> + .filters = NULL, .nfilters = 0, .error = false };
>>
>> - if (!filters) {
>> - ret = nwfilters->count;
>> - goto cleanup;
>> + virObjectRWLockRead(nwfilters);
>> + if (filters &&
>> + VIR_ALLOC_N(data.filters, virHashSize(nwfilters->objs) + 1) <
>> 0) {
>> + virObjectRWUnlock(nwfilters);
>> + return -1;
>> }
>>
>> - if (VIR_ALLOC_N(tmp_filters, nwfilters->count + 1) < 0)
>> - goto cleanup;
>> + virHashForEach(nwfilters->objs, virNWFilterObjListExportCallback,
>> &data);
>> + virObjectRWUnlock(nwfilters);
>>
>> - for (i = 0; i < nwfilters->count; i++) {
>> - obj = nwfilters->objs[i];
>> - virObjectRWLockRead(obj);
>> - def = obj->def;
>> - if (!filter || filter(conn, def)) {
>> - if (!(nwfilter = virGetNWFilter(conn, def->name,
>> def->uuid))) {
>> - virObjectRWUnlock(obj);
>> - goto cleanup;
>> - }
>> - tmp_filters[nfilters++] = nwfilter;
>> - }
>> - virObjectRWUnlock(obj);
>> + if (data.error)
>> + goto cleanup;
>> +
>> + if (data.filters) {
>> + /* trim the array to the final size */
>> + ignore_value(VIR_REALLOC_N(data.filters, data.nfilters + 1));
>
> can we ignore errors here and not return -1 in case an error occurs ?
>
Yes, the error would be we cannot shrink... This is common between
vir.*ObjList.*Export.* functions
Thanks again for the detailed review. After I push patch 1, I'll repost
the remaining ones. Maybe Laine will also have some luck with his
nwfilter testing. He's pursuing either a locking problem or a *severe*
performance problem with the libvirt-tck test.
John
>> + *filters = data.filters;
>> }
>>
>> - *filters = tmp_filters;
>> - tmp_filters = NULL;
>> - ret = nfilters;
>> + return data.nfilters;
>>
>> cleanup:
>> - if (tmp_filters) {
>> - for (i = 0; i < nfilters; i ++)
>> - virObjectUnref(tmp_filters[i]);
>> - }
>> - VIR_FREE(tmp_filters);
>> -
>> - return ret;
>> + virObjectListFree(data.filters);
>> + return -1;
>> }
>>
>>
>> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
>> index 0281bc5f5..cabb42a71 100644
>> --- a/src/conf/virnwfilterobj.h
>> +++ b/src/conf/virnwfilterobj.h
>> @@ -65,7 +65,7 @@ virNWFilterObjListRemove(virNWFilterObjListPtr
>> nwfilters,
>>
>> virNWFilterObjPtr
>> virNWFilterObjListFindByUUID(virNWFilterObjListPtr nwfilters,
>> - const unsigned char *uuid);
>> + const char *uuidstr);
>>
>> virNWFilterObjPtr
>> virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>> diff --git a/src/nwfilter/nwfilter_driver.c
>> b/src/nwfilter/nwfilter_driver.c
>> index b38740b15..b24f59379 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -369,11 +369,10 @@ nwfilterObjFromNWFilter(const unsigned char *uuid)
>> virNWFilterObjPtr obj;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>>
>> - if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters,
>> uuid))) {
>> - virUUIDFormat(uuid, uuidstr);
>> + virUUIDFormat(uuid, uuidstr);
>> + if (!(obj = virNWFilterObjListFindByUUID(driver->nwfilters,
>> uuidstr)))
>> virReportError(VIR_ERR_NO_NWFILTER,
>> _("no nwfilter with matching uuid '%s'"),
>> uuidstr);
>> - }
>> return obj;
>> }
>>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 02/02/2018 02:33 PM, John Ferlan wrote:
>
> On 01/31/2018 10:10 PM, Stefan Berger wrote:
>> On 12/08/2017 09:01 AM, John Ferlan wrote:
>>> Implement the self locking object list for nwfilter object lists
>>> that uses two hash tables to store the nwfilter object by UUID or
>>> by Name.
>>>
>>> As part of this alter the uuid argument to virNWFilterObjLookupByUUID
>>> to expect an already formatted uuidstr.
>>>
>>> Alter the existing list traversal code to implement the hash table
>>> find/lookup/search functionality.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>> src/conf/virnwfilterobj.c | 402
>>> ++++++++++++++++++++++++++++-------------
>>> src/conf/virnwfilterobj.h | 2 +-
>>> src/nwfilter/nwfilter_driver.c | 5 +-
>>> 3 files changed, 282 insertions(+), 127 deletions(-)
>>>
>>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>>> index 6b4758656..a4e6a03d2 100644
>>> --- a/src/conf/virnwfilterobj.c
>>> +++ b/src/conf/virnwfilterobj.c
>>> @@ -43,12 +43,21 @@ struct _virNWFilterObj {
>>> };
>>>
>>> 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 virClassPtr virNWFilterObjClass;
>>> +static virClassPtr virNWFilterObjListClass;
>>> static void virNWFilterObjDispose(void *opaque);
>>> +static void virNWFilterObjListDispose(void *opaque);
>>>
>>>
>>> static int
>>> @@ -60,6 +69,12 @@ virNWFilterObjOnceInit(void)
>>> virNWFilterObjDispose)))
>>> return -1;
>>>
>>> + if (!(virNWFilterObjListClass =
>>> virClassNew(virClassForObjectRWLockable(),
>>> + "virNWFilterObjList",
>>> +
>>> sizeof(virNWFilterObjList),
>>> +
>>> virNWFilterObjListDispose)))
>>> + return -1;
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -144,14 +159,20 @@ virNWFilterObjDispose(void *opaque)
>>> }
>>>
>>>
>>> +static void
>>> +virNWFilterObjListDispose(void *opaque)
>>> +{
>>> + virNWFilterObjListPtr nwfilters = opaque;
>>> +
>>> + virHashFree(nwfilters->objs);
>>> + virHashFree(nwfilters->objsName);
>>> +}
>>> +
>>> +
>>> void
>>> virNWFilterObjListFree(virNWFilterObjListPtr nwfilters)
>>> {
>>> - size_t i;
>>> - for (i = 0; i < nwfilters->count; i++)
>>> - virObjectUnref(nwfilters->objs[i]);
>>> - VIR_FREE(nwfilters->objs);
>>> - VIR_FREE(nwfilters);
>>> + virObjectUnref(nwfilters);
>>> }
>>>
>>>
>>> @@ -160,8 +181,23 @@ 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))) {
> [1] As part of the magic of hash tables, when an element is removed from
> the hash table the virObjectFreeHashData is called which does a
> virObjectUnref...
Ah. I was looking for 'symmetry' ... so ideally we would have automatic
increase of the refcount as well.
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
>>>
>>> @@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>>> nwfilters,
>>> {
>>> virNWFilterObjPtr obj;
>>> virNWFilterDefPtr objdef;
>>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>
>>> - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
>>> + virUUIDFormat(def->uuid, uuidstr);
>>> +
>>> + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) {
>>> objdef = obj->def;
>>>
>>> if (STRNEQ(def->name, objdef->name)) {
>>> @@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>>> nwfilters,
>>> virNWFilterObjEndAPI(&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,
>>> _("filter '%s' already exists with uuid
>>> %s"),
>>> def->name, uuidstr);
>>> @@ -424,11 +482,13 @@
>>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>> return NULL;
>>> }
>>>
>>> -
>>> - /* Get a READ lock and immediately promote to WRITE while we adjust
>>> - * data within. */
>>> - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>> - virNWFilterObjPromoteToWrite(obj);
>>> + /* We're about to make some changes to objects on the list - so get
>>> + * the list READ lock in order to Find the object and WRITE lock it
>>> + * while we adjust data within. */
>>> + virObjectRWLockRead(nwfilters);
>>> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
>>> def->name))) {
>>> + virObjectRWUnlock(nwfilters);
>>> + virObjectRWLockWrite(obj);
>>>
>>> objdef = obj->def;
>>> if (virNWFilterDefEqual(def, objdef)) {
>>
>> I think you should have left the above PromoteToWrite(obj) rather than
>> doing a virObjectRWLockWrite(obj) now and would then adapt the code here
>> as mentioned in review of 2/4.
>>
>
> Hmm... true... I think that's because originally I had done the list
> patch before the object patch... So it's probably one of those merge
> things...
>
> Good catch.
>
Revisiting this particular place after some testing I've just done using
the libvirt-tck test... and lots of debug code ;-)...
When we return from the FindByNameLocked, all we have is a refcnt
incremented object. So, we have to Lock it while we still hold the
nwfilters read lock, thus this particular hunk was "mostly" correct.
... Should have taken the lock while we had nwfilters lock
... Using a Write lock avoids the immediate promote in either path
[...]
I'm going to post a v5 soon with the adjustments. I also added one more
place to Demote in the new series. That'd be in the end of this
AssignDef where we get a new filter to be consistent we should return
with the filter read locked only.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.