* [PATCH 0/3] Fixes for bitfield unit tests
@ 2020-08-21 22:42 mwilck
2020-08-21 22:42 ` [PATCH 1/3] multipath-tools tests: fix bitfield tests for small fields mwilck
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: mwilck @ 2020-08-21 22:42 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Hi Christophe, hi Ben,
embarassingly, it turns out that my unit test code for the bitfield
code was broken in various ways, which at the same time shows that
I didn't test this as broadly as I should have before submitting.
The good news is that only the test code was broken, not the
implementation itself.
With these fixes, I successfully compiled it on both little and
big endian architectures, with and without glibc 2.27.
(https://build.opensuse.org/package/show/Base:System/multipath-tools)
Regards,
Martin
Martin Wilck (3):
multipath-tools tests: fix bitfield tests for small fields
multipath-tools tests: fix bitfield tests for big endian
multipath-tools tests: fix small bitfield tests for big endian
tests/util.c | 146 ++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 120 insertions(+), 26 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] multipath-tools tests: fix bitfield tests for small fields
2020-08-21 22:42 [PATCH 0/3] Fixes for bitfield unit tests mwilck
@ 2020-08-21 22:42 ` mwilck
2020-08-21 22:42 ` [PATCH 2/3] multipath-tools tests: fix bitfield tests for big endian mwilck
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: mwilck @ 2020-08-21 22:42 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
The bitmask tests may fail if sizeof(bitfield_t) is 32, depending
on compiler options and other circumstances.
This is because allocation of the bitfield with calloc() only zeroes
out the actual length of the bitfield rounded to 32, and thus
the assertion *((uint64_t *)bf->bits) == 0 may fail.
Use uint32_t in the tests instead of uint64_t.
Fixes: "libmultipath: create bitfield abstraction"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
tests/util.c | 104 +++++++++++++++++++++++++++++++++++++++------------
1 file changed, 81 insertions(+), 23 deletions(-)
diff --git a/tests/util.c b/tests/util.c
index 16774df..63a5f59 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -282,32 +282,48 @@ static void test_bitmask_len_0(void **state)
static void _test_bitmask_small(unsigned int n)
{
struct bitfield *bf;
- uint64_t *arr;
+ uint32_t *arr;
+ unsigned int size = (n - 1) / 32 + 1, i;
+ assert(sizeof(bitfield_t) == 4 || sizeof(bitfield_t) == 8);
assert(n <= 64);
assert(n >= 1);
bf = alloc_bitfield(n);
assert_non_null(bf);
assert_int_equal(bf->len, n);
- arr = (uint64_t *)bf->bits;
+ arr = (uint32_t *)bf->bits;
- assert_int_equal(*arr, 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n + 1, bf);
- assert_int_equal(*arr, 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n, bf);
- assert_int_equal(*arr, 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n - 1, bf);
- assert_int_equal(*arr, 1ULL << (n - 1));
+ for (i = 0; i < size; i++) {
+ unsigned int k = (n - 1) / 32;
+ unsigned int j = (n - 1) - k * 32;
+
+ if (i == k)
+ assert_int_equal(arr[i], 1UL << j);
+ else
+ assert_int_equal(arr[i], 0);
+ }
clear_bit_in_bitfield(n - 1, bf);
- assert_int_equal(*arr, 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(0, bf);
- assert_int_equal(*arr, 1);
+ assert_int_equal(arr[0], 1);
+ for (i = 1; i < size; i++)
+ assert_int_equal(arr[i], 0);
free(bf);
}
@@ -315,7 +331,8 @@ static void _test_bitmask_small(unsigned int n)
static void _test_bitmask_small_2(unsigned int n)
{
struct bitfield *bf;
- uint64_t *arr;
+ uint32_t *arr;
+ unsigned int size = (n - 1) / 32 + 1, i;
assert(n <= 128);
assert(n >= 65);
@@ -323,34 +340,75 @@ static void _test_bitmask_small_2(unsigned int n)
bf = alloc_bitfield(n);
assert_non_null(bf);
assert_int_equal(bf->len, n);
- arr = (uint64_t *)bf->bits;
+ arr = (uint32_t *)bf->bits;
- assert_int_equal(arr[0], 0);
- assert_int_equal(arr[1], 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n + 1, bf);
- assert_int_equal(arr[0], 0);
- assert_int_equal(arr[1], 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n, bf);
- assert_int_equal(arr[0], 0);
- assert_int_equal(arr[1], 0);
+ for (i = 0; i < size; i++)
+ assert_int_equal(arr[i], 0);
set_bit_in_bitfield(n - 1, bf);
assert_int_equal(arr[0], 0);
- assert_int_equal(arr[1], 1ULL << (n - 65));
+ for (i = 0; i < size; i++) {
+ unsigned int k = (n - 1) / 32;
+ unsigned int j = (n - 1) - k * 32;
+
+ if (i == k)
+ assert_int_equal(arr[i], 1UL << j);
+ else
+ assert_int_equal(arr[i], 0);
+ }
set_bit_in_bitfield(0, bf);
- assert_int_equal(arr[0], 1);
- assert_int_equal(arr[1], 1ULL << (n - 65));
+ for (i = 0; i < size; i++) {
+ unsigned int k = (n - 1) / 32;
+ unsigned int j = (n - 1) - k * 32;
+
+ if (i == k && k == 0)
+ assert_int_equal(arr[i], (1UL << j) | 1);
+ else if (i == k)
+ assert_int_equal(arr[i], 1UL << j);
+ else if (i == 0)
+ assert_int_equal(arr[i], 1);
+ else
+ assert_int_equal(arr[i], 0);
+ }
set_bit_in_bitfield(64, bf);
- assert_int_equal(arr[0], 1);
- assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
+ for (i = 0; i < size; i++) {
+ unsigned int k = (n - 1) / 32;
+ unsigned int j = (n - 1) - k * 32;
+
+ if (i == k && (k == 0 || k == 2))
+ assert_int_equal(arr[i], (1UL << j) | 1);
+ else if (i == k)
+ assert_int_equal(arr[i], 1UL << j);
+ else if (i == 2 || i == 0)
+ assert_int_equal(arr[i], 1);
+ else
+ assert_int_equal(arr[i], 0);
+ }
clear_bit_in_bitfield(0, bf);
- assert_int_equal(arr[0], 0);
- assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
+ for (i = 0; i < size; i++) {
+ unsigned int k = (n - 1) / 32;
+ unsigned int j = (n - 1) - k * 32;
+
+ if (i == k && k == 2)
+ assert_int_equal(arr[i], (1UL << j) | 1);
+ else if (i == k)
+ assert_int_equal(arr[i], 1UL << j);
+ else if (i == 2)
+ assert_int_equal(arr[i], 1);
+ else
+ assert_int_equal(arr[i], 0);
+ }
free(bf);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] multipath-tools tests: fix bitfield tests for big endian
2020-08-21 22:42 [PATCH 0/3] Fixes for bitfield unit tests mwilck
2020-08-21 22:42 ` [PATCH 1/3] multipath-tools tests: fix bitfield tests for small fields mwilck
@ 2020-08-21 22:42 ` mwilck
2020-08-21 22:42 ` [PATCH 3/3] multipath-tools tests: fix small " mwilck
2020-09-15 11:16 ` [PATCH 0/3] Fixes for bitfield unit tests Martin Wilck
3 siblings, 0 replies; 6+ messages in thread
From: mwilck @ 2020-08-21 22:42 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
On big endian systems, the 32bit words need to be swapped in
the test code to get the byte ordering right.
Fixes: "libmultipath: create bitfield abstraction"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
tests/util.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/tests/util.c b/tests/util.c
index 63a5f59..ec38c55 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -162,6 +162,22 @@ static int test_basenamecpy(void)
return cmocka_run_group_tests(tests, NULL, NULL);
}
+/*
+ * On big endian systems, if bitfield_t is 32bit, we need
+ * to swap the two 32 bit parts of a 64bit value to make
+ * the tests below work.
+ */
+static uint64_t maybe_swap(uint64_t v)
+{
+ uint32_t *s = (uint32_t *)&v;
+
+ if (sizeof(bitfield_t) == 4)
+ /* this is identity for little endian */
+ return ((uint64_t)s[1] << 32) | s[0];
+ else
+ return v;
+}
+
static void test_bitmask_1(void **state)
{
struct bitfield *bf;
@@ -184,7 +200,7 @@ static void test_bitmask_1(void **state)
b, j, k, arr[k]);
#endif
if (k == j)
- assert_int_equal(arr[j], 1ULL << i);
+ assert_int_equal(maybe_swap(arr[j]), 1ULL << i);
else
assert_int_equal(arr[k], 0ULL);
}
@@ -235,7 +251,7 @@ static void test_bitmask_2(void **state)
assert_int_equal(arr[k], 0ULL);
else
assert_int_equal(
- arr[k],
+ maybe_swap(arr[k]),
(1ULL << (i + 1)) - 1);
}
}
@@ -260,7 +276,7 @@ static void test_bitmask_2(void **state)
assert_int_equal(arr[k], ~0ULL);
else
assert_int_equal(
- arr[k],
+ maybe_swap(arr[k]),
~((1ULL << (i + 1)) - 1));
}
}
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] multipath-tools tests: fix small bitfield tests for big endian
2020-08-21 22:42 [PATCH 0/3] Fixes for bitfield unit tests mwilck
2020-08-21 22:42 ` [PATCH 1/3] multipath-tools tests: fix bitfield tests for small fields mwilck
2020-08-21 22:42 ` [PATCH 2/3] multipath-tools tests: fix bitfield tests for big endian mwilck
@ 2020-08-21 22:42 ` mwilck
2020-09-15 11:16 ` [PATCH 0/3] Fixes for bitfield unit tests Martin Wilck
3 siblings, 0 replies; 6+ messages in thread
From: mwilck @ 2020-08-21 22:42 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
This is the opposite case of the previous patch: 64 bit bitfield_t,
tested with 32 bit words.
Fixes: "libmultipath: create bitfield abstraction"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
tests/util.c | 60 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 40 insertions(+), 20 deletions(-)
diff --git a/tests/util.c b/tests/util.c
index ec38c55..21311b2 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -22,6 +22,7 @@
#include <setjmp.h>
#include <stdlib.h>
#include <cmocka.h>
+#include <endian.h>
#include "util.h"
#include "globals.c"
@@ -295,11 +296,25 @@ static void test_bitmask_len_0(void **state)
assert_null(bf);
}
+/*
+ * We use uint32_t in the "small bitmask" tests below.
+ * This means that we may have to swap 32bit words if bitfield_t
+ * is 64bit wide.
+ */
+static unsigned int maybe_swap_idx(unsigned int i)
+{
+ if (BYTE_ORDER == LITTLE_ENDIAN || sizeof(bitfield_t) == 4)
+ return i;
+ else
+ /* 0<->1, 2<->3, ... */
+ return i + (i % 2 == 0 ? 1 : -1);
+}
+
static void _test_bitmask_small(unsigned int n)
{
struct bitfield *bf;
uint32_t *arr;
- unsigned int size = (n - 1) / 32 + 1, i;
+ unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
assert(sizeof(bitfield_t) == 4 || sizeof(bitfield_t) == 8);
assert(n <= 64);
@@ -325,11 +340,12 @@ static void _test_bitmask_small(unsigned int n)
for (i = 0; i < size; i++) {
unsigned int k = (n - 1) / 32;
unsigned int j = (n - 1) - k * 32;
+ unsigned int i1 = maybe_swap_idx(i);
if (i == k)
- assert_int_equal(arr[i], 1UL << j);
+ assert_int_equal(arr[i1], 1UL << j);
else
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[i1], 0);
}
clear_bit_in_bitfield(n - 1, bf);
@@ -337,9 +353,9 @@ static void _test_bitmask_small(unsigned int n)
assert_int_equal(arr[i], 0);
set_bit_in_bitfield(0, bf);
- assert_int_equal(arr[0], 1);
+ assert_int_equal(arr[maybe_swap_idx(0)], 1);
for (i = 1; i < size; i++)
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[maybe_swap_idx(i)], 0);
free(bf);
}
@@ -348,7 +364,7 @@ static void _test_bitmask_small_2(unsigned int n)
{
struct bitfield *bf;
uint32_t *arr;
- unsigned int size = (n - 1) / 32 + 1, i;
+ unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
assert(n <= 128);
assert(n >= 65);
@@ -374,56 +390,60 @@ static void _test_bitmask_small_2(unsigned int n)
for (i = 0; i < size; i++) {
unsigned int k = (n - 1) / 32;
unsigned int j = (n - 1) - k * 32;
+ unsigned int i1 = maybe_swap_idx(i);
if (i == k)
- assert_int_equal(arr[i], 1UL << j);
+ assert_int_equal(arr[i1], 1UL << j);
else
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[i1], 0);
}
set_bit_in_bitfield(0, bf);
for (i = 0; i < size; i++) {
unsigned int k = (n - 1) / 32;
unsigned int j = (n - 1) - k * 32;
+ unsigned int i1 = maybe_swap_idx(i);
if (i == k && k == 0)
- assert_int_equal(arr[i], (1UL << j) | 1);
+ assert_int_equal(arr[i1], (1UL << j) | 1);
else if (i == k)
- assert_int_equal(arr[i], 1UL << j);
+ assert_int_equal(arr[i1], 1UL << j);
else if (i == 0)
- assert_int_equal(arr[i], 1);
+ assert_int_equal(arr[i1], 1);
else
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[i1], 0);
}
set_bit_in_bitfield(64, bf);
for (i = 0; i < size; i++) {
unsigned int k = (n - 1) / 32;
unsigned int j = (n - 1) - k * 32;
+ unsigned int i1 = maybe_swap_idx(i);
if (i == k && (k == 0 || k == 2))
- assert_int_equal(arr[i], (1UL << j) | 1);
+ assert_int_equal(arr[i1], (1UL << j) | 1);
else if (i == k)
- assert_int_equal(arr[i], 1UL << j);
+ assert_int_equal(arr[i1], 1UL << j);
else if (i == 2 || i == 0)
- assert_int_equal(arr[i], 1);
+ assert_int_equal(arr[i1], 1);
else
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[i1], 0);
}
clear_bit_in_bitfield(0, bf);
for (i = 0; i < size; i++) {
unsigned int k = (n - 1) / 32;
unsigned int j = (n - 1) - k * 32;
+ unsigned int i1 = maybe_swap_idx(i);
if (i == k && k == 2)
- assert_int_equal(arr[i], (1UL << j) | 1);
+ assert_int_equal(arr[i1], (1UL << j) | 1);
else if (i == k)
- assert_int_equal(arr[i], 1UL << j);
+ assert_int_equal(arr[i1], 1UL << j);
else if (i == 2)
- assert_int_equal(arr[i], 1);
+ assert_int_equal(arr[i1], 1);
else
- assert_int_equal(arr[i], 0);
+ assert_int_equal(arr[i1], 0);
}
free(bf);
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Fixes for bitfield unit tests
2020-08-21 22:42 [PATCH 0/3] Fixes for bitfield unit tests mwilck
` (2 preceding siblings ...)
2020-08-21 22:42 ` [PATCH 3/3] multipath-tools tests: fix small " mwilck
@ 2020-09-15 11:16 ` Martin Wilck
2020-09-15 16:52 ` Benjamin Marzinski
3 siblings, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2020-09-15 11:16 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
On Sat, 2020-08-22 at 00:42 +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> Hi Christophe, hi Ben,
>
> embarassingly, it turns out that my unit test code for the bitfield
> code was broken in various ways, which at the same time shows that
> I didn't test this as broadly as I should have before submitting.
> The good news is that only the test code was broken, not the
> implementation itself.
>
> With these fixes, I successfully compiled it on both little and
> big endian architectures, with and without glibc 2.27.
> (https://build.opensuse.org/package/show/Base:System/multipath-tools)
>
> Regards,
> Martin
>
> Martin Wilck (3):
> multipath-tools tests: fix bitfield tests for small fields
> multipath-tools tests: fix bitfield tests for big endian
> multipath-tools tests: fix small bitfield tests for big endian
>
> tests/util.c | 146 ++++++++++++++++++++++++++++++++++++++++++-------
> --
> 1 file changed, 120 insertions(+), 26 deletions(-)
@Ben: Gentle review reminder ...
Cheers,
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] Fixes for bitfield unit tests
2020-09-15 11:16 ` [PATCH 0/3] Fixes for bitfield unit tests Martin Wilck
@ 2020-09-15 16:52 ` Benjamin Marzinski
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2020-09-15 16:52 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Tue, Sep 15, 2020 at 01:16:19PM +0200, Martin Wilck wrote:
> On Sat, 2020-08-22 at 00:42 +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> >
> > Hi Christophe, hi Ben,
> >
> > embarassingly, it turns out that my unit test code for the bitfield
> > code was broken in various ways, which at the same time shows that
> > I didn't test this as broadly as I should have before submitting.
> > The good news is that only the test code was broken, not the
> > implementation itself.
> >
> > With these fixes, I successfully compiled it on both little and
> > big endian architectures, with and without glibc 2.27.
> > (https://build.opensuse.org/package/show/Base:System/multipath-tools)
> >
> > Regards,
> > Martin
> >
> > Martin Wilck (3):
> > multipath-tools tests: fix bitfield tests for small fields
> > multipath-tools tests: fix bitfield tests for big endian
> > multipath-tools tests: fix small bitfield tests for big endian
> >
> > tests/util.c | 146 ++++++++++++++++++++++++++++++++++++++++++-------
> > --
> > 1 file changed, 120 insertions(+), 26 deletions(-)
>
> @Ben: Gentle review reminder ...
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> Cheers,
> Martin
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-15 16:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21 22:42 [PATCH 0/3] Fixes for bitfield unit tests mwilck
2020-08-21 22:42 ` [PATCH 1/3] multipath-tools tests: fix bitfield tests for small fields mwilck
2020-08-21 22:42 ` [PATCH 2/3] multipath-tools tests: fix bitfield tests for big endian mwilck
2020-08-21 22:42 ` [PATCH 3/3] multipath-tools tests: fix small " mwilck
2020-09-15 11:16 ` [PATCH 0/3] Fixes for bitfield unit tests Martin Wilck
2020-09-15 16:52 ` Benjamin Marzinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.