Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw

[David Hildenbrand <david@redhat.com>]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Update memblock_alloc() tests so that they test either memblock_alloc()
> or memblock_alloc_raw() depending on the value of alloc_test_flags. Run
> through all the existing tests in memblock_alloc_api twice: once for
> memblock_alloc() and once for memblock_alloc_raw().
> 
> When the tests run memblock_alloc(), they test that the entire memory
> region is zero. When the tests run memblock_alloc_raw(), they test that
> the entire memory region is nonzero.

Could add a comment stating that we initialize the content to nonzero in
that case, and expect it to remain unchanged (== not zeroed).

> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  tools/testing/memblock/tests/alloc_api.c | 98 ++++++++++++++++--------
>  tools/testing/memblock/tests/common.h    | 25 ++++++
>  2 files changed, 90 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
> index 65bff77dd55b..cf67687ae044 100644
> --- a/tools/testing/memblock/tests/alloc_api.c
> +++ b/tools/testing/memblock/tests/alloc_api.c
> @@ -1,6 +1,29 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #include "alloc_api.h"
>  
> +static const char * const func_testing[] = {
> +	"memblock_alloc",
> +	"memblock_alloc_raw"
> +};
> +
> +static int alloc_test_flags = TEST_ZEROED;
> +
> +static inline const char * const get_func_testing(int flags)
> +{
> +	if (flags & TEST_RAW)
> +		return func_testing[1];
> +	else
> +		return func_testing[0];

No need for the else, you can return directly.

Can we avoid the func_testing array?


Persoally, I consider the "get_func_testing()" name a bit confusing.

get_memblock_alloc_name() ?


> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
> index 58f84bf2c9ae..4fd3534ff955 100644
> --- a/tools/testing/memblock/tests/common.h
> +++ b/tools/testing/memblock/tests/common.h
> @@ -12,6 +12,11 @@
>  
>  #define MEM_SIZE SZ_16K
>  
> +enum test_flags {
> +	TEST_ZEROED = 0x0,
> +	TEST_RAW = 0x1
> +};

I'd have called this

enum test_flags {
	/* No special request. */
	TEST_F_NONE = 0x0,
	/* Perform raw allocations (no zeroing of memory).
	TEST_F_RAW = 0x1,
};

Further, I'd just have use #define for the flags.

> +
>  /**
>   * ASSERT_EQ():
>   * Check the condition
> @@ -63,6 +68,18 @@
>  	} \
>  } while (0)
>  
> +/**
> + * ASSERT_MEM_NE():
> + * Check that none of the first @_size bytes of @_seen are equal to @_expected.
> + * If false, print failed test message (if running with --verbose) and then
> + * assert.
> + */
> +#define ASSERT_MEM_NE(_seen, _expected, _size) do { \
> +	for (int _i = 0; _i < (_size); _i++) { \
> +		ASSERT_NE((_seen)[_i], (_expected)); \
> +	} \
> +} while (0)
> +
>  #define PREFIX_PUSH() prefix_push(__func__)
>  
>  /*
> @@ -116,4 +133,12 @@ static inline void run_bottom_up(int (*func)())
>  	prefix_pop();
>  }
>  
> +static inline void verify_mem_content(void *mem, int size, int flags)

nit: why use verify here when the other functions "assert". I'd have
called this something like "assert_mem_content()"


-- 
Thanks,

David / dhildenb