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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list