[libvirt] [PATCH v4 2/7] util: Refactor virHashForEach so it returns as soon as an iterator fails

[Michal Privoznik]

On 12.02.2016 11:08, Erik Skultety wrote:
> The method will now return 0 on success and -1 on error, rather than number of
> items which it iterated over before it returned back to the caller. Since the
> only place where we actually check the number of elements iterated is in
> virhashtest, return value of 0 and -1 can be a pretty accurate hint that it
> iterated over all the items. However, if we really want to know the number of
> items iterated over (like virhashtest does), a counter has to be provided
> through opaque data to each iterator call. This patch adjusts return value of
> virHashForEach, refactors the body, so it returns as soon as one of the
> iterators fail and adjusts virhashtest to reflect these changes.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/util/virhash.c  | 23 +++++++++++++++--------
>  src/util/virhash.h  |  2 +-
>  tests/virhashtest.c | 35 ++++++++++++++---------------------
>  3 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/src/util/virhash.c b/src/util/virhash.c
> index d6f208e..a8e0edf 100644
> --- a/src/util/virhash.c
> +++ b/src/util/virhash.c
> @@ -579,13 +579,16 @@ virHashRemoveEntry(virHashTablePtr table, const void *name)
>   * Iterates over every element in the hash table, invoking the
>   * 'iter' callback. The callback is allowed to remove the current element
>   * using virHashRemoveEntry but calling other virHash* functions is prohibited.
> + * If @iter fails and returns a negative value, the evaluation is stopped and -1
> + * is returned.
>   *
> - * Returns number of items iterated over upon completion, -1 on failure
> + * Returns 0 on success or -1 on failure.
>   */
> -ssize_t
> +int
>  virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
>  {
> -    size_t i, count = 0;
> +    size_t i;
> +    int ret = -1;
>  
>      if (table == NULL || iter == NULL)
>          return -1;
> @@ -599,20 +602,24 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
>          virHashEntryPtr entry = table->table[i];
>          while (entry) {
>              virHashEntryPtr next = entry->next;
> -
>              table->current = entry;
> -            iter(entry->payload, entry->name, data);
> +            ret = iter(entry->payload, entry->name, data);
>              table->current = NULL;
>  
> -            count++;
> +            if (ret < 0)
> +                goto cleanup;
> +
>              entry = next;
>          }
>      }
> -    table->iterating = false;
>  
> -    return count;
> +    ret = 0;
> + cleanup:
> +    table->iterating = false;
> +    return ret;
>  }
>  
> +
>  /**
>   * virHashRemoveSet
>   * @table: the hash table to process
> diff --git a/src/util/virhash.h b/src/util/virhash.h
> index ed6bba3..61c172b 100644
> --- a/src/util/virhash.h
> +++ b/src/util/virhash.h
> @@ -191,7 +191,7 @@ bool virHashEqual(const virHashTable *table1,
>  /*
>   * Iterators
>   */
> -ssize_t virHashForEach(virHashTablePtr table, virHashIterator iter, void *data);
> +int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data);
>  ssize_t virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *data);
>  void *virHashSearch(const virHashTable *table, virHashSearcher iter,
>                      const void *data);
> diff --git a/tests/virhashtest.c b/tests/virhashtest.c
> index 323c68e..e538ea4 100644
> --- a/tests/virhashtest.c
> +++ b/tests/virhashtest.c
> @@ -61,24 +61,27 @@ testHashCheckForEachCount(void *payload ATTRIBUTE_UNUSED,
>                            const void *name ATTRIBUTE_UNUSED,
>                            void *data ATTRIBUTE_UNUSED)
>  {
> +    ssize_t *count = data;
> +    *count += 1;
>      return 0;
>  }
>  
>  static int
> -testHashCheckCount(virHashTablePtr hash, size_t count)
> +testHashCheckCount(virHashTablePtr hash, int count)
>  {
> -    ssize_t iter_count = 0;
> +    int iter_count = 0;

Why are you changing the type if the callback expects ssize_t anyway?

>  
>      if (virHashSize(hash) != count) {
>          VIR_TEST_VERBOSE("\nhash contains %zu instead of %zu elements\n",
> -                  (size_t)virHashSize(hash), count);
> +                  (size_t)virHashSize(hash), (size_t)count);

This makes not much sense. We are in the control of formatting string, so why the typecasting?

>          return -1;
>      }
>  
> -    iter_count = virHashForEach(hash, testHashCheckForEachCount, NULL);
> +    virHashForEach(hash, testHashCheckForEachCount, &iter_count);
>      if (count != iter_count) {
> -        VIR_TEST_VERBOSE("\nhash claims to have %zu elements but iteration finds %zu\n",
> -                  count, (size_t)iter_count);
> +        VIR_TEST_VERBOSE("\nhash claims to have %zu elements but iteration"
> +                         "finds %zu\n",
> +                  (size_t)count, (size_t)iter_count);
>          return -1;
>      }
>  

ACK with this squashed in:

diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index e538ea4..ed9ad46 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -67,21 +67,20 @@ testHashCheckForEachCount(void *payload ATTRIBUTE_UNUSED,
 }
 
 static int
-testHashCheckCount(virHashTablePtr hash, int count)
+testHashCheckCount(virHashTablePtr hash, size_t count)
 {
-    int iter_count = 0;
+    ssize_t iter_count = 0;
 
     if (virHashSize(hash) != count) {
-        VIR_TEST_VERBOSE("\nhash contains %zu instead of %zu elements\n",
-                  (size_t)virHashSize(hash), (size_t)count);
+        VIR_TEST_VERBOSE("\nhash contains %zd instead of %zu elements\n",
+                         virHashSize(hash), count);
         return -1;
     }
 
     virHashForEach(hash, testHashCheckForEachCount, &iter_count);
     if (count != iter_count) {
-        VIR_TEST_VERBOSE("\nhash claims to have %zu elements but iteration"
-                         "finds %zu\n",
-                  (size_t)count, (size_t)iter_count);
+        VIR_TEST_VERBOSE("\nhash claims to have %zd elements but iteration"
+                         "finds %zd\n", count, iter_count);
         return -1;
     }


Michal