| Message ID | 20220216142800.3295236-8-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix undefined behavior in loop macros | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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
#80 FILE: include/openvswitch/hmap.h:185:
HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)
ERROR: Inappropriate bracing around statement
#118 FILE: lib/ovs-numa.h:71:
HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->cores)
ERROR: Inappropriate bracing around statement
#122 FILE: lib/ovs-numa.h:74:
HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas)
ERROR: Use ovs_assert() in place of assert()
#133 FILE: tests/test-hmap.c:65:
assert(e == NULL);
ERROR: Use ovs_assert() in place of assert()
#141 FILE: tests/test-hmap.c:86:
assert(e == NULL);
ERROR: Use ovs_assert() in place of assert()
#150 FILE: tests/test-hmap.c:249:
assert(next == NULL);
ERROR: Use ovs_assert() in place of assert()
#152 FILE: tests/test-hmap.c:251:
assert(&next->node == hmap_next(&hmap, &e->node));
ERROR: Use ovs_assert() in place of assert()
#161 FILE: tests/test-hmap.c:269:
assert(next == NULL);
ERROR: Use ovs_assert() in place of assert()
#162 FILE: tests/test-hmap.c:270:
assert(e == NULL);
Lines checked: 169, 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
On 2/16/22 15:27, 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> > --- A couple tiny nits, with those addressed: Acked-by: Dumitru Ceara <dceara@redhat.com> > 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) Nit: kind of unrelated. > > #define FOR_EACH_NUMA_ON_DUMP(ITER, DUMP) \ > - HMAP_FOR_EACH((ITER), hmap_node, &(DUMP)->numas) > + HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas) Nit: here too.
On 2/25/22 13:07, Dumitru Ceara wrote: > On 2/16/22 15:27, 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> >> --- > > A couple tiny nits, with those addressed: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > >> 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) > > Nit: kind of unrelated. > Well... yes and no :) The internal iterator name is now determined by: #define ITER_VAR(NAME) NAME ## __iterator__ if we pass (myvar), the variable declaration will fail because (myvar)__iterator__ is not a valid variable name. Parenthesis would be required if we supported arbitrary expressions as the first argument but macros won't really work in that case. I can add a comment explaining this limitation to the macros in include/openvswitch/util.h. Or maybe you can think of a way to overcome this limitation? >> >> #define FOR_EACH_NUMA_ON_DUMP(ITER, DUMP) \ >> - HMAP_FOR_EACH((ITER), hmap_node, &(DUMP)->numas) >> + HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas) > > Nit: here too. > Thanks
On 2/28/22 10:18, Adrian Moreno wrote: > > > On 2/25/22 13:07, Dumitru Ceara wrote: >> On 2/16/22 15:27, 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> >>> --- >> >> A couple tiny nits, with those addressed: >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> >> >>> 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) >> >> Nit: kind of unrelated. >> > > Well... yes and no :) > > The internal iterator name is now determined by: > > #define ITER_VAR(NAME) NAME ## __iterator__ > > if we pass (myvar), the variable declaration will fail because > (myvar)__iterator__ is not a valid variable name. Parenthesis would be > required if we supported arbitrary expressions as the first argument but > macros won't really work in that case. > Thanks, I had missed that. > I can add a comment explaining this limitation to the macros in > include/openvswitch/util.h. Or maybe you can think of a way to overcome > this limitation? > Looking at hmap.h, it seems to me that we had this limitation from the beginning. We might as well leave it as-is. Please ignore my original comment. > >>> #define FOR_EACH_NUMA_ON_DUMP(ITER, DUMP) \ >>> - HMAP_FOR_EACH((ITER), hmap_node, &(DUMP)->numas) >>> + HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas) >> >> Nit: here too. >> > > > Thanks Regards, Dumitru
diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h index 4e001cc69..9827e993d 100644 --- a/include/openvswitch/hmap.h +++ b/include/openvswitch/hmap.h @@ -134,17 +134,19 @@ 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, \ + ITER_VAR(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, \ + ITER_VAR(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 +172,36 @@ 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, \ + ITER_VAR(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, \ + ITER_VAR(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)) {
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 | 65 ++++++++++++++++++++------------------ lib/ovs-numa.h | 4 +-- tests/test-hmap.c | 9 ++++++ 3 files changed, 46 insertions(+), 32 deletions(-)