Re: [ovs-dev] [PATCH v4 04/15] list: use multi-variable helpers for list loops

Adrian Moreno Wed, 23 Mar 2022 00:21:54 -0700


On 3/18/22 14:32, Eelco Chaudron wrote:



On 9 Mar 2022, at 17:10, Adrian Moreno wrote:

Use multi-variable iteration helpers to rewrite non-safe loops.

There is an important behavior change compared with the previous
implementation: When the loop ends normally (i.e: not via "break;"), the
object pointer provided by the user is NULL. This is safer because it's
not guaranteed that it would end up pointing a valid address.

For pop iterator, set the variable to NULL when the loop ends.

Clang-analyzer has successfully picked the potential null-pointer
dereference on the code that triggered this change (bond.c) and nothing
else has been detected.

For _SAFE loops, use the LONG version for backwards compatibility.

Signed-off-by: Adrian Moreno <[email protected]>


Some nits below, rest looks good:

Acked-by: Eelco Chaudron <[email protected]>

---
  include/openvswitch/list.h | 73 ++++++++++++++++++++++----------------
  ofproto/bond.c             |  2 +-
  tests/test-list.c          | 14 ++++++--
  3 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h
index 8ad5eeb32..bbd2edbd0 100644
--- a/include/openvswitch/list.h
+++ b/include/openvswitch/list.h
@@ -72,37 +72,48 @@ static inline bool ovs_list_is_empty(const struct ovs_list 
*);
  static inline bool ovs_list_is_singleton(const struct ovs_list *);
  static inline bool ovs_list_is_short(const struct ovs_list *);

-#define LIST_FOR_EACH(ITER, MEMBER, LIST)                               \
-    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);                    \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST)                      \
-    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER);             \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST)                       \
-    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                    \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_REVERSE_SAFE(ITER, PREV, MEMBER, LIST)        \
-    for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);                \
-         (&(ITER)->MEMBER != (LIST)                                 \
-          ? INIT_CONTAINER(PREV, (ITER)->MEMBER.prev, MEMBER), 1    \
-          : 0);                                                     \
-         (ITER) = (PREV))
-#define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST)              \
-    for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER);           \
-         &(ITER)->MEMBER != (LIST);                                     \
-         ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)               \
-    for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);               \
-         (&(ITER)->MEMBER != (LIST)                                \
-          ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1   \
-          : 0);                                                    \
-         (ITER) = (NEXT))
-#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST)                      \
-    while (!ovs_list_is_empty(LIST)                                    \
-           && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1))
+#define LIST_FOR_EACH(VAR, MEMBER, LIST)                                      \
+    for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list);           \
+         CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));            \
+         UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next))
+
+#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST)                             \
+    for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list);       \
+         CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));            \
+         UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->next))
+
+#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST)                              \
+    for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list);           \
+         CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));            \
+         UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev))
+
+#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST)                     \
+    for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list);       \
+         CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));            \
+         UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev))
+
+#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST)                   \
+    for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev,             \
+                                 struct ovs_list);                            \
+         CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER,                      \
+                                      ITER_VAR(VAR) != (LIST),                \
+                                      ITER_VAR(PREV) = ITER_VAR(VAR)->prev,   \
+                                      ITER_VAR(PREV) != (LIST));              \
+         UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV))
+
+#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST)                           \
+    for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next,             \
+                                 struct ovs_list);                            \
+         CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER,                      \
+                                      ITER_VAR(VAR) != (LIST),                \
+                                      ITER_VAR(NEXT) = ITER_VAR(VAR)->next,   \
+                                      ITER_VAR(NEXT) != (LIST));              \
+         UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT))
+
+#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST)                                 \
+    while (!ovs_list_is_empty(LIST) ?                                         \
+           (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1) :      \
+           (ITER = NULL, 0))
  
  /* Inline implementations. */

diff --git a/ofproto/bond.c b/ofproto/bond.c
index cdfdf0b9d..7e30fd8bd 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -1258,7 +1258,7 @@ insert_bal(struct ovs_list *bals, struct bond_member 
*member)
              break;
          }
      }
-    ovs_list_insert(&pos->bal_node, &member->bal_node);
+    ovs_list_insert(pos ? &pos->bal_node: bals, &member->bal_node);


Space before :


Ugh, I forgot this one. It was also pointed out by Dumitru I believe.

  }

  /* Removes 'member' from its current list and then inserts it into 'bals' so
diff --git a/tests/test-list.c b/tests/test-list.c
index 6f1fb059b..7c02ea40f 100644
--- a/tests/test-list.c
+++ b/tests/test-list.c
@@ -61,7 +61,7 @@ check_list(struct ovs_list *list, const int values[], size_t 
n)
          assert(e->value == values[i]);
          i++;
      }
-    assert(&e->node == list);
+    assert(e == NULL);
      assert(i == n);

      i = 0;
@@ -70,7 +70,7 @@ check_list(struct ovs_list *list, const int values[], size_t 
n)
          assert(e->value == values[n - i - 1]);
          i++;
      }
-    assert(&e->node == list);
+    assert(e == NULL);
      assert(i == n);

      assert(ovs_list_is_empty(list) == !n);
@@ -135,6 +135,13 @@ test_list_for_each_safe(void)
              values_idx = 0;
              n_remaining = n;
              LIST_FOR_EACH_SAFE (e, next, node, &list) {
+                /*next is valid as long as it's not pointing to &list*/


Capital N for next, and ending dot.


I'm referring to the "next" variable so maybe just quote it?

+                if (&e->node == list.prev) {
+                    assert(next == NULL);
+                } else {
+                    assert(&next->node == e->node.next);
+                }
+
                  assert(i < n);
                  if (pattern & (1ul << i)) {
                      ovs_list_remove(&e->node);
@@ -148,7 +155,8 @@ test_list_for_each_safe(void)
                  i++;
              }
              assert(i == n);
-            assert(&e->node == &list);
+            assert(e == NULL);
+            assert(next == NULL);

              for (i = 0; i < n; i++) {
                  if (pattern & (1ul << i)) {
--
2.34.1

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


--
Adrián Moreno

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 04/15] list: use multi-variable helpers for list loops

Reply via email to