diff options
author | Adrian Moreno <amorenoz@redhat.com> | 2022-03-23 12:56:13 +0100 |
---|---|---|
committer | Ilya Maximets <i.maximets@ovn.org> | 2022-03-30 20:03:34 +0200 |
commit | f7083afb854dde682899c22e117a4fd9cbc69442 (patch) | |
tree | 2cff41015486335c3c12e9cfac6f706021d461c2 | |
parent | 57c44fa35eafc3e7fd5a948aef5c2788ccc4991d (diff) | |
download | openvswitch-f7083afb854dde682899c22e117a4fd9cbc69442.tar.gz |
list: use multi-variable helpers for list loops.
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.
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
-rw-r--r-- | include/openvswitch/list.h | 73 | ||||
-rw-r--r-- | ofproto/bond.c | 2 | ||||
-rw-r--r-- | 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 405202fb6..54e06211a 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -1165,7 +1165,7 @@ insert_bal(struct ovs_list *bals, struct bond_slave *slave) break; } } - ovs_list_insert(&pos->bal_node, &slave->bal_node); + ovs_list_insert(pos ? &pos->bal_node : bals, &slave->bal_node); } /* Removes 'slave' 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..648e02a5e 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. */ + 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)) { |