diff mbox series

[ovs-dev,v2,11/16] hindex: remove the next variable in safe loops

Message ID 20220214101359.3846911-12-amorenoz@redhat.com
State Changes Requested
Headers show
Series Fix undefined behavior in loop macros | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Adrián Moreno Feb. 14, 2022, 10:13 a.m. UTC
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.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/conntrack.c     |  4 ++--
 lib/hindex.h        | 46 +++++++++++++++++++++++++++++++++++++--------
 tests/test-hindex.c | 31 ++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 10 deletions(-)

Comments

0-day Robot Feb. 14, 2022, 11:59 a.m. UTC | #1
Bleep bloop.  Greetings Adrian Moreno, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Use ovs_assert() in place of assert()
#122 FILE: tests/test-hindex.c:296:
            assert(n == n_remaining);

ERROR: Use ovs_assert() in place of assert()
#131 FILE: tests/test-hindex.c:305:
                assert(i < n);

ERROR: Use ovs_assert() in place of assert()
#136 FILE: tests/test-hindex.c:310:
                        assert(j < n_remaining);

ERROR: Use ovs_assert() in place of assert()
#146 FILE: tests/test-hindex.c:320:
            assert(i == n);

Lines checked: 154, Warnings: 0, Errors: 4


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 9c96b69e4..2f0777102 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2857,8 +2857,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 c20752c25..8f537069c 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -134,15 +134,31 @@  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));       \
-         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));         \
+         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 */
+#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));             \
+         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_GET_SAFE_MACRO_WITH_HASH(_1, _2, _3, _4, _5, NAME, ...) NAME
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE(...)                                   \
+    HINDEX_GET_SAFE_MACRO_WITH_HASH(__VA_ARGS__,                              \
+                        HINDEX_FOR_EACH_WITH_HASH_SAFE_LONG,                  \
+                        HINDEX_FOR_EACH_WITH_HASH_SAFE_SHORT)(__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 *
@@ -167,7 +183,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));   \
          CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,                     \
                                       ITER_VAR(NODE) != NULL,                 \
@@ -175,6 +191,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 */
+#define HINDEX_FOR_EACH_SAFE_SHORT(NODE, MEMBER, HINDEX)                      \
+    for (INIT_MULTIVAR_SAFE_SHORT(NODE, MEMBER, hindex_first(HINDEX));        \
+         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_GET_SAFE_MACRO(_1, _2, _3, _4, NAME, ...) NAME
+#define HINDEX_FOR_EACH_SAFE(...)                                   \
+    HINDEX_GET_SAFE_MACRO(__VA_ARGS__,                              \
+                          HINDEX_FOR_EACH_SAFE_LONG,                \
+                          HINDEX_FOR_EACH_SAFE_SHORT)(__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++;