|SUCCESS| [2/2] test: parenthesize assertion macro parameters

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163395

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-29

20260429144632.164970-2-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>

test: use inline helpers in buffer comparison macros

This patch refactors the TEST_ASSERT_BUFFERS_ARE_EQUAL family of macros
by moving comparison logic into static inline helper functions instead
of doing all the work within the macro bodies, addressing multiple
correctness and maintainability issues with the original implementation.

> 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>

[ ... ]

> +static inline bool
> +test_buffers_equal(const void *a, const void *b, size_t len)
> +{
> +	return memcmp(a, b, len) == 0;
> +}

Does this code unnecessarily compare memcmp() against zero when
converting to bool, rather than using the implicit boolean conversion?
The coding style guide indicates that comparisons against zero should be
explicit, but does the return statement here need == 0, or would
!memcmp(a, b, len) be clearer?

> +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;

Does this code check whether off + len could overflow size_t or result
in out-of-bounds access to the buffers? The original macro also lacked
bounds checking, but does adding a proper helper provide an opportunity
to validate these parameters?

> +	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);

Does this code handle the case where extra is 0 correctly? When extra is
0, (8 - extra) becomes 8, so (1U << 8) triggers undefined behavior for
8-bit types. Although the if condition guards against extra == 0, is the
cast and shift operation well-defined for all valid values of extra?

> +		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;

Does this code handle len < first_bits correctly? When off_bits is non-
zero and first_bits is computed as (8 - off_bits), could len ever be
smaller than first_bits, causing an underflow of the size_t len
variable?

> +	}
> +	return test_buffers_equal_bit(pa + off_bytes, pb + off_bytes, len);
> +}

Does this code validate that pa + off_bytes and pb + off_bytes remain
within the allocated buffer regions before dereferencing them in
test_buffers_equal_bit()?

>  /* 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)

[ ... ]

> -#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)

The refactoring successfully addresses the variadic argument issue by
ensuring ##__VA_ARGS__ is passed through all macro wrappers. Does the
conversion preserve the same semantics as the original implementation
for all edge cases?