Re: [PATCH bpf-next v2 2/2] selftests/bpf: introduce veristat test

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2025-02-10 at 13:51 +0000, Mykyta Yatsenko wrote:

[...]

> +void test_veristat_set_global_vars_succeeds(void)
> +{

test_progs based tests are usually organized as a hierarchy of tests and sub-tests.
E.g. take a look at tools/testing/selftests/bpf/prog_tests/ksyms_btf.c:
- it defines an entry point test_ksyms_btf;
- and a bunch of sub-tests declared as static void functions,
  called from entry point;
- test__start_subtest() function is used to check if sub-test has to
  be executed.

> +	char command[512];
> +	struct fixture *fix = init_fixture();
> +
> +	snprintf(command, sizeof(command),
> +		 "./veristat set_global_vars.bpf.o"\
> +		 " -G \"var_s64 = 0xf000000000000001\" "\
> +		 " -G \"var_u64 = 0xfedcba9876543210\" "\
> +		 " -G \"var_s32 = -0x80000000\" "\
> +		 " -G \"var_u32 = 0x76543210\" "\
> +		 " -G \"var_s16 = -32768\" "\
> +		 " -G \"var_u16 = 60652\" "\
> +		 " -G \"var_s8 = -128\" "\
> +		 " -G \"var_u8 = 255\" "\
> +		 " -G \"var_ea = EA2\" "\
> +		 " -G \"var_eb = EB2\" "\
> +		 " -G \"var_ec = EC2\" "\
> +		 " -G \"var_b = 1\" "\
> +		 "-vl2 > %s", fix->tmpfile);
> +	if (!ASSERT_EQ(0, system(command), "command"))
> +		goto out;

Nit: there is SYS macro in test_progs.h, it combines
     snprintf/system/ASSERT_OK/goto.

> +
> +	read(fix->fd, fix->output, fix->sz);

Nit: check error for read() call (same read()/write() in tests below).

> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0xf000000000000001 "),
> +		   "var_s64 = 0xf000000000000001");

Nit: I'd do these checks as below:

#define __CHECK_STR(str, name) \
	if (!ASSERT_HAS_SUBSTR(fix->output, (str), (str))) goto out
        __CHECK_STR("_w=0xf000000000000001 ");
        ...
#undef __CHECK_STR

     this way fix->output would be printed if sub-string is not found.
     For other tests I suggest using ASSERT_HAS_SUBSTR as well,
     as it prints the string where sub-string was looked for.

> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0xfedcba9876543210 "),
> +		   "var_u64 = 0xfedcba9876543210");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0x80000000 "), "var_s32 = -0x80000000");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0x76543210 "), "var_u32 = 0x76543210");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0x8000 "), "var_s16 = -32768");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0xecec "), "var_u16 = 60652");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=128 "), "var_s8 = -128");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=255 "), "var_u8 = 255");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=11 "), "var_ea = EA2");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=12 "), "var_eb = EB2");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=13 "), "var_ec = EC2");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=1 "), "var_b = 1");
> +
> +out:
> +	teardown_fixture(fix);
> +}
> +
> +void test_veristat_set_global_vars_from_file_succeeds(void)
> +{
> +	struct fixture *fix = init_fixture();
> +	char command[512];
> +	char input_file[80];
> +	const char *vars = "var_s16 = -32768\nvar_u16 = 60652";
> +	int fd;
> +
> +	snprintf(input_file, sizeof(input_file), "/tmp/veristat_input.XXXXXX");
> +	fd = mkstemp(input_file);
> +	if (!ASSERT_GT(fd, 0, "valid fd"))

Nit: ASSERT_GE.

> +		goto out;
> +
> +	write(fd, vars, strlen(vars));
> +	snprintf(command, sizeof(command),
> +		 "./veristat set_global_vars.bpf.o -G \"@%s\" -vl2 > %s",
> +		 input_file, fix->tmpfile);
> +
> +	ASSERT_EQ(0, system(command), "command");
> +	read(fix->fd, fix->output, fix->sz);
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0x8000 "), "var_s16 = -32768");
> +	ASSERT_NEQ(NULL, strstr(fix->output, "_w=0xecec "), "var_u16 = 60652");
> +
> +out:
> +	close(fd);
> +	remove(input_file);
> +	teardown_fixture(fix);
> +}

[...]