With or without addressing inline comments below, Acked-by: Marat Khalili <[email protected]>
> -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Wednesday 29 April 2026 15:42 > To: [email protected] > Cc: Stephen Hemminger <[email protected]>; Weijun Pan > <[email protected]>; [email protected]; > Deepak Kumar Jain <[email protected]>; Pablo de Lara > <[email protected]> > Subject: [PATCH 1/2] test: use inline helpers in buffer comparison macros > > 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; > +} Not sure it's really justified to define a separate function for this, could either use memcmp directly or if striving for symmetry call test_buffers_equal_offset with zero last argument. > + > +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); I know this is from the original, but I think we had some macros for this like RTE_GENMASK32. > + > + 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; Would be nice to have some consistency in naming these. > + > + if (off_bits != 0) { > + uint8_t first_bits = 8 - off_bits; > + uint8_t mask = (1U << first_bits) - 1; Could use a standard macro here as well. > + > + if ((pa[off_bytes] & mask) != (pb[off_bytes] & mask)) > + return false; > + off_bytes++; > + len -= first_bits; Would an assert that len >= first_bits make sense before this line? > + } > + return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len); > +} > + // skip the rest
