On 2/16/22 15:27, 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. > > 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 <amore...@redhat.com> > ---
A few tiny nits below, otherwise: Acked-by: Dumitru Ceara <dce...@redhat.com> > include/openvswitch/list.h | 71 ++++++++++++++++++++++---------------- > ofproto/bond.c | 2 +- > tests/test-list.c | 15 ++++++-- > 3 files changed, 54 insertions(+), 34 deletions(-) > > diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h > index 8ad5eeb32..8673bab93 100644 > --- a/include/openvswitch/list.h > +++ b/include/openvswitch/list.h > @@ -72,36 +72,47 @@ 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) \ > +#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) = 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) = 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) = ITER_VAR(VAR)->prev))) > + > + Nit: extra new line. > +#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) = 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)) > > /* Inline implementations. */ > diff --git a/ofproto/bond.c b/ofproto/bond.c > index cdfdf0b9d..6e95dfdff 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); Nit: missing whitespace before '?'. > } > > /* 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..efed5c82c 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); > @@ -144,11 +151,13 @@ test_list_for_each_safe(void) > } else { > values_idx++; > } > + Unrelated? > check_list(&list, values, n_remaining); > i++; > } > assert(i == n); > - assert(&e->node == &list); > + assert(e == NULL); > + assert(next == NULL); > > for (i = 0; i < n; i++) { > if (pattern & (1ul << i)) { _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev