| Message ID | 20260901210356.730438-1-jtanenba@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Ales Musil |
| Headers | show |
| Series | [ovs-dev,v1,1/2] lib: Fix n_elems accounting in dynamic_bitmap_or(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
On Tue, Sep 1, 2026 at 11:04 PM Jacob Tanenbaum via dev < ovs-dev@openvswitch.org> wrote: > Performing dynamic_bitmap_or() can change the number of elements, > causing n_elems to potentially not reflect the number of elements in the > bitmap. Recalculate the n_elems in dynamic_bitmap_or(). > > Fixes: 2d2e1e600fd0 ("util: Improve dynamic_bitmap APIs.") > Assisted-by: Claude Opus 4.8, Claude Code > Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com> > --- > lib/ovn-util.h | 1 + > tests/ovn.at | 1 + > tests/test-sparse-array.c | 54 ++++++++++++++++++++++++++++++++++++--- > 3 files changed, 53 insertions(+), 3 deletions(-) > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 3ffcf42c3..9897db8f0 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -640,6 +640,7 @@ dynamic_bitmap_or(struct dynamic_bitmap *db, > { > ovs_assert(db->capacity == n); > bitmap_or(db->map, arg, n); > + db->n_elems = bitmap_count1(db->map, db->capacity); > nit: dynamic_bitmap_count1(db). > } > > static inline unsigned long * > diff --git a/tests/ovn.at b/tests/ovn.at > index b9fdb9c98..7e5eb301e 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -2458,6 +2458,7 @@ AT_CLEANUP > AT_SETUP([Sparse array operations]) > check ovstest test-sparse-array add > check ovstest test-sparse-array remove-replace > +check ovstest test-sparse-array bitmap-or > AT_CLEANUP > > AT_SETUP([SPSC ring buffer]) > diff --git a/tests/test-sparse-array.c b/tests/test-sparse-array.c > index 3948ba884..4258550c1 100644 > --- a/tests/test-sparse-array.c > +++ b/tests/test-sparse-array.c > @@ -188,14 +188,62 @@ test_remove_replace(struct ovs_cmdl_context *ctx > OVS_UNUSED) > free(item_five); > } > > +static void > +test_dynamic_bitmap_or(struct ovs_cmdl_context *ctx OVS_UNUSED) > +{ > + struct dynamic_bitmap a, b; > + > + dynamic_bitmap_alloc(&a, 8); > + dynamic_bitmap_alloc(&b, 8); > + > + /* Set bits 1, 3 in 'a' through the tracked API. */ > + dynamic_bitmap_set1(&a, 1); > + dynamic_bitmap_set1(&a, 3); > + ovs_assert(a.n_elems == 2); > + > + /* Set bits 3, 5, 7 in 'b'. */ > + dynamic_bitmap_set1(&b, 3); > + dynamic_bitmap_set1(&b, 5); > + dynamic_bitmap_set1(&b, 7); > + ovs_assert(b.n_elems == 3); > + > + /* OR 'b' into 'a'. Result should be {1, 3, 5, 7} = 4 elements. > + * Before the fix, n_elems stayed at 2 because dynamic_bitmap_or > + * did not recount. */ > + dynamic_bitmap_or(&a, b.map, b.capacity); > + ovs_assert(a.n_elems == 4); > + ovs_assert(dynamic_bitmap_is_set(&a, 1)); > + ovs_assert(dynamic_bitmap_is_set(&a, 3)); > + ovs_assert(dynamic_bitmap_is_set(&a, 5)); > + ovs_assert(dynamic_bitmap_is_set(&a, 7)); > + ovs_assert(!dynamic_bitmap_is_set(&a, 0)); > + ovs_assert(!dynamic_bitmap_is_set(&a, 2)); > + > + /* Clearing a bit that was added by the OR must not underflow > + * n_elems. Before the fix, n_elems was 2 here so clearing two > + * OR-added bits would wrap to SIZE_MAX. */ > + dynamic_bitmap_set0(&a, 5); > + ovs_assert(a.n_elems == 3); > + dynamic_bitmap_set0(&a, 7); > + ovs_assert(a.n_elems == 2); > + dynamic_bitmap_set0(&a, 1); > + ovs_assert(a.n_elems == 1); > + dynamic_bitmap_set0(&a, 3); > + ovs_assert(a.n_elems == 0); > + > + dynamic_bitmap_free(&a); > + dynamic_bitmap_free(&b); > +} > + > static void > test_sparse_array_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) > { > ovn_set_program_name(argv[0]); > static const struct ovs_cmdl_command commands[] = { > - {"add", NULL, 0, 0, test_add, OVS_RO}, > - {"remove-replace", NULL, 0, 0, test_remove_replace, OVS_RO}, > - {NULL, NULL, 0, 0, NULL, OVS_RO}, > + {"add", NULL, 0, 0, test_add, OVS_RO}, > + {"remove-replace", NULL, 0, 0, test_remove_replace, OVS_RO}, > + {"bitmap-or", NULL, 0, 0, test_dynamic_bitmap_or, OVS_RO}, > + {NULL, NULL, 0, 0, NULL, OVS_RO}, > }; > struct ovs_cmdl_context ctx; > ctx.argc = argc - 1; > -- > 2.55.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thank you Jacob, applied to main, with the nit fixed, and backported down to 26.03. Regards, Ales
diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 3ffcf42c3..9897db8f0 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -640,6 +640,7 @@ dynamic_bitmap_or(struct dynamic_bitmap *db, { ovs_assert(db->capacity == n); bitmap_or(db->map, arg, n); + db->n_elems = bitmap_count1(db->map, db->capacity); } static inline unsigned long * diff --git a/tests/ovn.at b/tests/ovn.at index b9fdb9c98..7e5eb301e 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -2458,6 +2458,7 @@ AT_CLEANUP AT_SETUP([Sparse array operations]) check ovstest test-sparse-array add check ovstest test-sparse-array remove-replace +check ovstest test-sparse-array bitmap-or AT_CLEANUP AT_SETUP([SPSC ring buffer]) diff --git a/tests/test-sparse-array.c b/tests/test-sparse-array.c index 3948ba884..4258550c1 100644 --- a/tests/test-sparse-array.c +++ b/tests/test-sparse-array.c @@ -188,14 +188,62 @@ test_remove_replace(struct ovs_cmdl_context *ctx OVS_UNUSED) free(item_five); } +static void +test_dynamic_bitmap_or(struct ovs_cmdl_context *ctx OVS_UNUSED) +{ + struct dynamic_bitmap a, b; + + dynamic_bitmap_alloc(&a, 8); + dynamic_bitmap_alloc(&b, 8); + + /* Set bits 1, 3 in 'a' through the tracked API. */ + dynamic_bitmap_set1(&a, 1); + dynamic_bitmap_set1(&a, 3); + ovs_assert(a.n_elems == 2); + + /* Set bits 3, 5, 7 in 'b'. */ + dynamic_bitmap_set1(&b, 3); + dynamic_bitmap_set1(&b, 5); + dynamic_bitmap_set1(&b, 7); + ovs_assert(b.n_elems == 3); + + /* OR 'b' into 'a'. Result should be {1, 3, 5, 7} = 4 elements. + * Before the fix, n_elems stayed at 2 because dynamic_bitmap_or + * did not recount. */ + dynamic_bitmap_or(&a, b.map, b.capacity); + ovs_assert(a.n_elems == 4); + ovs_assert(dynamic_bitmap_is_set(&a, 1)); + ovs_assert(dynamic_bitmap_is_set(&a, 3)); + ovs_assert(dynamic_bitmap_is_set(&a, 5)); + ovs_assert(dynamic_bitmap_is_set(&a, 7)); + ovs_assert(!dynamic_bitmap_is_set(&a, 0)); + ovs_assert(!dynamic_bitmap_is_set(&a, 2)); + + /* Clearing a bit that was added by the OR must not underflow + * n_elems. Before the fix, n_elems was 2 here so clearing two + * OR-added bits would wrap to SIZE_MAX. */ + dynamic_bitmap_set0(&a, 5); + ovs_assert(a.n_elems == 3); + dynamic_bitmap_set0(&a, 7); + ovs_assert(a.n_elems == 2); + dynamic_bitmap_set0(&a, 1); + ovs_assert(a.n_elems == 1); + dynamic_bitmap_set0(&a, 3); + ovs_assert(a.n_elems == 0); + + dynamic_bitmap_free(&a); + dynamic_bitmap_free(&b); +} + static void test_sparse_array_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) { ovn_set_program_name(argv[0]); static const struct ovs_cmdl_command commands[] = { - {"add", NULL, 0, 0, test_add, OVS_RO}, - {"remove-replace", NULL, 0, 0, test_remove_replace, OVS_RO}, - {NULL, NULL, 0, 0, NULL, OVS_RO}, + {"add", NULL, 0, 0, test_add, OVS_RO}, + {"remove-replace", NULL, 0, 0, test_remove_replace, OVS_RO}, + {"bitmap-or", NULL, 0, 0, test_dynamic_bitmap_or, OVS_RO}, + {NULL, NULL, 0, 0, NULL, OVS_RO}, }; struct ovs_cmdl_context ctx; ctx.argc = argc - 1;
Performing dynamic_bitmap_or() can change the number of elements, causing n_elems to potentially not reflect the number of elements in the bitmap. Recalculate the n_elems in dynamic_bitmap_or(). Fixes: 2d2e1e600fd0 ("util: Improve dynamic_bitmap APIs.") Assisted-by: Claude Opus 4.8, Claude Code Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com> --- lib/ovn-util.h | 1 + tests/ovn.at | 1 + tests/test-sparse-array.c | 54 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 3 deletions(-)