Re: [PATCH v3 bpf-next 6/8] printk: extend test_printf to test %pT BTF-based format specifier

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jun 23, 2020 at 01:07:09PM +0100, Alan Maguire wrote:
> Add tests to verify basic type display and to iterate through all
> enums, structs, unions and typedefs ensuring expected behaviour
> occurs.  Since test_printf can be built as a module we need to
> export a BTF kind iterator function to allow us to iterate over
> all names of a particular BTF kind.
> 
> These changes add up to approximately 20,000 new tests covering
> all enum, struct, union and typedefs in vmlinux BTF.
> 
> Individual tests are also added for int, char, struct, enum
> and typedefs which verify output is as expected.

...

>  #include <linux/mm.h>
>  
>  #include <linux/property.h>

+ blank line, you see, headers are grouped.

> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <linux/skbuff.h>

> +#define	__TEST_BTF(fmt, type, ptr, expected)				       \
> +	test(expected, "%pT"fmt, ptr)
> +
> +#define TEST_BTF_C(type, var, ...)					       \
> +	do {								       \
> +		type var = __VA_ARGS__;					       \
> +		struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type);		       \

> +		pr_debug("type %s: %pTc", #type, ptr);			       \

Hmm... Can't we modify test() (or underneath macros / functions) to do this?

> +		__TEST_BTF("c", type, ptr, "(" #type ")" #__VA_ARGS__);	       \
> +	} while (0)
> +
> +#define TEST_BTF(fmt, type, var, expected, ...)				       \
> +	do {								       \
> +		type var = __VA_ARGS__;					       \
> +		struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type);		       \
> +		pr_debug("type %s: %pT"fmt, #type, ptr);		       \
> +		__TEST_BTF(fmt, type, ptr, expected);			       \
> +	} while (0)

...

> +static void __init
> +btf_print_kind(u8 kind, const char *kind_name, u64 fillval)
> +{

> +	const char *fmt1 = "%pT", *fmt2 = "%pTN", *fmt3 = "%pT0";

This is hard to read. Provide a simple data structure or an array.

> +	const char *name, *fmt = fmt1;
> +	int i, res1, res2, res3, res4;
> +	char type_name[256];
> +	char *buf, *buf2;
> +	u8 *dummy_data;
> +	s32 id = 0;
> +
> +	dummy_data = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);

check?

> +	/* fill our dummy data with supplied fillval. */
> +	for (i = 0; i < BTF_MAX_DATA_SIZE; i++)
> +		dummy_data[i] = fillval;

> +	buf = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
> +	buf2 = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);

Ditto.

> +	for (;;) {
> +		name = btf_vmlinux_next_type_name(kind, &id);
> +		if (!name)
> +			break;
> +
> +		total_tests++;
> +
> +		snprintf(type_name, sizeof(type_name), "%s%s",
> +			 kind_name, name);
> +
> +		res1 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt1,
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +		res2 = snprintf(buf, 0, fmt1,
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +		res3 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt2,
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +		res4 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt3,
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +
> +		(void) snprintf(buf, BTF_MAX_DATA_SIZE, "%pT",
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +		(void) snprintf(buf2, BTF_MAX_DATA_SIZE, "%pT",
> +				BTF_PTR_TYPE(dummy_data, type_name));
> +
> +		/*
> +		 * Ensure return value is > 0 and identical irrespective
> +		 * of whether we pass in a big enough buffer;
> +		 * also ensure that printing names always results in as
> +		 * long/longer buffer length.
> +		 */
> +		if (res1 <= 0 || res2 <= 0 || res3 <= 0 || res4 <= 0) {
> +			if (res3 <= 0)
> +				fmt = fmt2;
> +			if (res4 <= 0)
> +				fmt = fmt3;

> +			pr_warn("snprintf(%s%s); %d <= 0 (fmt %s)",
> +				kind_name, name,
> +				res1 <= 0 ? res1 : res2 <= 0 ? res2 :
> +				res3 <= 0 ? res3 : res4, fmt);
> +			failed_tests++;

For these kind of prints you can use a new macro, right?

> +		} else if (res1 != res2) {

> +			pr_warn("snprintf(%s%s): %d (to buf) != %d (no buf)",
> +				kind_name, name, res1, res2);
> +			failed_tests++;

Ditto.

> +		} else if (res3 > res2) {

> +			pr_warn("snprintf(%s%s); %d (no names) > %d (names)",
> +				kind_name, name, res3, res2);
> +			failed_tests++;

Ditto.

> +		} else if (strcmp(buf, buf2) != 0) {

> +			/* Safe and unsafe buffers should match. */
> +			pr_warn("snprintf(%s%s); safe != unsafe",
> +				kind_name, name);
> +			pr_warn("safe: %s", buf);
> +			pr_warn("unsafe: %s", buf2);
> +			failed_tests++;

Perhaps also makes sense in a macro then somebody may reuse in the future.
That said, the first warning here somehow cryptic, please be more human friendly.

> +		} else {
> +			pr_debug("Printed %s%s (%d bytes)",
> +				 kind_name, name, res1);
> +		}
> +	}
> +	kfree(dummy_data);
> +	kfree(buf);
> +	kfree(buf2);
> +}

...

> +	TEST_BTF_C(int, testint, 1234);
> +	TEST_BTF("cN", int, testint, "1234", 1234);

We use small letter macros in other cases. So can you?


...

> +	/* typedef struct */
> +	TEST_BTF_C(atomic_t, testtype, {.counter = (int)1,});
> +	TEST_BTF("cN", atomic_t, testtype, "{1,}", {.counter = 1,});
> +	/* typedef with 0 value should be printed at toplevel */
> +	TEST_BTF("c", atomic_t, testtype, "(atomic_t){}", {.counter = 0,});
> +	TEST_BTF("cN", atomic_t, testtype, "{}", {.counter = 0,});
> +	TEST_BTF("c0", atomic_t, testtype, "(atomic_t){.counter = (int)0,}",
> +		 {.counter = 0,});
> +	TEST_BTF("cN0", atomic_t, testtype, "{0,}", {.counter = 0,});

For one type, provide a data structure filled with test data and use loops.
Same for all similar places over the code.

...

> +	u64 fillvals[] = { 0x0, 0xffffffffffffffff, 0x0123456789abcdef };

U64_MAX?

-- 
With Best Regards,
Andy Shevchenko