[libvirt] [PATCH v2] interfaces: Convert virInterfaceObjList to virObjectRWLockable

13 Oct 2017

Rather than a forward linked list, let's use the ObjectRWLockable object
in order to manage the data.

Requires numerous changes from List to Object management similar to
many other drivers/vir*obj.c modules

Signed-off-by: John Ferlan <jferlan@redhat.com>
---

 v1: https://www.redhat.com/archives/libvir-list/2017-October/msg00387.html

 Difference to v1 - separate out each of the virHashForEach callback helpers
 and *Data structures.

 src/conf/virinterfaceobj.c | 341 ++++++++++++++++++++++++++++++---------------
 src/libvirt_private.syms   |   1 -
 src/test/test_driver.c     |   6 +-
 3 files changed, 229 insertions(+), 119 deletions(-)

diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
index e993b929d7..4bd371bb1e 100644
--- a/src/conf/virinterfaceobj.c
+++ b/src/conf/virinterfaceobj.c
@@ -25,6 +25,7 @@
 #include "viralloc.h"
 #include "virerror.h"
 #include "virinterfaceobj.h"
+#include "virhash.h"
 #include "virlog.h"
 #include "virstring.h"
 
@@ -40,14 +41,19 @@ struct _virInterfaceObj {
 };
 
 struct _virInterfaceObjList {
-    size_t count;
-    virInterfaceObjPtr *objs;
+    virObjectRWLockable parent;
+
+    /* name string -> virInterfaceObj  mapping
+     * for O(1), lockless lookup-by-name */
+    virHashTable *objsName;
 };
 
 /* virInterfaceObj manipulation */
 
 static virClassPtr virInterfaceObjClass;
+static virClassPtr virInterfaceObjListClass;
 static void virInterfaceObjDispose(void *obj);
+static void virInterfaceObjListDispose(void *obj);
 
 static int
 virInterfaceObjOnceInit(void)
@@ -58,6 +64,12 @@ virInterfaceObjOnceInit(void)
                                              virInterfaceObjDispose)))
         return -1;
 
+    if (!(virInterfaceObjListClass = virClassNew(virClassForObjectRWLockable(),
+                                                 "virInterfaceObjList",
+                                                 sizeof(virInterfaceObjList),
+                                                 virInterfaceObjListDispose)))
+        return -1;
+
     return 0;
 }
 
@@ -130,119 +142,189 @@ virInterfaceObjListNew(void)
 {
     virInterfaceObjListPtr interfaces;
 
-    if (VIR_ALLOC(interfaces) < 0)
+    if (virInterfaceObjInitialize() < 0)
         return NULL;
+
+    if (!(interfaces = virObjectRWLockableNew(virInterfaceObjListClass)))
+        return NULL;
+
+    if (!(interfaces->objsName = virHashCreate(10, virObjectFreeHashData))) {
+        virObjectUnref(interfaces);
+        return NULL;
+    }
+
     return interfaces;
 }
 
 
+struct _virInterfaceObjForEachData {
+    bool wantActive;
+    const char *matchStr;
+    bool error;
+    int nElems;
+    int maxElems;
+    char **const elems;
+};
+
+static int
+virInterfaceObjListFindByMACStringCb(void *payload,
+                                     const void *name ATTRIBUTE_UNUSED,
+                                     void *opaque)
+{
+    virInterfaceObjPtr obj = payload;
+    struct _virInterfaceObjForEachData *data = opaque;
+
+    if (data->error)
+        return 0;
+
+    if (data->nElems == data->maxElems)
+        return 0;
+
+    virObjectLock(obj);
+
+    if (STRCASEEQ(obj->def->mac, data->matchStr)) {
+        if (VIR_STRDUP(data->elems[data->nElems], data->matchStr) < 0) {
+            data->error = true;
+            goto cleanup;
+        }
+        data->nElems++;
+    }
+
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
+}
+
+
 int
 virInterfaceObjListFindByMACString(virInterfaceObjListPtr interfaces,
                                    const char *mac,
                                    char **const matches,
                                    int maxmatches)
 {
-    size_t i;
-    int matchct = 0;
+    struct _virInterfaceObjForEachData data = { .matchStr = mac,
+                                                .error = false,
+                                                .nElems = 0,
+                                                .maxElems = maxmatches,
+                                                .elems = matches };
 
-    for (i = 0; i < interfaces->count; i++) {
-        virInterfaceObjPtr obj = interfaces->objs[i];
-        virInterfaceDefPtr def;
+    virObjectRWLockRead(interfaces);
+    virHashForEach(interfaces->objsName, virInterfaceObjListFindByMACStringCb,
+                   &data);
+    virObjectRWUnlock(interfaces);
 
-        virObjectLock(obj);
-        def = obj->def;
-        if (STRCASEEQ(def->mac, mac)) {
-            if (matchct < maxmatches) {
-                if (VIR_STRDUP(matches[matchct], def->name) < 0) {
-                    virObjectUnlock(obj);
-                    goto error;
-                }
-                matchct++;
-            }
-        }
-        virObjectUnlock(obj);
-    }
-    return matchct;
+    if (data.error)
+        goto error;
+
+    return data.nElems;
 
  error:
-    while (--matchct >= 0)
-        VIR_FREE(matches[matchct]);
+    while (--data.nElems >= 0)
+        VIR_FREE(data.elems[data.nElems]);
 
     return -1;
 }
 
 
+static virInterfaceObjPtr
+virInterfaceObjListFindByNameLocked(virInterfaceObjListPtr interfaces,
+                                    const char *name)
+{
+    return virObjectRef(virHashLookup(interfaces->objsName, name));
+}
+
+
 virInterfaceObjPtr
 virInterfaceObjListFindByName(virInterfaceObjListPtr interfaces,
                               const char *name)
 {
-    size_t i;
-
-    for (i = 0; i < interfaces->count; i++) {
-        virInterfaceObjPtr obj = interfaces->objs[i];
-        virInterfaceDefPtr def;
-
+    virInterfaceObjPtr obj;
+    virObjectRWLockRead(interfaces);
+    obj = virInterfaceObjListFindByNameLocked(interfaces, name);
+    virObjectRWUnlock(interfaces);
+    if (obj)
         virObjectLock(obj);
-        def = obj->def;
-        if (STREQ(def->name, name))
-            return virObjectRef(obj);
-        virObjectUnlock(obj);
-    }
 
-    return NULL;
+    return obj;
 }
 
 
 void
-virInterfaceObjListFree(virInterfaceObjListPtr interfaces)
+virInterfaceObjListDispose(void *obj)
+{
+    virInterfaceObjListPtr interfaces = obj;
+
+    virHashFree(interfaces->objsName);
+}
+
+
+struct _virInterfaceObjListCloneData {
+    bool error;
+    virInterfaceObjListPtr dest;
+};
+
+static int
+virInterfaceObjListCloneCb(void *payload,
+                           const void *name ATTRIBUTE_UNUSED,
+                           void *opaque)
 {
-    size_t i;
+    virInterfaceObjPtr srcObj = payload;
+    struct _virInterfaceObjListCloneData *data = opaque;
+    char *xml = NULL;
+    virInterfaceDefPtr backup = NULL;
+    virInterfaceObjPtr obj;
+
+    if (data->error)
+        return 0;
+
+    virObjectLock(srcObj);
+
+    if (!(xml = virInterfaceDefFormat(srcObj->def)))
+        goto error;
+
+    if (!(backup = virInterfaceDefParseString(xml)))
+        goto error;
+    VIR_FREE(xml);
 
-    for (i = 0; i < interfaces->count; i++)
-        virObjectUnref(interfaces->objs[i]);
-    VIR_FREE(interfaces->objs);
-    VIR_FREE(interfaces);
+    if (!(obj = virInterfaceObjListAssignDef(data->dest, backup)))
+        goto error;
+    virInterfaceObjEndAPI(&obj);
+
+    virObjectUnlock(srcObj);
+    return 0;
+
+ error:
+    data->error = true;
+    VIR_FREE(xml);
+    virInterfaceDefFree(backup);
+    virObjectUnlock(srcObj);
+    return 0;
 }
 
 
 virInterfaceObjListPtr
 virInterfaceObjListClone(virInterfaceObjListPtr interfaces)
 {
-    size_t i;
-    unsigned int cnt;
-    virInterfaceObjListPtr dest;
+    struct _virInterfaceObjListCloneData data = { .error = false,
+                                                  .dest = NULL };
 
     if (!interfaces)
         return NULL;
 
-    if (!(dest = virInterfaceObjListNew()))
+    if (!(data.dest = virInterfaceObjListNew()))
         return NULL;
 
-    cnt = interfaces->count;
-    for (i = 0; i < cnt; i++) {
-        virInterfaceObjPtr srcobj = interfaces->objs[i];
-        virInterfaceDefPtr backup;
-        virInterfaceObjPtr obj;
-        char *xml = virInterfaceDefFormat(srcobj->def);
+    virObjectRWLockRead(interfaces);
+    virHashForEach(interfaces->objsName, virInterfaceObjListCloneCb, &data);
+    virObjectRWUnlock(interfaces);
 
-        if (!xml)
-            goto error;
+    if (data.error)
+        goto error;
 
-        if (!(backup = virInterfaceDefParseString(xml))) {
-            VIR_FREE(xml);
-            goto error;
-        }
-
-        VIR_FREE(xml);
-        if (!(obj = virInterfaceObjListAssignDef(dest, backup)))
-            goto error;
-        virInterfaceObjEndAPI(&obj);
-    }
-
-    return dest;
+    return data.dest;
 
  error:
-    virInterfaceObjListFree(dest);
+    virObjectUnref(data.dest);
     return NULL;
 }
 
@@ -253,9 +335,11 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
 {
     virInterfaceObjPtr obj;
 
-    if ((obj = virInterfaceObjListFindByName(interfaces, def->name))) {
+    virObjectRWLockWrite(interfaces);
+    if ((obj = virInterfaceObjListFindByNameLocked(interfaces, def->name))) {
         virInterfaceDefFree(obj->def);
         obj->def = def;
+        virObjectRWUnlock(interfaces);
 
         return obj;
     }
@@ -263,13 +347,19 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces,
     if (!(obj = virInterfaceObjNew()))
         return NULL;
 
-    if (VIR_APPEND_ELEMENT_COPY(interfaces->objs,
-                                interfaces->count, obj) < 0) {
-        virInterfaceObjEndAPI(&obj);
-        return NULL;
-    }
+    if (virHashAddEntry(interfaces->objsName, def->name, obj) < 0)
+        goto error;
+    virObjectRef(obj);
+
     obj->def = def;
-    return virObjectRef(obj);
+    virObjectRWUnlock(interfaces);
+
+    return obj;
+
+ error:
+    virInterfaceObjEndAPI(&obj);
+    virObjectRWUnlock(interfaces);
+    return NULL;
 }
 
 
@@ -277,20 +367,48 @@ void
 virInterfaceObjListRemove(virInterfaceObjListPtr interfaces,
                           virInterfaceObjPtr obj)
 {
-    size_t i;
-
+    virObjectRef(obj);
+    virObjectUnlock(obj);
+    virObjectRWLockWrite(interfaces);
+    virObjectLock(obj);
+    virHashRemoveEntry(interfaces->objsName, obj->def->name);
     virObjectUnlock(obj);
-    for (i = 0; i < interfaces->count; i++) {
-        virObjectLock(interfaces->objs[i]);
-        if (interfaces->objs[i] == obj) {
-            virObjectUnlock(interfaces->objs[i]);
-            virObjectUnref(interfaces->objs[i]);
-
-            VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count);
-            break;
+    virObjectUnref(obj);
+    virObjectRWUnlock(interfaces);
+}
+
+
+static int
+virInterfaceObjListForEachCb(void *payload,
+                             const void *name ATTRIBUTE_UNUSED,
+                             void *opaque)
+{
+    virInterfaceObjPtr obj = payload;
+    struct _virInterfaceObjForEachData *data = opaque;
+
+    if (data->error)
+        return 0;
+
+    if (data->maxElems >= 0 && data->nElems == data->maxElems)
+        return 0;
+
+    virObjectLock(obj);
+
+    if (data->wantActive != virInterfaceObjIsActive(obj))
+        goto cleanup;
+
+    if (data->elems) {
+        if (VIR_STRDUP(data->elems[data->nElems], obj->def->name) < 0) {
+            data->error = true;
+            goto cleanup;
         }
-        virObjectUnlock(interfaces->objs[i]);
     }
+
+    data->nElems++;
+
+ cleanup:
+    virObjectUnlock(obj);
+    return 0;
 }
 
 
@@ -298,18 +416,17 @@ int
 virInterfaceObjListNumOfInterfaces(virInterfaceObjListPtr interfaces,
                                    bool wantActive)
 {
-    size_t i;
-    int ninterfaces = 0;
+    struct _virInterfaceObjForEachData data = { .wantActive = wantActive,
+                                                .error = false,
+                                                .nElems = 0,
+                                                .maxElems = -1,
+                                                .elems = NULL };
 
-    for (i = 0; (i < interfaces->count); i++) {
-        virInterfaceObjPtr obj = interfaces->objs[i];
-        virObjectLock(obj);
-        if (wantActive == virInterfaceObjIsActive(obj))
-            ninterfaces++;
-        virObjectUnlock(obj);
-    }
+    virObjectRWLockRead(interfaces);
+    virHashForEach(interfaces->objsName, virInterfaceObjListForEachCb, &data);
+    virObjectRWUnlock(interfaces);
 
-    return ninterfaces;
+    return data.nElems;
 }
 
 
@@ -319,30 +436,24 @@ virInterfaceObjListGetNames(virInterfaceObjListPtr interfaces,
                             char **const names,
                             int maxnames)
 {
-    int nnames = 0;
-    size_t i;
+    struct _virInterfaceObjForEachData data = { .wantActive = wantActive,
+                                                .error = false,
+                                                .nElems = 0,
+                                                .maxElems = maxnames,
+                                                .elems = names };
 
-    for (i = 0; i < interfaces->count && nnames < maxnames; i++) {
-        virInterfaceObjPtr obj = interfaces->objs[i];
-        virInterfaceDefPtr def;
+    virObjectRWLockRead(interfaces);
+    virHashForEach(interfaces->objsName, virInterfaceObjListForEachCb, &data);
+    virObjectRWUnlock(interfaces);
 
-        virObjectLock(obj);
-        def = obj->def;
-        if (wantActive == virInterfaceObjIsActive(obj)) {
-            if (VIR_STRDUP(names[nnames], def->name) < 0) {
-                virObjectUnlock(obj);
-                goto failure;
-            }
-            nnames++;
-        }
-        virObjectUnlock(obj);
-    }
+    if (data.error)
+        goto error;
 
-    return nnames;
+    return data.nElems;
 
- failure:
-    while (--nnames >= 0)
-        VIR_FREE(names[nnames]);
+ error:
+    while (--data.nElems >= 0)
+        VIR_FREE(data.elems[data.nElems]);
 
     return -1;
 }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 26c5ddb405..9133199895 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -932,7 +932,6 @@ virInterfaceObjListAssignDef;
 virInterfaceObjListClone;
 virInterfaceObjListFindByMACString;
 virInterfaceObjListFindByName;
-virInterfaceObjListFree;
 virInterfaceObjListGetNames;
 virInterfaceObjListNew;
 virInterfaceObjListNumOfInterfaces;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 1c48347994..14bba7494c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -154,7 +154,7 @@ testDriverFree(testDriverPtr driver)
     virObjectUnref(driver->domains);
     virNodeDeviceObjListFree(driver->devs);
     virObjectUnref(driver->networks);
-    virInterfaceObjListFree(driver->ifaces);
+    virObjectUnref(driver->ifaces);
     virStoragePoolObjListFree(&driver->pools);
     virObjectUnref(driver->eventState);
     virMutexUnlock(&driver->lock);
@@ -3880,7 +3880,7 @@ testInterfaceChangeCommit(virConnectPtr conn,
         goto cleanup;
     }
 
-    virInterfaceObjListFree(privconn->backupIfaces);
+    virObjectUnref(privconn->backupIfaces);
     privconn->transaction_running = false;
 
     ret = 0;
@@ -3910,7 +3910,7 @@ testInterfaceChangeRollback(virConnectPtr conn,
         goto cleanup;
     }
 
-    virInterfaceObjListFree(privconn->ifaces);
+    virObjectUnref(privconn->ifaces);
     privconn->ifaces = privconn->backupIfaces;
     privconn->backupIfaces = NULL;
 
-- 
2.13.6

    

John Ferlan

John Ferlan

tags

participants (1)