util: don't check for parallel iteration in hash-related functions

This is the responsability of the caller to apply the correct lock
before using these functions. Moreover, the use of a simple boolean
was still racy: two threads may check the boolean and "lock" it
simultaneously.

Users of functions from src/util/virhash.c have to be checked for
correctness. Lookups and iteration should hold a RO
lock. Modifications should hold a RW lock.

Most important uses seem to be covered. Callers have now a greater
responsability, notably the ability to execute some operations while
iterating were reliably forbidden before are now accepted.

Signed-off-by: Vincent Bernat <vincent@bernat.im>

diff --git a/src/util/virhash.c b/src/util/virhash.c
index 0ffbfcce2c640ed459152bf2ce03dc15d7e455ea..475c2b0281b277578b74b598c91337680022b255 100644 (file)
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -41,12 +41,6 @@ VIR_LOG_INIT("util.hash");
 
 /* #define DEBUG_GROW */
 
-#define virHashIterationError(ret) \
-    do { \
-        VIR_ERROR(_("Hash operation not allowed during iteration")); \
-        return ret; \
-    } while (0)
-
 /*
  * A single entry in the hash table
  */
@@ -66,10 +60,6 @@ struct _virHashTable {
     uint32_t seed;
     size_t size;
     size_t nbElems;
-    /* True iff we are iterating over hash entries. */
-    bool iterating;
-    /* Pointer to the current entry during iteration. */
-    virHashEntryPtr current;
     virHashDataFree dataFree;
     virHashKeyCode keyCode;
     virHashKeyEqual keyEqual;
@@ -339,9 +329,6 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name,
     if ((table == NULL) || (name == NULL))
         return -1;
 
-    if (table->iterating)
-        virHashIterationError(-1);
-
     key = virHashComputeKey(table, name);
 
     /* Check for duplicate entry */
@@ -551,9 +538,6 @@ virHashRemoveEntry(virHashTablePtr table, const void *name)
     nextptr = table->table + virHashComputeKey(table, name);
     for (entry = *nextptr; entry; entry = entry->next) {
         if (table->keyEqual(entry->name, name)) {
-            if (table->iterating && table->current != entry)
-                virHashIterationError(-1);
-
             if (table->dataFree)
                 table->dataFree(entry->payload, entry->name);
             if (table->keyFree)
@@ -593,18 +577,11 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
     if (table == NULL || iter == NULL)
         return -1;
 
-    if (table->iterating)
-        virHashIterationError(-1);
-
-    table->iterating = true;
-    table->current = NULL;
     for (i = 0; i < table->size; i++) {
         virHashEntryPtr entry = table->table[i];
         while (entry) {
             virHashEntryPtr next = entry->next;
-            table->current = entry;
             ret = iter(entry->payload, entry->name, data);
-            table->current = NULL;
 
             if (ret < 0)
                 goto cleanup;
@@ -615,7 +592,6 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
 
     ret = 0;
  cleanup:
-    table->iterating = false;
     return ret;
 }
 
@@ -643,11 +619,6 @@ virHashRemoveSet(virHashTablePtr table,
     if (table == NULL || iter == NULL)
         return -1;
 
-    if (table->iterating)
-        virHashIterationError(-1);
-
-    table->iterating = true;
-    table->current = NULL;
     for (i = 0; i < table->size; i++) {
         virHashEntryPtr *nextptr = table->table + i;
 
@@ -667,7 +638,6 @@ virHashRemoveSet(virHashTablePtr table,
             }
         }
     }
-    table->iterating = false;
 
     return count;
 }
@@ -723,23 +693,16 @@ void *virHashSearch(const virHashTable *ctable,
     if (table == NULL || iter == NULL)
         return NULL;
 
-    if (table->iterating)
-        virHashIterationError(NULL);
-
-    table->iterating = true;
-    table->current = NULL;
     for (i = 0; i < table->size; i++) {
         virHashEntryPtr entry;
         for (entry = table->table[i]; entry; entry = entry->next) {
             if (iter(entry->payload, entry->name, data)) {
-                table->iterating = false;
                 if (name)
                     *name = table->keyCopy(entry->name);
                 return entry->payload;
             }
         }
     }
-    table->iterating = false;
 
     return NULL;
 }

diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index 3b85b62c301d4f56ab1d6f0a006bbacba59b40a5..e9c03c1afbbc9baeb0b1e015e41d5bc01060783a 100644 (file)
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -221,32 +221,6 @@ testHashRemoveForEachAll(void *payload ATTRIBUTE_UNUSED,
 }
 
 
-const int testHashCountRemoveForEachForbidden = ARRAY_CARDINALITY(uuids);
-
-static int
-testHashRemoveForEachForbidden(void *payload ATTRIBUTE_UNUSED,
-                               const void *name,
-                               void *data)
-{
-    virHashTablePtr hash = data;
-    size_t i;
-
-    for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
-        if (STREQ(uuids_subset[i], name)) {
-            int next = (i + 1) % ARRAY_CARDINALITY(uuids_subset);
-
-            if (virHashRemoveEntry(hash, uuids_subset[next]) == 0) {
-                VIR_TEST_VERBOSE(
-                        "\nentry \"%s\" should not be allowed to be removed",
-                        uuids_subset[next]);
-            }
-            break;
-        }
-    }
-    return 0;
-}
-
-
 static int
 testHashRemoveForEach(const void *data)
 {
@@ -303,61 +277,6 @@ testHashSteal(const void *data ATTRIBUTE_UNUSED)
 }
 
 
-static int
-testHashIter(void *payload ATTRIBUTE_UNUSED,
-             const void *name ATTRIBUTE_UNUSED,
-             void *data ATTRIBUTE_UNUSED)
-{
-    return 0;
-}
-
-static int
-testHashForEachIter(void *payload ATTRIBUTE_UNUSED,
-                    const void *name ATTRIBUTE_UNUSED,
-                    void *data)
-{
-    virHashTablePtr hash = data;
-
-    if (virHashAddEntry(hash, uuids_new[0], NULL) == 0)
-        VIR_TEST_VERBOSE("\nadding entries in ForEach should be forbidden");
-
-    if (virHashUpdateEntry(hash, uuids_new[0], NULL) == 0)
-        VIR_TEST_VERBOSE("\nupdating entries in ForEach should be forbidden");
-
-    if (virHashSteal(hash, uuids_new[0]) != NULL)
-        VIR_TEST_VERBOSE("\nstealing entries in ForEach should be forbidden");
-
-    if (virHashSteal(hash, uuids_new[0]) != NULL)
-        VIR_TEST_VERBOSE("\nstealing entries in ForEach should be forbidden");
-
-    if (virHashForEach(hash, testHashIter, NULL) >= 0)
-        VIR_TEST_VERBOSE("\niterating through hash in ForEach"
-                " should be forbidden");
-    return 0;
-}
-
-static int
-testHashForEach(const void *data ATTRIBUTE_UNUSED)
-{
-    virHashTablePtr hash;
-    int ret = -1;
-
-    if (!(hash = testHashInit(0)))
-        return -1;
-
-    if (virHashForEach(hash, testHashForEachIter, hash)) {
-        VIR_TEST_VERBOSE("\nvirHashForEach didn't go through all entries");
-        goto cleanup;
-    }
-
-    ret = 0;
-
- cleanup:
-    virHashFree(hash);
-    return ret;
-}
-
-
 static int
 testHashRemoveSetIter(const void *payload ATTRIBUTE_UNUSED,
                       const void *name,
@@ -628,9 +547,7 @@ mymain(void)
     DO_TEST("Remove", Remove);
     DO_TEST_DATA("Remove in ForEach", RemoveForEach, Some);
     DO_TEST_DATA("Remove in ForEach", RemoveForEach, All);
-    DO_TEST_DATA("Remove in ForEach", RemoveForEach, Forbidden);
     DO_TEST("Steal", Steal);
-    DO_TEST("Forbidden ops in ForEach", ForEach);
     DO_TEST("RemoveSet", RemoveSet);
     DO_TEST("Search", Search);
     DO_TEST("GetItems", GetItems);