The TEST_ASSERT_BUFFERS_ARE_EQUAL family of macros had several problems stemming from doing all the work inside the macro body:
- Macro parameters were used directly in arithmetic expressions without parentheses. - Arguments were evaluated multiple times. - There was no type checking. In addition, the _OFFSET and _BIT_OFFSET wrappers chained to the inner assertion macro passing only "msg" and dropped the variadic arguments, so any printf parameters after the format string were silently lost on failure. Move the comparison logic into static inline helpers and keep the macros as thin wrappers around the printf / TEST_TRACE_FAILURE / return TEST_FAILED boilerplate (which must remain a macro to capture __func__ / __LINE__ and to return from the caller). Each argument is now evaluated exactly once where the macro hands it to the helper, the size_t parameters give the compiler real types to check against, and the bit-mask arithmetic lives in C rather than the preprocessor. Existing call sites need no changes. Bugzilla ID: 1925 Suggested-by: Weijun Pan <[email protected]> Fixes: db4faf469816 ("app/test: add new buffer comparison macros") Cc: [email protected] Signed-off-by: Stephen Hemminger <[email protected]> --- app/test/test.h | 147 +++++++++++++++++++++++++++++++----------------- 1 file changed, 95 insertions(+), 52 deletions(-) diff --git a/app/test/test.h b/app/test/test.h index 1f12fc5397..ebf6727639 100644 --- a/app/test/test.h +++ b/app/test/test.h @@ -6,8 +6,12 @@ #define _TEST_H_ #include <errno.h> +#include <stdbool.h> #include <stddef.h> +#include <stdint.h> +#include <stdio.h> #include <stdlib.h> +#include <string.h> #include <sys/queue.h> #include <rte_hexdump.h> @@ -32,70 +36,109 @@ #define TEST_ASSERT_EQUAL RTE_TEST_ASSERT_EQUAL +/* + * Helpers backing the TEST_ASSERT_BUFFERS_ARE_EQUAL* macros. + * + * Keeping the comparison logic in inline functions ensures each macro + * argument is evaluated exactly once and gives the compiler real types + * to check against, which the original all-macro implementation could + * not provide. + */ +static inline bool +test_buffers_equal(const void *a, const void *b, size_t len) +{ + return memcmp(a, b, len) == 0; +} + +static inline bool +test_buffers_equal_offset(const void *a, const void *b, + size_t len, size_t off) +{ + const uint8_t *pa = (const uint8_t *)a + off; + const uint8_t *pb = (const uint8_t *)b + off; + + return memcmp(pa, pb, len) == 0; +} + +static inline bool +test_buffers_equal_bit(const void *a, const void *b, size_t len) +{ + const uint8_t *pa = a; + const uint8_t *pb = b; + size_t whole = len >> 3; + size_t extra = len & 7; + + if (memcmp(pa, pb, whole) != 0) + return false; + if (extra != 0) { + uint8_t mask = (uint8_t)~((1U << (8 - extra)) - 1); + + if ((pa[whole] & mask) != (pb[whole] & mask)) + return false; + } + return true; +} + +static inline bool +test_buffers_equal_bit_offset(const void *a, const void *b, + size_t len, size_t off) +{ + const uint8_t *pa = a; + const uint8_t *pb = b; + size_t off_bits = off & 7; + size_t off_bytes = off >> 3; + + if (off_bits != 0) { + uint8_t first_bits = 8 - off_bits; + uint8_t mask = (1U << first_bits) - 1; + + if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask)) + return false; + off_bytes++; + len -= first_bits; + } + return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len); +} + /* Compare two buffers (length in bytes) */ -#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len, msg, ...) do { \ - if (memcmp(a, b, len)) { \ - printf("TestCase %s() line %d failed: " \ - msg "\n", __func__, __LINE__, ##__VA_ARGS__); \ - TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ - return TEST_FAILED; \ - } \ +#define TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, len, msg, ...) do { \ + if (!test_buffers_equal((a), (b), (len))) { \ + printf("TestCase %s() line %d failed: " msg "\n", \ + __func__, __LINE__, ##__VA_ARGS__); \ + TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ + return TEST_FAILED; \ + } \ } while (0) /* Compare two buffers with offset (length and offset in bytes) */ #define TEST_ASSERT_BUFFERS_ARE_EQUAL_OFFSET(a, b, len, off, msg, ...) do { \ - const uint8_t *_a_with_off = (const uint8_t *)a + off; \ - const uint8_t *_b_with_off = (const uint8_t *)b + off; \ - TEST_ASSERT_BUFFERS_ARE_EQUAL(_a_with_off, _b_with_off, len, msg); \ + if (!test_buffers_equal_offset((a), (b), (len), (off))) { \ + printf("TestCase %s() line %d failed: " msg "\n", \ + __func__, __LINE__, ##__VA_ARGS__); \ + TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ + return TEST_FAILED; \ + } \ } while (0) /* Compare two buffers (length in bits) */ #define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(a, b, len, msg, ...) do { \ - uint8_t _last_byte_a, _last_byte_b; \ - uint8_t _last_byte_mask, _last_byte_bits; \ - TEST_ASSERT_BUFFERS_ARE_EQUAL(a, b, (len >> 3), msg); \ - if (len % 8) { \ - _last_byte_bits = len % 8; \ - _last_byte_mask = ~((1 << (8 - _last_byte_bits)) - 1); \ - _last_byte_a = ((const uint8_t *)a)[len >> 3]; \ - _last_byte_b = ((const uint8_t *)b)[len >> 3]; \ - _last_byte_a &= _last_byte_mask; \ - _last_byte_b &= _last_byte_mask; \ - if (_last_byte_a != _last_byte_b) { \ - printf("TestCase %s() line %d failed: " \ - msg "\n", __func__, __LINE__, ##__VA_ARGS__);\ - TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ - return TEST_FAILED; \ - } \ - } \ + if (!test_buffers_equal_bit((a), (b), (len))) { \ + printf("TestCase %s() line %d failed: " msg "\n", \ + __func__, __LINE__, ##__VA_ARGS__); \ + TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ + return TEST_FAILED; \ + } \ } while (0) /* Compare two buffers with offset (length and offset in bits) */ -#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) do { \ - uint8_t _first_byte_a, _first_byte_b; \ - uint8_t _first_byte_mask, _first_byte_bits; \ - uint32_t _len_without_first_byte = (off % 8) ? \ - len - (8 - (off % 8)) : \ - len; \ - uint32_t _off_in_bytes = (off % 8) ? (off >> 3) + 1 : (off >> 3); \ - const uint8_t *_a_with_off = (const uint8_t *)a + _off_in_bytes; \ - const uint8_t *_b_with_off = (const uint8_t *)b + _off_in_bytes; \ - TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT(_a_with_off, _b_with_off, \ - _len_without_first_byte, msg); \ - if (off % 8) { \ - _first_byte_bits = 8 - (off % 8); \ - _first_byte_mask = (1 << _first_byte_bits) - 1; \ - _first_byte_a = *(_a_with_off - 1); \ - _first_byte_b = *(_b_with_off - 1); \ - _first_byte_a &= _first_byte_mask; \ - _first_byte_b &= _first_byte_mask; \ - if (_first_byte_a != _first_byte_b) { \ - printf("TestCase %s() line %d failed: " \ - msg "\n", __func__, __LINE__, ##__VA_ARGS__); \ - TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ - return TEST_FAILED; \ - } \ - } \ +#define TEST_ASSERT_BUFFERS_ARE_EQUAL_BIT_OFFSET(a, b, len, off, msg, ...) \ +do { \ + if (!test_buffers_equal_bit_offset((a), (b), (len), (off))) { \ + printf("TestCase %s() line %d failed: " msg "\n", \ + __func__, __LINE__, ##__VA_ARGS__); \ + TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__); \ + return TEST_FAILED; \ + } \ } while (0) #define TEST_ASSERT_NOT_EQUAL RTE_TEST_ASSERT_NOT_EQUAL -- 2.53.0
