Rather than use a forward linked list of elements, it'll be much more
efficient to use a hash table to reference the elements by unique name
and to perform hash searches.

This patch does all the heavy lifting of converting the list object to
use a self locking list that contains the hash table. Each of the FindBy
functions that do not involve finding the object by it's key (name) is
converted to use virHashSearch in order to find the specific object.
When searching for the key (name), it's possible to use virHashLookup.
For any of the list perusal functions that are required to evaluate
each object, the virHashForEach function is used.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virnodedeviceobj.c | 575 ++++++++++++++++++++++++++++++--------------
 1 file changed, 400 insertions(+), 175 deletions(-)

diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
index 035a56d..58481e7 100644
--- a/src/conf/virnodedeviceobj.c
+++ b/src/conf/virnodedeviceobj.c
@@ -25,6 +25,7 @@
 #include "viralloc.h"
 #include "virnodedeviceobj.h"
 #include "virerror.h"
+#include "virhash.h"
 #include "virlog.h"
 #include "virstring.h"
 
@@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
 };
 
 struct _virNodeDeviceObjList {
-    size_t count;
-    virNodeDeviceObjPtr *objs;
+    virObjectLockable parent;
+
+    /* name string -> virNodeDeviceObj  mapping
+     * for O(1), lockless lookup-by-uuid */
+    virHashTable *objs;
+
 };
 
 
 static virClassPtr virNodeDeviceObjClass;
+static virClassPtr virNodeDeviceObjListClass;
 static void virNodeDeviceObjDispose(void *opaque);
+static void virNodeDeviceObjListDispose(void *opaque);
 
 static int
 virNodeDeviceObjOnceInit(void)
@@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
                                               virNodeDeviceObjDispose)))
         return -1;
 
+    if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
+                                                  "virNodeDeviceObjList",
+                                                  sizeof(virNodeDeviceObjList),
+                                                  virNodeDeviceObjListDispose)))
+        return -1;
+
     return 0;
 }
 
@@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
 }
 
 
+static int
+virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
+                                            const void *name ATTRIBUTE_UNUSED,
+                                            const void *opaque)
+{
+    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+    virNodeDeviceDefPtr def;
+    const char *sysfs_path = opaque;
+    int want = 0;
+
+    virObjectLock(obj);
+    def = obj->def;
+    if (STREQ_NULLABLE(def->sysfs_path, sysfs_path))
+        want = 1;
+    virObjectUnlock(obj);
+    return want;
+}
+
+
 virNodeDeviceObjPtr
 virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
                                     const char *sysfs_path)
 {
-    size_t i;
+    virNodeDeviceObjPtr obj;
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virNodeDeviceDefPtr def;
+    virObjectLock(devs);
+    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
+                        (void *)sysfs_path, NULL);
+    virObjectRef(obj);
+    virObjectUnlock(devs);
 
+    if (obj)
         virObjectLock(obj);
-        def = obj->def;
-        if ((def->sysfs_path != NULL) &&
-            (STREQ(def->sysfs_path, sysfs_path))) {
-            return virObjectRef(obj);
-        }
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+static virNodeDeviceObjPtr
+virNodeDeviceObjListFindByNameLocked(virNodeDeviceObjListPtr devs,
+                                     const char *name)
+{
+    return virObjectRef(virHashLookup(devs->objs, name));
 }
 
 
@@ -238,20 +274,42 @@ virNodeDeviceObjPtr
 virNodeDeviceObjListFindByName(virNodeDeviceObjListPtr devs,
                                const char *name)
 {
-    size_t i;
-
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virNodeDeviceDefPtr def;
+    virNodeDeviceObjPtr obj;
 
+    virObjectLock(devs);
+    obj = virNodeDeviceObjListFindByNameLocked(devs, name);
+    virObjectUnlock(devs);
+    if (obj)
         virObjectLock(obj);
-        def = obj->def;
-        if (STREQ(def->name, name))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+struct virNodeDeviceObjListFindByWWNsData {
+    const char *parent_wwnn;
+    const char *parent_wwpn;
+};
+
+static int
+virNodeDeviceObjListFindByWWNsCallback(const void *payload,
+                                       const void *name ATTRIBUTE_UNUSED,
+                                       const void *opaque)
+{
+    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+    struct virNodeDeviceObjListFindByWWNsData *data =
+        (struct virNodeDeviceObjListFindByWWNsData *) opaque;
+    virNodeDevCapsDefPtr cap;
+    int want = 0;
+
+    virObjectLock(obj);
+    if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
+        STREQ_NULLABLE(cap->data.scsi_host.wwnn, data->parent_wwnn) &&
+        STREQ_NULLABLE(cap->data.scsi_host.wwpn, data->parent_wwpn) &&
+        virNodeDeviceFindVPORTCapDef(obj))
+        want = 1;
+    virObjectUnlock(obj);
+    return want;
 }
 
 
@@ -260,22 +318,40 @@ virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs,
                                const char *parent_wwnn,
                                const char *parent_wwpn)
 {
-    size_t i;
+    virNodeDeviceObjPtr obj;
+    struct virNodeDeviceObjListFindByWWNsData data = {
+        .parent_wwnn = parent_wwnn, .parent_wwpn = parent_wwpn };
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virNodeDevCapsDefPtr cap;
+    virObjectLock(devs);
+    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByWWNsCallback,
+                        &data, NULL);
+    virObjectRef(obj);
+    virObjectUnlock(devs);
 
+    if (obj)
         virObjectLock(obj);
-        if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
-            STREQ_NULLABLE(cap->data.scsi_host.wwnn, parent_wwnn) &&
-            STREQ_NULLABLE(cap->data.scsi_host.wwpn, parent_wwpn) &&
-            virNodeDeviceFindVPORTCapDef(obj))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+static int
+virNodeDeviceObjListFindByFabricWWNCallback(const void *payload,
+                                            const void *name ATTRIBUTE_UNUSED,
+                                            const void *opaque)
+{
+    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+    const char *matchstr = opaque;
+    virNodeDevCapsDefPtr cap;
+    int want = 0;
+
+    virObjectLock(obj);
+    if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
+        STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, matchstr) &&
+        virNodeDeviceFindVPORTCapDef(obj))
+        want = 1;
+    virObjectUnlock(obj);
+    return want;
 }
 
 
@@ -283,21 +359,35 @@ static virNodeDeviceObjPtr
 virNodeDeviceObjListFindByFabricWWN(virNodeDeviceObjListPtr devs,
                                     const char *parent_fabric_wwn)
 {
-    size_t i;
+    virNodeDeviceObjPtr obj;
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virNodeDevCapsDefPtr cap;
+    virObjectLock(devs);
+    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByFabricWWNCallback,
+                        (void *)parent_fabric_wwn, NULL);
+    virObjectRef(obj);
+    virObjectUnlock(devs);
 
+    if (obj)
         virObjectLock(obj);
-        if ((cap = virNodeDeviceFindFCCapDef(obj)) &&
-            STREQ_NULLABLE(cap->data.scsi_host.fabric_wwn, parent_fabric_wwn) &&
-            virNodeDeviceFindVPORTCapDef(obj))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+static int
+virNodeDeviceObjListFindByCapCallback(const void *payload,
+                                      const void *name ATTRIBUTE_UNUSED,
+                                      const void *opaque)
+{
+    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+    const char *matchstr = opaque;
+    int want = 0;
+
+    virObjectLock(obj);
+    if (virNodeDeviceObjHasCap(obj, matchstr))
+        want = 1;
+    virObjectUnlock(obj);
+    return want;
 }
 
 
@@ -305,18 +395,59 @@ static virNodeDeviceObjPtr
 virNodeDeviceObjListFindByCap(virNodeDeviceObjListPtr devs,
                               const char *cap)
 {
-    size_t i;
+    virNodeDeviceObjPtr obj;
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
+    virObjectLock(devs);
+    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindByCapCallback,
+                        (void *)cap, NULL);
+    virObjectRef(obj);
+    virObjectUnlock(devs);
 
+    if (obj)
         virObjectLock(obj);
-        if (virNodeDeviceObjHasCap(obj, cap))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+struct virNodeDeviceObjListFindSCSIHostByWWNsData {
+    const char *wwnn;
+    const char *wwpn;
+};
+
+static int
+virNodeDeviceObjListFindSCSIHostByWWNsCallback(const void *payload,
+                                               const void *name ATTRIBUTE_UNUSED,
+                                               const void *opaque)
+{
+    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
+    virNodeDeviceDefPtr def;
+    struct virNodeDeviceObjListFindSCSIHostByWWNsData *data =
+        (struct virNodeDeviceObjListFindSCSIHostByWWNsData *) opaque;
+    virNodeDevCapsDefPtr cap;
+    int want = 0;
+
+    virObjectLock(obj);
+    def = obj->def;
+    cap = def->caps;
+
+    while (cap) {
+        if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
+            virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
+            if (cap->data.scsi_host.flags &
+                VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
+                if (STREQ(cap->data.scsi_host.wwnn, data->wwnn) &&
+                    STREQ(cap->data.scsi_host.wwpn, data->wwpn)) {
+                    want = 1;
+                    break;
+                }
+            }
+        }
+        cap = cap->next;
+     }
+
+    virObjectUnlock(obj);
+    return want;
 }
 
 
@@ -325,31 +456,30 @@ virNodeDeviceObjListFindSCSIHostByWWNs(virNodeDeviceObjListPtr devs,
                                        const char *wwnn,
                                        const char *wwpn)
 {
-    size_t i;
+    virNodeDeviceObjPtr obj;
+    struct virNodeDeviceObjListFindSCSIHostByWWNsData data = {
+        .wwnn = wwnn, .wwpn = wwpn };
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virNodeDevCapsDefPtr cap;
+    virObjectLock(devs);
+    obj = virHashSearch(devs->objs,
+                        virNodeDeviceObjListFindSCSIHostByWWNsCallback,
+                        &data, NULL);
+    virObjectRef(obj);
+    virObjectUnlock(devs);
 
+    if (obj)
         virObjectLock(obj);
-        cap = obj->def->caps;
-
-        while (cap) {
-            if (cap->data.type == VIR_NODE_DEV_CAP_SCSI_HOST) {
-                virNodeDeviceGetSCSIHostCaps(&cap->data.scsi_host);
-                if (cap->data.scsi_host.flags &
-                    VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) {
-                    if (STREQ(cap->data.scsi_host.wwnn, wwnn) &&
-                        STREQ(cap->data.scsi_host.wwpn, wwpn))
-                        return virObjectRef(obj);
-                }
-            }
-            cap = cap->next;
-        }
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
+}
+
+
+static void
+virNodeDeviceObjListDispose(void *obj)
+{
+    virNodeDeviceObjListPtr devs = obj;
+
+    virHashFree(devs->objs);
 }
 
 
@@ -358,8 +488,17 @@ virNodeDeviceObjListNew(void)
 {
     virNodeDeviceObjListPtr devs;
 
-    if (VIR_ALLOC(devs) < 0)
+    if (virNodeDeviceObjInitialize() < 0)
         return NULL;
+
+    if (!(devs = virObjectLockableNew(virNodeDeviceObjListClass)))
+        return NULL;
+
+    if (!(devs->objs = virHashCreate(50, virObjectFreeHashData))) {
+        virObjectUnref(devs);
+        return NULL;
+    }
+
     return devs;
 }
 
@@ -367,11 +506,7 @@ virNodeDeviceObjListNew(void)
 void
 virNodeDeviceObjListFree(virNodeDeviceObjListPtr devs)
 {
-    size_t i;
-    for (i = 0; i < devs->count; i++)
-        virObjectUnref(devs->objs[i]);
-    VIR_FREE(devs->objs);
-    VIR_FREE(devs);
+    virObjectUnref(devs);
 }
 
 
@@ -381,22 +516,28 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs,
 {
     virNodeDeviceObjPtr obj;
 
-    if ((obj = virNodeDeviceObjListFindByName(devs, def->name))) {
+    virObjectLock(devs);
+
+    if ((obj = virNodeDeviceObjListFindByNameLocked(devs, def->name))) {
+        virObjectLock(obj);
         virNodeDeviceDefFree(obj->def);
         obj->def = def;
-        return obj;
-    }
+    } else {
+        if (!(obj = virNodeDeviceObjNew()))
+            goto cleanup;
 
-    if (!(obj = virNodeDeviceObjNew()))
-        return NULL;
+        if (virHashAddEntry(devs->objs, def->name, obj) < 0) {
+            virNodeDeviceObjEndAPI(&obj);
+            goto cleanup;
+        }
 
-    if (VIR_APPEND_ELEMENT_COPY(devs->objs, devs->count, obj) < 0) {
-        virNodeDeviceObjEndAPI(&obj);
-        return NULL;
+        obj->def = def;
+        virObjectRef(obj);
     }
-    obj->def = def;
 
-    return virObjectRef(obj);
+ cleanup:
+    virObjectUnlock(devs);
+    return obj;
 }
 
 
@@ -404,21 +545,20 @@ void
 virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs,
                            virNodeDeviceObjPtr obj)
 {
-    size_t i;
-
-    virObjectUnlock(obj);
+    virNodeDeviceDefPtr def;
 
-    for (i = 0; i < devs->count; i++) {
-        virObjectLock(devs->objs[i]);
-        if (devs->objs[i] == obj) {
-            virObjectUnlock(devs->objs[i]);
-            virObjectUnref(devs->objs[i]);
+    if (!obj)
+        return;
+    def = obj->def;
 
-            VIR_DELETE_ELEMENT(devs->objs, i, devs->count);
-            break;
-        }
-        virObjectUnlock(devs->objs[i]);
-    }
+    virObjectRef(obj);
+    virObjectUnlock(obj);
+    virObjectLock(devs);
+    virObjectLock(obj);
+    virHashRemoveEntry(devs->objs, def->name);
+    virObjectUnlock(obj);
+    virObjectUnref(obj);
+    virObjectUnlock(devs);
 }
 
 
@@ -619,25 +759,89 @@ virNodeDeviceCapMatch(virNodeDeviceObjPtr obj,
 }
 
 
+struct virNodeDeviceCountData {
+    virConnectPtr conn;
+    virNodeDeviceObjListFilter aclfilter;
+    const char *matchstr;
+    int count;
+};
+
+static int
+virNodeDeviceObjListNumOfDevicesCallback(void *payload,
+                                         const void *name ATTRIBUTE_UNUSED,
+                                         void *opaque)
+{
+    virNodeDeviceObjPtr obj = payload;
+    virNodeDeviceDefPtr def;
+    struct virNodeDeviceCountData *data = opaque;
+    virNodeDeviceObjListFilter aclfilter = data->aclfilter;
+
+    virObjectLock(obj);
+    def = obj->def;
+    if ((!aclfilter || aclfilter(data->conn, def)) &&
+        (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr)))
+        data->count++;
+
+    virObjectUnlock(obj);
+    return 0;
+}
+
+
 int
 virNodeDeviceObjListNumOfDevices(virNodeDeviceObjListPtr devs,
                                  virConnectPtr conn,
                                  const char *cap,
                                  virNodeDeviceObjListFilter aclfilter)
 {
-    size_t i;
-    int ndevs = 0;
+    struct virNodeDeviceCountData data = {
+        .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .count = 0 };
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virObjectLock(obj);
-        if ((!aclfilter || aclfilter(conn, obj->def)) &&
-            (!cap || virNodeDeviceObjHasCap(obj, cap)))
-            ++ndevs;
-        virObjectUnlock(obj);
-    }
+    virObjectLock(devs);
+    virHashForEach(devs->objs, virNodeDeviceObjListNumOfDevicesCallback, &data);
+    virObjectUnlock(devs);
+
+    return data.count;
+}
+
+
+struct virNodeDeviceGetNamesData {
+    virConnectPtr conn;
+    virNodeDeviceObjListFilter aclfilter;
+    const char *matchstr;
+    int nnames;
+    char **names;
+    int maxnames;
+    bool error;
+};
+
+static int
+virNodeDeviceObjListGetNamesCallback(void *payload,
+                                     const void *name ATTRIBUTE_UNUSED,
+                                     void *opaque)
+{
+    virNodeDeviceObjPtr obj = payload;
+    virNodeDeviceDefPtr def;
+    struct virNodeDeviceGetNamesData *data = opaque;
+    virNodeDeviceObjListFilter aclfilter = data->aclfilter;
+
+    if (data->error)
+        return 0;
 
-    return ndevs;
+    virObjectLock(obj);
+    def = obj->def;
+
+    if ((!aclfilter || aclfilter(data->conn, def)) &&
+        (!data->matchstr || virNodeDeviceObjHasCap(obj, data->matchstr))) {
+        if (VIR_STRDUP(data->names[data->nnames], def->name) < 0) {
+            data->error = true;
+            goto cleanup;
+        }
+        data->nnames++;
+     }
+
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
 }
 
 
@@ -649,28 +853,22 @@ virNodeDeviceObjListGetNames(virNodeDeviceObjListPtr devs,
                              char **const names,
                              int maxnames)
 {
-    int nnames = 0;
-    size_t i;
+    struct virNodeDeviceGetNamesData data = {
+        .conn = conn, .aclfilter = aclfilter, .matchstr = cap, .names = names,
+        .nnames = 0, .maxnames = maxnames, .error = false };
 
-    for (i = 0; i < devs->count && nnames < maxnames; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virObjectLock(obj);
-        if ((!aclfilter || aclfilter(conn, obj->def)) &&
-            (!cap || virNodeDeviceObjHasCap(obj, cap))) {
-            if (VIR_STRDUP(names[nnames], obj->def->name) < 0) {
-                virObjectUnlock(obj);
-                goto failure;
-            }
-            nnames++;
-        }
-        virObjectUnlock(obj);
-    }
+    virObjectLock(devs);
+    virHashForEach(devs->objs, virNodeDeviceObjListGetNamesCallback, &data);
+    virObjectUnlock(devs);
+
+    if (data.error)
+        goto error;
 
-    return nnames;
+    return data.nnames;
 
- failure:
-    while (--nnames >= 0)
-        VIR_FREE(names[nnames]);
+ error:
+    while (--data.nnames)
+        VIR_FREE(data.names[data.nnames]);
     return -1;
 }
 
@@ -707,6 +905,51 @@ virNodeDeviceMatch(virNodeDeviceObjPtr obj,
 #undef MATCH
 
 
+struct virNodeDeviceObjListExportData {
+    virConnectPtr conn;
+    virNodeDeviceObjListFilter aclfilter;
+    unsigned int flags;
+    virNodeDevicePtr *devices;
+    int ndevices;
+    bool error;
+};
+
+static int
+virNodeDeviceObjListExportCallback(void *payload,
+                                   const void *name ATTRIBUTE_UNUSED,
+                                   void *opaque)
+{
+    virNodeDeviceObjPtr obj = payload;
+    virNodeDeviceDefPtr def;
+    struct virNodeDeviceObjListExportData *data = opaque;
+    virNodeDevicePtr device = NULL;
+
+    if (data->error)
+        return 0;
+
+    virObjectLock(obj);
+    def = obj->def;
+
+    if ((!data->aclfilter || data->aclfilter(data->conn, def)) &&
+        virNodeDeviceMatch(obj, data->flags)) {
+        if (data->devices) {
+            if (!(device = virGetNodeDevice(data->conn, def->name)) ||
+                VIR_STRDUP(device->parent, def->parent) < 0) {
+                virObjectUnref(device);
+                data->error = true;
+                goto cleanup;
+            }
+            data->devices[data->ndevices] = device;
+        }
+        data->ndevices++;
+    }
+
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
+}
+
+
 int
 virNodeDeviceObjListExport(virConnectPtr conn,
                            virNodeDeviceObjListPtr devs,
@@ -714,49 +957,31 @@ virNodeDeviceObjListExport(virConnectPtr conn,
                            virNodeDeviceObjListFilter aclfilter,
                            unsigned int flags)
 {
-    virNodeDevicePtr *tmp_devices = NULL;
-    virNodeDevicePtr device = NULL;
-    int ndevices = 0;
-    int ret = -1;
-    size_t i;
+    struct virNodeDeviceObjListExportData data = {
+        .conn = conn, .aclfilter = aclfilter, .flags = flags,
+        .devices = NULL, .ndevices = 0, .error = false };
+
+    virObjectLock(devs);
+    if (devices &&
+        VIR_ALLOC_N(data.devices, virHashSize(devs->objs) + 1) < 0) {
+        virObjectUnlock(devs);
+        return -1;
+    }
 
-    if (devices && VIR_ALLOC_N(tmp_devices, devs->count + 1) < 0)
-        goto cleanup;
+    virHashForEach(devs->objs, virNodeDeviceObjListExportCallback, &data);
+    virObjectUnlock(devs);
 
-    for (i = 0; i < devs->count; i++) {
-        virNodeDeviceObjPtr obj = devs->objs[i];
-        virObjectLock(obj);
-        if ((!aclfilter || aclfilter(conn, obj->def)) &&
-            virNodeDeviceMatch(obj, flags)) {
-            if (devices) {
-                if (!(device = virGetNodeDevice(conn, obj->def->name)) ||
-                    VIR_STRDUP(device->parent, obj->def->parent) < 0) {
-                    virObjectUnref(device);
-                    virObjectUnlock(obj);
-                    goto cleanup;
-                }
-                tmp_devices[ndevices] = device;
-            }
-            ndevices++;
-        }
-        virObjectUnlock(obj);
-    }
+    if (data.error)
+        goto cleanup;
 
-    if (tmp_devices) {
-        /* trim the array to the final size */
-        ignore_value(VIR_REALLOC_N(tmp_devices, ndevices + 1));
-        *devices = tmp_devices;
-        tmp_devices = NULL;
-    }
+    if (data.devices) {
+        ignore_value(VIR_REALLOC_N(data.devices, data.ndevices + 1));
+        *devices = data.devices;
+     }
 
-    ret = ndevices;
+    return data.ndevices;
 
  cleanup:
-    if (tmp_devices) {
-        for (i = 0; i < ndevices; i++)
-            virObjectUnref(tmp_devices[i]);
-    }
-
-    VIR_FREE(tmp_devices);
-    return ret;
+    virObjectListFree(data.devices);
+    return -1;
 }
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Jul 20, 2017 at 10:08:14AM -0400, John Ferlan wrote:
> Rather than use a forward linked list of elements, it'll be much more
> efficient to use a hash table to reference the elements by unique name
> and to perform hash searches.
>
> This patch does all the heavy lifting of converting the list object to
> use a self locking list that contains the hash table. Each of the FindBy
> functions that do not involve finding the object by it's key (name) is
> converted to use virHashSearch in order to find the specific object.
> When searching for the key (name), it's possible to use virHashLookup.
> For any of the list perusal functions that are required to evaluate
> each object, the virHashForEach function is used.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virnodedeviceobj.c | 575 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 400 insertions(+), 175 deletions(-)
>
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index 035a56d..58481e7 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -25,6 +25,7 @@
>  #include "viralloc.h"
>  #include "virnodedeviceobj.h"
>  #include "virerror.h"
> +#include "virhash.h"
>  #include "virlog.h"
>  #include "virstring.h"
>
> @@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
>  };
>
>  struct _virNodeDeviceObjList {
> -    size_t count;
> -    virNodeDeviceObjPtr *objs;
> +    virObjectLockable parent;
> +
> +    /* name string -> virNodeDeviceObj  mapping
> +     * for O(1), lockless lookup-by-uuid */
> +    virHashTable *objs;
> +
>  };
>
>
>  static virClassPtr virNodeDeviceObjClass;
> +static virClassPtr virNodeDeviceObjListClass;
>  static void virNodeDeviceObjDispose(void *opaque);
> +static void virNodeDeviceObjListDispose(void *opaque);
>
>  static int
>  virNodeDeviceObjOnceInit(void)
> @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
>                                                virNodeDeviceObjDispose)))
>          return -1;
>
> +    if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
> +                                                  "virNodeDeviceObjList",
> +                                                  sizeof(virNodeDeviceObjList),
> +                                                  virNodeDeviceObjListDispose)))
> +        return -1;
> +
>      return 0;
>  }
>
> @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
>  }
>
>
> +static int
> +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
> +                                            const void *name ATTRIBUTE_UNUSED,
> +                                            const void *opaque)
> +{
> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
> +    virNodeDeviceDefPtr def;

As we discussed in v5, I'd like you to drop ^these @def (and only @def...)
declarations and usages across the whole patch for the reasons I already
mentioned - it makes sense in general, but it's not very useful in this
specific case.

> +    const char *sysfs_path = opaque;
> +    int want = 0;
> +
> +    virObjectLock(obj);
> +    def = obj->def;

...

>  virNodeDeviceObjPtr
>  virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>                                      const char *sysfs_path)
>  {
> -    size_t i;
> +    virNodeDeviceObjPtr obj;
>
> -    for (i = 0; i < devs->count; i++) {
> -        virNodeDeviceObjPtr obj = devs->objs[i];
> -        virNodeDeviceDefPtr def;
> +    virObjectLock(devs);
> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
> +                        (void *)sysfs_path, NULL);
> +    virObjectRef(obj);
> +    virObjectUnlock(devs);
>
> +    if (obj)
>          virObjectLock(obj);
> -        def = obj->def;
> -        if ((def->sysfs_path != NULL) &&
> -            (STREQ(def->sysfs_path, sysfs_path))) {
> -            return virObjectRef(obj);
> -        }
> -        virObjectUnlock(obj);
> -    }
>
> -    return NULL;
> +    return obj;

With reference to v5:
The fact that creating a helper wasn't met with an agreement from the
reviewer's side in one case doesn't mean it doesn't make sense to do in an
other case, like this one. So, what I actually meant by creating a helper for:

BySysfsPath
ByWWNs
ByFabricWWN
ByCap
ByCapSCSIByWWNs

is just simply move the lock and search logic to a separate function, that's
all, see my attached patch. And then, as you suggested in your v5 response to
this patch, we can move from here (my patch included) and potentially do some
more refactoring.

ACK if you move the lock-search-ref-unlock-return snippets to a separate
function for the cases mentioned above.

Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/24/2017 03:52 AM, Erik Skultety wrote:
> On Thu, Jul 20, 2017 at 10:08:14AM -0400, John Ferlan wrote:
>> Rather than use a forward linked list of elements, it'll be much more
>> efficient to use a hash table to reference the elements by unique name
>> and to perform hash searches.
>>
>> This patch does all the heavy lifting of converting the list object to
>> use a self locking list that contains the hash table. Each of the FindBy
>> functions that do not involve finding the object by it's key (name) is
>> converted to use virHashSearch in order to find the specific object.
>> When searching for the key (name), it's possible to use virHashLookup.
>> For any of the list perusal functions that are required to evaluate
>> each object, the virHashForEach function is used.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virnodedeviceobj.c | 575 ++++++++++++++++++++++++++++++--------------
>>  1 file changed, 400 insertions(+), 175 deletions(-)
>>
>> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
>> index 035a56d..58481e7 100644
>> --- a/src/conf/virnodedeviceobj.c
>> +++ b/src/conf/virnodedeviceobj.c
>> @@ -25,6 +25,7 @@
>>  #include "viralloc.h"
>>  #include "virnodedeviceobj.h"
>>  #include "virerror.h"
>> +#include "virhash.h"
>>  #include "virlog.h"
>>  #include "virstring.h"
>>
>> @@ -39,13 +40,19 @@ struct _virNodeDeviceObj {
>>  };
>>
>>  struct _virNodeDeviceObjList {
>> -    size_t count;
>> -    virNodeDeviceObjPtr *objs;
>> +    virObjectLockable parent;
>> +
>> +    /* name string -> virNodeDeviceObj  mapping
>> +     * for O(1), lockless lookup-by-uuid */
>> +    virHashTable *objs;
>> +
>>  };
>>
>>
>>  static virClassPtr virNodeDeviceObjClass;
>> +static virClassPtr virNodeDeviceObjListClass;
>>  static void virNodeDeviceObjDispose(void *opaque);
>> +static void virNodeDeviceObjListDispose(void *opaque);
>>
>>  static int
>>  virNodeDeviceObjOnceInit(void)
>> @@ -56,6 +63,12 @@ virNodeDeviceObjOnceInit(void)
>>                                                virNodeDeviceObjDispose)))
>>          return -1;
>>
>> +    if (!(virNodeDeviceObjListClass = virClassNew(virClassForObjectLockable(),
>> +                                                  "virNodeDeviceObjList",
>> +                                                  sizeof(virNodeDeviceObjList),
>> +                                                  virNodeDeviceObjListDispose)))
>> +        return -1;
>> +
>>      return 0;
>>  }
>>
>> @@ -211,26 +224,49 @@ virNodeDeviceFindVPORTCapDef(const virNodeDeviceObj *obj)
>>  }
>>
>>
>> +static int
>> +virNodeDeviceObjListFindBySysfsPathCallback(const void *payload,
>> +                                            const void *name ATTRIBUTE_UNUSED,
>> +                                            const void *opaque)
>> +{
>> +    virNodeDeviceObjPtr obj = (virNodeDeviceObjPtr) payload;
>> +    virNodeDeviceDefPtr def;
> 
> As we discussed in v5, I'd like you to drop ^these @def (and only @def...)
> declarations and usages across the whole patch for the reasons I already
> mentioned - it makes sense in general, but it's not very useful in this
> specific case.
> 

<sigh> OK, I really do dislike the obj->def->X syntax "just" for a few
functions while others use the @def nomenclature.

>> +    const char *sysfs_path = opaque;
>> +    int want = 0;
>> +
>> +    virObjectLock(obj);
>> +    def = obj->def;
> 
> ...
> 
>>  virNodeDeviceObjPtr
>>  virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>>                                      const char *sysfs_path)
>>  {
>> -    size_t i;
>> +    virNodeDeviceObjPtr obj;
>>
>> -    for (i = 0; i < devs->count; i++) {
>> -        virNodeDeviceObjPtr obj = devs->objs[i];
>> -        virNodeDeviceDefPtr def;
>> +    virObjectLock(devs);
>> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
>> +                        (void *)sysfs_path, NULL);
>> +    virObjectRef(obj);
>> +    virObjectUnlock(devs);
>>
>> +    if (obj)
>>          virObjectLock(obj);
>> -        def = obj->def;
>> -        if ((def->sysfs_path != NULL) &&
>> -            (STREQ(def->sysfs_path, sysfs_path))) {
>> -            return virObjectRef(obj);
>> -        }
>> -        virObjectUnlock(obj);
>> -    }
>>
>> -    return NULL;
>> +    return obj;
> 
> With reference to v5:
> The fact that creating a helper wasn't met with an agreement from the
> reviewer's side in one case doesn't mean it doesn't make sense to do in an
> other case, like this one. So, what I actually meant by creating a helper for:
> 
> BySysfsPath
> ByWWNs
> ByFabricWWN
> ByCap
> ByCapSCSIByWWNs
> 
> is just simply move the lock and search logic to a separate function, that's
> all, see my attached patch. And then, as you suggested in your v5 response to
> this patch, we can move from here (my patch included) and potentially do some
> more refactoring.

Replies don't include your patch; however, I will note if you jump to
patch 13:

https://www.redhat.com/archives/libvir-list/2017-June/msg00929.html

of the virObject series I posted last month:

https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html

that you'll see that is the direction this would be headed anyway. I can
do this sooner that's fine, although I prefer the name
virNodeDeviceObjListSearch as opposed to virNodeDeviceObjListFindHelper.

Ironically though you didn't like the @def usage, still you chose
virHashSearcher cb = $functionName which has one use to be used as an
argument to the function even though $functionName could be used
directly in the call. The only advantage of @cb is that it reduces the
line length of the call.

John

> 
> ACK if you move the lock-search-ref-unlock-return snippets to a separate
> function for the cases mentioned above.
> 
> Erik
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> >>  virNodeDeviceObjPtr
> >>  virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
> >>                                      const char *sysfs_path)
> >>  {
> >> -    size_t i;
> >> +    virNodeDeviceObjPtr obj;
> >>
> >> -    for (i = 0; i < devs->count; i++) {
> >> -        virNodeDeviceObjPtr obj = devs->objs[i];
> >> -        virNodeDeviceDefPtr def;
> >> +    virObjectLock(devs);
> >> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
> >> +                        (void *)sysfs_path, NULL);
> >> +    virObjectRef(obj);
> >> +    virObjectUnlock(devs);
> >>
> >> +    if (obj)
> >>          virObjectLock(obj);
> >> -        def = obj->def;
> >> -        if ((def->sysfs_path != NULL) &&
> >> -            (STREQ(def->sysfs_path, sysfs_path))) {
> >> -            return virObjectRef(obj);
> >> -        }
> >> -        virObjectUnlock(obj);
> >> -    }
> >>
> >> -    return NULL;
> >> +    return obj;
> >
> > With reference to v5:
> > The fact that creating a helper wasn't met with an agreement from the
> > reviewer's side in one case doesn't mean it doesn't make sense to do in an
> > other case, like this one. So, what I actually meant by creating a helper for:
> >
> > BySysfsPath
> > ByWWNs
> > ByFabricWWN
> > ByCap
> > ByCapSCSIByWWNs
> >
> > is just simply move the lock and search logic to a separate function, that's
> > all, see my attached patch. And then, as you suggested in your v5 response to
> > this patch, we can move from here (my patch included) and potentially do some
> > more refactoring.
>
> Replies don't include your patch; however, I will note if you jump to
> patch 13:

What do you mean? Don't you see squash.patch in the attachment? (yes, I
attached a patch instead of inlining it, the reason for that being that the
patch is not particularly small and inlining it would disrupt the thread...)

>
> https://www.redhat.com/archives/libvir-list/2017-June/msg00929.html
>
> of the virObject series I posted last month:
>
> https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
>
> that you'll see that is the direction this would be headed anyway. I can
> do this sooner that's fine, although I prefer the name
> virNodeDeviceObjListSearch as opposed to virNodeDeviceObjListFindHelper.

Yeah, ok, since I'd rather not review multiple series that more-or-less depend
on each other in parallel and try to connect relating changes back and forth, I
couldn't have noticed this fact, but looking at the patch you linked, sure, the
approach is the same. As long as the change we're discussing makes it in
(one way or the other), I'm fine with it.

>
> Ironically though you didn't like the @def usage, still you chose
> virHashSearcher cb = $functionName which has one use to be used as an

Alright, fair point, any further discussion would be unnecessary, act like I
didn't say anything.

My ACK still stands.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/24/2017 09:08 AM, Erik Skultety wrote:
>>>>  virNodeDeviceObjPtr
>>>>  virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>>>>                                      const char *sysfs_path)
>>>>  {
>>>> -    size_t i;
>>>> +    virNodeDeviceObjPtr obj;
>>>>
>>>> -    for (i = 0; i < devs->count; i++) {
>>>> -        virNodeDeviceObjPtr obj = devs->objs[i];
>>>> -        virNodeDeviceDefPtr def;
>>>> +    virObjectLock(devs);
>>>> +    obj = virHashSearch(devs->objs, virNodeDeviceObjListFindBySysfsPathCallback,
>>>> +                        (void *)sysfs_path, NULL);
>>>> +    virObjectRef(obj);
>>>> +    virObjectUnlock(devs);
>>>>
>>>> +    if (obj)
>>>>          virObjectLock(obj);
>>>> -        def = obj->def;
>>>> -        if ((def->sysfs_path != NULL) &&
>>>> -            (STREQ(def->sysfs_path, sysfs_path))) {
>>>> -            return virObjectRef(obj);
>>>> -        }
>>>> -        virObjectUnlock(obj);
>>>> -    }
>>>>
>>>> -    return NULL;
>>>> +    return obj;
>>>
>>> With reference to v5:
>>> The fact that creating a helper wasn't met with an agreement from the
>>> reviewer's side in one case doesn't mean it doesn't make sense to do in an
>>> other case, like this one. So, what I actually meant by creating a helper for:
>>>
>>> BySysfsPath
>>> ByWWNs
>>> ByFabricWWN
>>> ByCap
>>> ByCapSCSIByWWNs
>>>
>>> is just simply move the lock and search logic to a separate function, that's
>>> all, see my attached patch. And then, as you suggested in your v5 response to
>>> this patch, we can move from here (my patch included) and potentially do some
>>> more refactoring.
>>
>> Replies don't include your patch; however, I will note if you jump to
>> patch 13:
> 
> What do you mean? Don't you see squash.patch in the attachment? (yes, I
> attached a patch instead of inlining it, the reason for that being that the
> patch is not particularly small and inlining it would disrupt the thread...)
> 

Oh - I meant when I reply to your patch with an attachment, I don't get
your attachment in the reply. So I guess it was a early morning and
feeble attempt to note that one had to go look at your reply in order to
get context!

I've altered the two @def in the callback functions to be
"obj->def->sysfs_path" and "obj->def->caps".  I've also made the single
virHashSearch API/helper....

I also changed the comment in my branch for this patch (from v5 review)
to say "lookup-by-name" instead of "lookup-by-uuid" and modified the
comment in .4 to be appropriately placed.

Just running things through a couple of tests before doing the push...

Thanks -

John

>>
>> https://www.redhat.com/archives/libvir-list/2017-June/msg00929.html
>>
>> of the virObject series I posted last month:
>>
>> https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html
>>
>> that you'll see that is the direction this would be headed anyway. I can
>> do this sooner that's fine, although I prefer the name
>> virNodeDeviceObjListSearch as opposed to virNodeDeviceObjListFindHelper.
> 
> Yeah, ok, since I'd rather not review multiple series that more-or-less depend
> on each other in parallel and try to connect relating changes back and forth, I
> couldn't have noticed this fact, but looking at the patch you linked, sure, the
> approach is the same. As long as the change we're discussing makes it in
> (one way or the other), I'm fine with it.
> 
>>
>> Ironically though you didn't like the @def usage, still you chose
>> virHashSearcher cb = $functionName which has one use to be used as an
> 
> Alright, fair point, any further discussion would be unnecessary, act like I
> didn't say anything.>
> My ACK still stands.
> 
> Erik
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> >> Replies don't include your patch; however, I will note if you jump to
> >> patch 13:
> >
> > What do you mean? Don't you see squash.patch in the attachment? (yes, I
> > attached a patch instead of inlining it, the reason for that being that the
> > patch is not particularly small and inlining it would disrupt the thread...)
> >
>
> Oh - I meant when I reply to your patch with an attachment, I don't get
> your attachment in the reply. So I guess it was a early morning and
> feeble attempt to note that one had to go look at your reply in order to
> get context!

Nah, it's just my poor English - I could have considered other meanings, I just
went with the most straightforward without thinking about it much, it does make
sense now that you helped to steer the light so it could dawn on the marblehead.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list