On Wed, Mar 09, 2011 at 02:20:37PM +0100, Jiri Denemark wrote:

Calling most hash APIs is not safe from inside of an iterator callback. Exceptions are APIs that do not modify the hash table and removing current hash entry from virHashFroEach callback.

This patch make all APIs which are not safe fail instead of just relying on the callback being nice not calling any unsafe APIs. --- src/util/hash.c | 39 +++++++++++++++++++++++++++++++-------- 1 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/src/util/hash.c b/src/util/hash.c index 2a9a9cf..1a300f6 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -54,6 +54,10 @@ struct _virHashTable { struct _virHashEntry *table; int size; int 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; @@ -313,7 +317,7 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, char *new_name; bool found;

- if ((table == NULL) || (name == NULL)) + if ((table == NULL) || (name == NULL) || table->iterating) return (-1);

/* @@ -513,6 +517,8 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) for (entry = &(table->table[key]); entry != NULL; entry = entry->next) { if (table->keyEqual(entry->name, name)) { + if (table->iterating && table->current != entry) + return -1;

I think we should emit an error message to indicate why it failed !

if (table->dataFree && (entry->payload != NULL)) table->dataFree(entry->payload, entry->name); entry->payload = NULL; @@ -548,28 +554,35 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) * @data: opaque data to pass to the iterator * * Iterates over every element in the hash table, invoking the - * 'iter' callback. The callback is allowed to remove the element using - * virHashRemoveEntry but calling other virHash* functions is prohibited. + * 'iter' callback. The callback is allowed to remove the current element + * using virHashRemoveEntry but calling other virHash* functions is prohibited. * * Returns number of items iterated over upon completion, -1 on failure */ -int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { +int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) +{ int i, count = 0;

- if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating) return (-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; if (entry->valid) { + table->current = entry; iter(entry->payload, entry->name, data); + table->current = NULL; count++; } entry = next; } } + table->iterating = false; + return (count); }

@@ -590,9 +603,11 @@ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *data) { int i, count = 0;

- if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating) return (-1);

+ table->iterating = true; + table->current = NULL; for (i = 0 ; i < table->size ; i++) { virHashEntryPtr prev = NULL; virHashEntryPtr entry = &(table->table[i]); @@ -629,6 +644,8 @@ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *da } } } + table->iterating = false; + return (count); }

@@ -646,18 +663,24 @@ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *da void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data) { int i;

- if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating)

Same thing an error message must be provided

return (NULL);

+ table->iterating = true; + table->current = NULL; for (i = 0 ; i < table->size ; i++) { virHashEntryPtr entry = table->table + i; while (entry) { if (entry->valid) { - if (iter(entry->payload, entry->name, data)) + if (iter(entry->payload, entry->name, data)) { + table->iterating = false; return entry->payload; + } } entry = entry->next; } } + table->iterating = false; + return (NULL); }

I would really prefer those to be added if needed after the initial commit, thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/