RE: [PATCH 1/2] test: use inline helpers in buffer comparison macros

[Marat Khalili <marat.khalili@huawei.com>]

With or without addressing inline comments below,

Acked-by: Marat Khalili <marat.khalili@huawei.com>

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday 29 April 2026 15:42
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Weijun Pan <wpan3636@gmail.com>; stable@dpdk.org;
> Deepak Kumar Jain <deepak.k.jain@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 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 <wpan3636@gmail.com>
> Fixes: db4faf469816 ("app/test: add new buffer comparison macros")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  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