diff mbox series

[ovs-dev,v4,06/15] hmap: use multi-variable helpers for hmap loops

Message ID 20220309161023.3347758-7-amorenoz@redhat.com
State Superseded
Headers show
Series Fix undefined behavior in loop macros | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/intel-ovs-compilation success test: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Adrián Moreno March 9, 2022, 4:10 p.m. UTC 
Rewrite hmap's loops using multi-variable helpers.

For SAFE loops, use the LONG version of the multi-variable macros to
keep backwards compatibility.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/openvswitch/hmap.h | 61 +++++++++++++++++++-------------------
 lib/ovs-numa.h             |  4 +--
 tests/test-hmap.c          |  9 ++++++
 3 files changed, 42 insertions(+), 32 deletions(-)

Comments

0-day Robot March 9, 2022, 5:29 p.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: Inappropriate bracing around statement
#77 FILE: include/openvswitch/hmap.h:182:
    HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)

ERROR: Inappropriate bracing around statement
#114 FILE: lib/ovs-numa.h:71:
    HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->cores)

ERROR: Inappropriate bracing around statement
#118 FILE: lib/ovs-numa.h:74:
    HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas)

ERROR: Use ovs_assert() in place of assert()
#129 FILE: tests/test-hmap.c:65:
    assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#137 FILE: tests/test-hmap.c:86:
        assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#146 FILE: tests/test-hmap.c:249:
                    assert(next == NULL);

ERROR: Use ovs_assert() in place of assert()
#148 FILE: tests/test-hmap.c:251:
                    assert(&next->node == hmap_next(&hmap, &e->node));

ERROR: Use ovs_assert() in place of assert()
#157 FILE: tests/test-hmap.c:269:
            assert(next == NULL);

ERROR: Use ovs_assert() in place of assert()
#158 FILE: tests/test-hmap.c:270:
            assert(e == NULL);

Lines checked: 165, Warnings: 0, Errors: 9


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

Thanks,
0-day Robot
Eelco Chaudron March 21, 2022, 10:32 a.m. UTC | #2 
On 9 Mar 2022, at 17:10, Adrian Moreno wrote:

> Rewrite hmap's loops using multi-variable helpers.
>
> For SAFE loops, use the LONG version of the multi-variable macros to
> keep backwards compatibility.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Changes look good to me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff mbox series

Patch

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index 4e001cc69..610ad9987 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -134,17 +134,17 @@  struct hmap_node *hmap_random_node(const struct hmap *);
  * without using 'break', NODE will be NULL.  This is true for all of the
  * HMAP_FOR_EACH_*() macros.
  */
-#define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)               \
-    for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
-         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))                \
-         || ((NODE = NULL), false);                                     \
-         ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),   \
-                          MEMBER))
-#define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)               \
-    for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
-         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))                \
-         || ((NODE = NULL), false);                                     \
-         ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)                     \
+    for (INIT_MULTIVAR(NODE, MEMBER, hmap_first_with_hash(HMAP, HASH),        \
+                       struct hmap_node);                                     \
+         CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);            \
+         UPDATE_MULTIVAR(NODE, hmap_next_with_hash(ITER_VAR(NODE))))
+
+#define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)                     \
+    for (INIT_MULTIVAR(NODE, MEMBER, hmap_first_in_bucket(HMAP, HASH),        \
+                       struct hmap_node);                                     \
+         CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);            \
+         UPDATE_MULTIVAR(NODE, hmap_next_in_bucket(ITER_VAR(NODE))))
 
 static inline struct hmap_node *hmap_first_with_hash(const struct hmap *,
                                                      size_t hash);
@@ -170,33 +170,34 @@  bool hmap_contains(const struct hmap *, const struct hmap_node *);
 /* Iterates through every node in HMAP. */
 #define HMAP_FOR_EACH(NODE, MEMBER, HMAP) \
     HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
-#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)                     \
-    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
-         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))                \
-         || ((NODE = NULL), false);                                     \
-         ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)                           \
+    for (INIT_MULTIVAR_EXP(NODE, MEMBER, hmap_first(HMAP), struct hmap_node,  \
+                           __VA_ARGS__);                                      \
+         CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);            \
+         UPDATE_MULTIVAR(NODE, hmap_next(HMAP, ITER_VAR(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 HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP) \
-    HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
-#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)          \
-    for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
-         ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))               \
-          || ((NODE = NULL), false)                                     \
-          ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
-          : 0);                                                         \
-         (NODE) = (NEXT))
+    HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)
+
+#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)                \
+    for (INIT_MULTIVAR_SAFE_LONG_EXP(NODE, NEXT, MEMBER, hmap_first(HMAP),    \
+                                      struct hmap_node, __VA_ARGS__);         \
+         CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,                     \
+                                      ITER_VAR(NODE) != NULL,                 \
+                            ITER_VAR(NEXT) = hmap_next(HMAP, ITER_VAR(NODE)), \
+                                      ITER_VAR(NEXT) != NULL);                \
+         UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
 /* Continues an iteration from just after NODE. */
 #define HMAP_FOR_EACH_CONTINUE(NODE, MEMBER, HMAP) \
     HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, (void) 0)
-#define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)            \
-    for (ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), \
-         __VA_ARGS__;                                                   \
-         (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))                \
-         || ((NODE = NULL), false);                                     \
-         ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_CONTINUE_INIT(NODE, MEMBER, HMAP, ...)                  \
+    for (INIT_MULTIVAR_EXP(NODE, MEMBER, hmap_next(HMAP, &(NODE)->MEMBER),    \
+                           struct hmap_node, __VA_ARGS__);                    \
+         CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);            \
+         UPDATE_MULTIVAR(NODE, hmap_next(HMAP, ITER_VAR(NODE))))
 
 static inline struct hmap_node *
 hmap_pop_helper__(struct hmap *hmap, size_t *bucket) {
diff --git a/lib/ovs-numa.h b/lib/ovs-numa.h
index ecc251a7f..83bd10cca 100644
--- a/lib/ovs-numa.h
+++ b/lib/ovs-numa.h
@@ -68,9 +68,9 @@  void ovs_numa_dump_destroy(struct ovs_numa_dump *);
 int ovs_numa_thread_setaffinity_core(unsigned core_id);
 
 #define FOR_EACH_CORE_ON_DUMP(ITER, DUMP)                    \
-    HMAP_FOR_EACH((ITER), hmap_node, &(DUMP)->cores)
+    HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->cores)
 
 #define FOR_EACH_NUMA_ON_DUMP(ITER, DUMP)                    \
-    HMAP_FOR_EACH((ITER), hmap_node, &(DUMP)->numas)
+    HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas)
 
 #endif /* ovs-numa.h */
diff --git a/tests/test-hmap.c b/tests/test-hmap.c
index 9259b0b3f..a40ac8953 100644
--- a/tests/test-hmap.c
+++ b/tests/test-hmap.c
@@ -62,6 +62,7 @@  check_hmap(struct hmap *hmap, const int values[], size_t n,
         hmap_values[i++] = e->value;
     }
     assert(i == n);
+    assert(e == NULL);
 
     memcpy(sort_values, values, sizeof *sort_values * n);
     qsort(sort_values, n, sizeof *sort_values, compare_ints);
@@ -82,6 +83,7 @@  check_hmap(struct hmap *hmap, const int values[], size_t n,
             count += e->value == values[i];
         }
         assert(count == 1);
+        assert(e == NULL);
     }
 
     /* Check counters. */
@@ -243,6 +245,11 @@  test_hmap_for_each_safe(hash_func *hash)
             i = 0;
             n_remaining = n;
             HMAP_FOR_EACH_SAFE (e, next, node, &hmap) {
+                if (hmap_next(&hmap, &e->node) == NULL) {
+                    assert(next == NULL);
+                } else {
+                    assert(&next->node == hmap_next(&hmap, &e->node));
+                }
                 assert(i < n);
                 if (pattern & (1ul << e->value)) {
                     size_t j;
@@ -259,6 +266,8 @@  test_hmap_for_each_safe(hash_func *hash)
                 i++;
             }
             assert(i == n);
+            assert(next == NULL);
+            assert(e == NULL);
 
             for (i = 0; i < n; i++) {
                 if (pattern & (1ul << i)) {