util: macmap: Convert to use GSList for storing macs instead of string lists

Since adding and removing is the main use case for the macmap module,
convert the code to a more efficient data structure.

The refactor also optimizes the loading from file where previously we'd
do a hash lookup + list lenght calculation for every entry.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

src/util/virmacmap.c

@@ -50,21 +50,18 @@ struct virMacMap {
 static virClassPtr virMacMapClass;
 
 
-static int
-virMacMapHashFree(void *payload,
-                  const char *name G_GNUC_UNUSED,
-                  void *opaque G_GNUC_UNUSED)
-{
-    g_strfreev(payload);
-    return 0;
-}
-
-
 static void
 virMacMapDispose(void *obj)
 {
     virMacMapPtr mgr = obj;
-    virHashForEach(mgr->macs, virMacMapHashFree, NULL);
+    GHashTableIter htitr;
+    void *value;
+
+    g_hash_table_iter_init(&htitr,  mgr->macs);
+
+    while (g_hash_table_iter_next(&htitr, NULL, &value))
+        g_slist_free_full(value, g_free);
+
     virHashFree(mgr->macs);
 }
 
@@ -80,48 +77,57 @@ static int virMacMapOnceInit(void)
 VIR_ONCE_GLOBAL_INIT(virMacMap);
 
 
-static int
+static void
 virMacMapAddLocked(virMacMapPtr mgr,
                    const char *domain,
                    const char *mac)
 {
-    char **macsList = NULL;
+    GSList *orig_list;
+    GSList *list;
+    GSList *next;
 
-    if ((macsList = virHashLookup(mgr->macs, domain)) &&
+    list = orig_list = g_hash_table_lookup(mgr->macs, domain);
-        virStringListHasString((const char**) macsList, mac)) {
+
-        return 0;
+    for (next = list; next; next = next->next) {
+        if (STREQ((const char *) next->data, mac))
+            return;
     }
 
-    if (virStringListAdd(&macsList, mac) < 0 ||
+    list = g_slist_append(list, g_strdup(mac));
-        virHashUpdateEntry(mgr->macs, domain, macsList) < 0)
-        return -1;
 
-    return 0;
+    if (list != orig_list)
+        g_hash_table_insert(mgr->macs, g_strdup(domain), list);
 }
 
 
-static int
+static void
 virMacMapRemoveLocked(virMacMapPtr mgr,
                       const char *domain,
                       const char *mac)
 {
-    char **macsList = NULL;
+    GSList *orig_list;
-    char **newMacsList = NULL;
+    GSList *list;
+    GSList *next;
 
-    if (!(macsList = virHashLookup(mgr->macs, domain)))
+    list = orig_list = g_hash_table_lookup(mgr->macs, domain);
-        return 0;
 
-    newMacsList = macsList;
+    if (!orig_list)
-    virStringListRemove(&newMacsList, mac);
+        return;
-    if (!newMacsList) {
+
-        virHashSteal(mgr->macs, domain);
+    for (next = list; next; next = next->next) {
-    } else {
+        if (STREQ((const char *) next->data, mac)) {
-        if (macsList != newMacsList &&
+            list = g_slist_remove_link(list, next);
-            virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0)
+            g_slist_free_full(next, g_free);
-            return -1;
+            break;
+        }
     }
 
-    return 0;
+    if (list != orig_list) {
+        if (list)
+            g_hash_table_insert(mgr->macs, g_strdup(domain), list);
+        else
+            g_hash_table_remove(mgr->macs, domain);
+    }
 }
 
 
@@ -162,6 +168,7 @@ virMacMapLoadFile(virMacMapPtr mgr,
         virJSONValuePtr macs;
         const char *domain;
         size_t j;
+        GSList *vals = NULL;
 
         if (!(domain = virJSONValueObjectGetString(tmp, "domain"))) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -175,13 +182,21 @@ virMacMapLoadFile(virMacMapPtr mgr,
             return -1;
         }
 
+        if (g_hash_table_contains(mgr->macs, domain)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("duplicate domain '%s'"), domain);
+            return -1;
+
+        }
+
         for (j = 0; j < virJSONValueArraySize(macs); j++) {
             virJSONValuePtr macJSON = virJSONValueArrayGet(macs, j);
-            const char *mac = virJSONValueGetString(macJSON);
 
-            if (virMacMapAddLocked(mgr, domain, mac) < 0)
+            vals = g_slist_prepend(vals, g_strdup(virJSONValueGetString(macJSON)));
-                return -1;
         }
+
+        vals = g_slist_reverse(vals);
+        g_hash_table_insert(mgr->macs, g_strdup(domain), vals);
     }
 
     return 0;
@@ -195,11 +210,11 @@ virMACMapHashDumper(void *payload,
 {
     g_autoptr(virJSONValue) obj = virJSONValueNewObject();
     g_autoptr(virJSONValue) arr = virJSONValueNewArray();
-    const char **macs = payload;
+    GSList *macs = payload;
-    size_t i;
+    GSList *next;
 
-    for (i = 0; macs[i]; i++) {
+    for (next = macs; next; next = next->next) {
-        virJSONValuePtr m = virJSONValueNewString(macs[i]);
+        virJSONValuePtr m = virJSONValueNewString((const char *) next->data);
 
         if (!m ||
             virJSONValueArrayAppend(arr, m) < 0) {
@@ -279,8 +294,8 @@ virMacMapNew(const char *file)
         return NULL;
 
     virObjectLock(mgr);
-    if (!(mgr->macs = virHashNew(NULL)))
+
-        goto error;
+    mgr->macs = virHashNew(NULL);
 
     if (file &&
         virMacMapLoadFile(mgr, file) < 0)
@@ -301,12 +316,10 @@ virMacMapAdd(virMacMapPtr mgr,
              const char *domain,
              const char *mac)
 {
-    int ret;
-
     virObjectLock(mgr);
-    ret = virMacMapAddLocked(mgr, domain, mac);
+    virMacMapAddLocked(mgr, domain, mac);
     virObjectUnlock(mgr);
-    return ret;
+    return 0;
 }
 
 
@@ -315,20 +328,19 @@ virMacMapRemove(virMacMapPtr mgr,
                 const char *domain,
                 const char *mac)
 {
-    int ret;
-
     virObjectLock(mgr);
-    ret = virMacMapRemoveLocked(mgr, domain, mac);
+    virMacMapRemoveLocked(mgr, domain, mac);
     virObjectUnlock(mgr);
-    return ret;
+    return 0;
 }
 
 
-const char *const *
+/* note that the returned pointer may be invalidated by other APIs in this module */
+GSList *
 virMacMapLookup(virMacMapPtr mgr,
                 const char *domain)
 {
-    const char *const *ret;
+    GSList *ret;
 
     virObjectLock(mgr);
     ret = virHashLookup(mgr->macs, domain);

src/util/virmacmap.h

@@ -20,6 +20,8 @@
 
 #pragma once
 
+#include "internal.h"
+
 typedef struct virMacMap virMacMap;
 typedef virMacMap *virMacMapPtr;
 
@@ -37,8 +39,8 @@ int virMacMapRemove(virMacMapPtr mgr,
                     const char *domain,
                     const char *mac);
 
-const char *const *virMacMapLookup(virMacMapPtr mgr,
+GSList *virMacMapLookup(virMacMapPtr mgr,
-                                   const char *domain);
+                        const char *domain);
 
 int virMacMapWriteFile(virMacMapPtr mgr,
                        const char *filename);

tests/virmacmaptest.c

@@ -36,7 +36,8 @@ testMACLookup(const void *opaque)
 {
     const struct testData *data = opaque;
     virMacMapPtr mgr = NULL;
-    const char * const * macs;
+    GSList *macs;
+    GSList *next;
     size_t i, j;
     char *file = NULL;
     int ret = -1;
@@ -48,26 +49,27 @@ testMACLookup(const void *opaque)
 
     macs = virMacMapLookup(mgr, data->domain);
 
-    for (i = 0; macs && macs[i]; i++) {
+    for (next = macs; next; next = next->next) {
         for (j = 0; data->macs && data->macs[j]; j++) {
-            if (STREQ(macs[i], data->macs[j]))
+            if (STREQ((const char *) next->data, data->macs[j]))
                 break;
         }
 
         if (!data->macs || !data->macs[j]) {
             fprintf(stderr,
-                    "Unexpected %s in the returned list of MACs\n", macs[i]);
+                    "Unexpected %s in the returned list of MACs\n",
+                    (const char *) next->data);
             goto cleanup;
         }
     }
 
     for (i = 0; data->macs && data->macs[i]; i++) {
-        for (j = 0; macs && macs[j]; j++) {
+        for (next = macs; next; next = next->next) {
-            if (STREQ(data->macs[i], macs[j]))
+            if (STREQ(data->macs[i], (const char *) next->data))
                 break;
         }
 
-        if (!macs || !macs[j]) {
+        if (!next) {
             fprintf(stderr,
                     "Expected %s in the returned list of MACs\n", data->macs[i]);
             goto cleanup;
@@ -87,7 +89,7 @@ testMACRemove(const void *opaque)
 {
     const struct testData *data = opaque;
     virMacMapPtr mgr = NULL;
-    const char * const * macs;
+    GSList *macs;
     size_t i;
     char *file = NULL;
     int ret = -1;
@@ -107,7 +109,8 @@ testMACRemove(const void *opaque)
 
     if ((macs = virMacMapLookup(mgr, data->domain))) {
         fprintf(stderr,
-                "Not removed all MACs for domain %s: %s\n", data->domain, macs[0]);
+                "Not removed all MACs for domain %s: %s\n",
+                data->domain, (const char *) macs->data);
         goto cleanup;
     }