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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list