From 4d7384eb9ddef2008cb0cc165eb808f74bc83d6b Mon Sep 17 00:00:00 2001 From: Vincent Bernat Date: Tue, 10 Apr 2018 08:27:15 +0200 Subject: [PATCH] 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 --- src/util/virhash.c | 37 -------------------- tests/virhashtest.c | 83 --------------------------------------------- 2 files changed, 120 deletions(-) diff --git a/src/util/virhash.c b/src/util/virhash.c index 0ffbfcce2c6..475c2b0281b 100644 --- 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 3b85b62c301..e9c03c1afbb 100644 --- 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);