❦ 4 avril 2018 17:00 +0200, Vincent Bernat <bernat@xxxxxxxx> :
>>> You can do that locally, but as a patch it's very unlikely to be
>>> accepted upstream because we've introduced RW locks to be able to access
>>> domain list from multiple threads.
>>
>> Looking a bit more, the whole "iterating" bit is currently
>> unsafe. First, it is racy since it's "check then take". Two threads may
>> check the value is false and start iterating both. Moreover, at some
>> places, it is not set, for example in virHashAddOrUpdateEntry(). So, it
>> would be possible to call this function and during the update to iterate
>> over the hash. This seems to be prevented only by callers using RW
>> lock. So, it seems we can just remove this "iterating" bool and just
>> check all users of these functions are using the appropriate lock.
>
> My bad, I didn't see the use of table->current...
It seems it can also go away if caller correctly use a RW lock. I am
currently testing the attached patch on a few servers (patch for master,
but applied on our 3.6 version). After checking many locations, I came
to conclusion that locks are used correctly.
>From 7bf32825325b124edce58687a8cd2ceff13328b9 Mon Sep 17 00:00:00 2001
From: Vincent Bernat <vincent@xxxxxxxxx>
Date: Wed, 4 Apr 2018 16:25:37 +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 these functions have to be checked for correctness:
- virHashSearch (RO)
- virHashRemoveSet (RW)
- virHashForEach (RO)
- virHashRemoveEntry (RW)
- virHashAddOrUpdateEntry (RW)
- virHashEqual (RO)
- virHashRemoveAll (RW)
- virHashAddEntry (RW)
- virHashUpdateEntry (RW?)
There are some fishy uses, but 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.
---
src/util/virhash.c | 37 ----------------------
tests/virhashtest.c | 75 ---------------------------------------------
2 files changed, 112 deletions(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 0ffbfcce2c64..475c2b0281b2 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 3b85b62c301d..a013bc716943 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)
{
@@ -311,53 +285,6 @@ testHashIter(void *payload 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 +555,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);
--
2.17.0
--
Use library functions.
- The Elements of Programming Style (Kernighan & Plauger)
_______________________________________________
libvirt-users mailing list
libvirt-users@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvirt-users
