On 9 Mar 2022, at 17:10, Adrian Moreno wrote:
> Using SHORT version of the *_SAFE loops makes the code cleaner and less
> error prone. So, use the SHORT version and remove the extra variable
> when possible for HINDEX_*_SAFE.
>
> In order to be able to use both long and short versions without changing
> the name of the macro for all the clients, overload the existing name
> and select the appropriate version depending on the number of arguments.
>
> Acked-by: Dumitru Ceara <[email protected]>
> Signed-off-by: Adrian Moreno <[email protected]>
Changes look good to me! Two small nits below.
Acked-by: Eelco Chaudron <[email protected]>
> ---
> lib/conntrack.c | 4 ++--
> lib/hindex.h | 48 ++++++++++++++++++++++++++++++++++++---------
> tests/test-hindex.c | 31 +++++++++++++++++++++++++++++
> 3 files changed, 72 insertions(+), 11 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 4e2eb2932..08da4ddf7 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2896,8 +2896,8 @@ expectation_clean(struct conntrack *ct, const struct
> conn_key *parent_key)
> {
> ovs_rwlock_wrlock(&ct->resources_lock);
>
> - struct alg_exp_node *node, *next;
> - HINDEX_FOR_EACH_WITH_HASH_SAFE (node, next, node_ref,
> + struct alg_exp_node *node;
> + HINDEX_FOR_EACH_WITH_HASH_SAFE (node, node_ref,
> conn_key_hash(parent_key,
> ct->hash_basis),
> &ct->alg_expectation_refs) {
> if (!conn_key_cmp(&node->parent_key, parent_key)) {
> diff --git a/lib/hindex.h b/lib/hindex.h
> index f7a30d511..6443c6e9e 100644
> --- a/lib/hindex.h
> +++ b/lib/hindex.h
> @@ -135,16 +135,32 @@ void hindex_remove(struct hindex *, struct hindex_node
> *);
>
> /* Safe when NODE may be freed (not needed when NODE may be removed from the
> * hash map but its members remain accessible and intact). */
> -#define HINDEX_FOR_EACH_WITH_HASH_SAFE(NODE, NEXT, MEMBER, HASH, HINDEX) \
> - for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
> - hindex_node_with_hash(HINDEX, HASH), \
> - struct hindex_node); \
> - CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
> - ITER_VAR(NODE) != NULL, \
> - ITER_VAR(NEXT) = ITER_VAR(NODE)->s, \
> - ITER_VAR(NEXT) != NULL); \
> +#define HINDEX_FOR_EACH_WITH_HASH_SAFE_LONG(NODE, NEXT, MEMBER, HASH,
> HINDEX) \
> + for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,
> \
> + hindex_node_with_hash(HINDEX, HASH),
> \
> + struct hindex_node);
> \
> + CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,
> \
> + ITER_VAR(NODE) != NULL,
> \
> + ITER_VAR(NEXT) = ITER_VAR(NODE)->s,
> \
> + ITER_VAR(NEXT) != NULL);
> \
> UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
>
> +/* Short version of HINDEX_FOR_EACH_WITH_HASH_SAFE */
Add dot at the end.
> +#define HINDEX_FOR_EACH_WITH_HASH_SAFE_SHORT(NODE, MEMBER, HASH, HINDEX)
> \
> + for (INIT_MULTIVAR_SAFE_SHORT(NODE, MEMBER,
> \
> + hindex_node_with_hash(HINDEX, HASH),
> \
> + struct hindex_node);
> \
> + CONDITION_MULTIVAR_SAFE_SHORT(NODE, MEMBER,
> \
> + ITER_VAR(NODE) != NULL,
> \
> + ITER_NEXT_VAR(NODE) = ITER_VAR(NODE)->s);
> \
> + UPDATE_MULTIVAR_SAFE_SHORT(NODE))
> +
> +#define HINDEX_FOR_EACH_WITH_HASH_SAFE(...)
> \
> + OVERLOAD_SAFE_MACRO(HINDEX_FOR_EACH_WITH_HASH_SAFE_LONG,
> \
> + HINDEX_FOR_EACH_WITH_HASH_SAFE_SHORT,
> \
> + 5, __VA_ARGS__)
> +
> +
> /* Returns the head node in 'hindex' with the given 'hash', or a null pointer
> * if no nodes have that hash value. */
> static inline struct hindex_node *
> @@ -169,7 +185,7 @@ hindex_node_with_hash(const struct hindex *hindex, size_t
> hash)
>
> /* Safe when NODE may be freed (not needed when NODE may be removed from the
> * hash index but its members remain accessible and intact). */
> -#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)
> \
> +#define HINDEX_FOR_EACH_SAFE_LONG(NODE, NEXT, MEMBER, HINDEX)
> \
> for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, hindex_first(HINDEX),
> \
> struct hindex_node);
> \
> CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,
> \
> @@ -178,6 +194,20 @@ hindex_node_with_hash(const struct hindex *hindex,
> size_t hash)
> ITER_VAR(NEXT) != NULL);
> \
> UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
>
> +/* Short version of HINDEX_FOR_EACH_SAFE */
Add dot at the end.
> +#define HINDEX_FOR_EACH_SAFE_SHORT(NODE, MEMBER, HINDEX)
> \
> + for (INIT_MULTIVAR_SAFE_SHORT(NODE, MEMBER, hindex_first(HINDEX),
> \
> + struct hindex_node);
> \
> + CONDITION_MULTIVAR_SAFE_SHORT(NODE, MEMBER,
> \
> + ITER_VAR(NODE) != NULL,
> \
> + ITER_NEXT_VAR(NODE) = hindex_next(HINDEX, ITER_VAR(NODE)));
> \
> + UPDATE_MULTIVAR_SAFE_SHORT(NODE))
> +
> +#define HINDEX_FOR_EACH_SAFE(...)
> \
> + OVERLOAD_SAFE_MACRO(HINDEX_FOR_EACH_SAFE_LONG,
> \
> + HINDEX_FOR_EACH_SAFE_SHORT,
> \
> + 4, __VA_ARGS__)
> +
> struct hindex_node *hindex_first(const struct hindex *);
> struct hindex_node *hindex_next(const struct hindex *,
> const struct hindex_node *);
> diff --git a/tests/test-hindex.c b/tests/test-hindex.c
> index 95e49284e..cc2b1b8bd 100644
> --- a/tests/test-hindex.c
> +++ b/tests/test-hindex.c
> @@ -288,6 +288,37 @@ test_hindex_for_each_safe(hash_func *hash)
> assert(i == n);
> assert(next == NULL);
>
> + for (i = 0; i < n; i++) {
> + if (pattern & (1ul << i)) {
> + n_remaining++;
> + }
> + }
> + assert(n == n_remaining);
> + hindex_destroy(&hindex);
> +
> + /* Test short version (without the next variable). */
> + make_hindex(&hindex, elements, values, n, hash);
> +
> + i = 0;
> + n_remaining = n;
> + HINDEX_FOR_EACH_SAFE (e, node, &hindex) {
> + assert(i < n);
> + if (pattern & (1ul << e->value)) {
> + size_t j;
> + hindex_remove(&hindex, &e->node);
> + for (j = 0; ; j++) {
> + assert(j < n_remaining);
> + if (values[j] == e->value) {
> + values[j] = values[--n_remaining];
> + break;
> + }
> + }
> + }
> + check_hindex(&hindex, values, n_remaining, hash);
> + i++;
> + }
> + assert(i == n);
> +
> for (i = 0; i < n; i++) {
> if (pattern & (1ul << i)) {
> n_remaining++;
> --
> 2.34.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
