[PATCH bpf-next 0/2] selftests/bpf: test_progs can read test lists from file

[PATCH bpf-next 0/2] selftests/bpf: test_progs can read test lists from file

[Stephen Veiss]

Hi,

BPF selftests have ALLOWLIST and DENYLIST files, used to control which
tests are run in CI. These files are currently parsed by a shell
script. [1]

This patchset allows those files to be specified directly on the
test_progs command line (eg, as -a @ALLOWLIST). 

This also fixes a bug in the existing test filter code causing
unnecessary duplicate top-level test filter entries to be created. 

Thanks,

Stephen

[1] https://github.com/kernel-patches/vmtest/blob/57feb460047b69f891cf4afe3cc860794a2ced17/ci/vmtest/run_selftests.sh#L21-L27

Stephen Veiss (2):
  selftests/bpf: extract insert_test from parse_test_list
  selftests/bpf: test_progs can read test lists from file

 .../selftests/bpf/prog_tests/arg_parsing.c    |  63 +++++
 tools/testing/selftests/bpf/test_progs.c      |  39 ++-
 tools/testing/selftests/bpf/testing_helpers.c | 225 ++++++++++++------
 tools/testing/selftests/bpf/testing_helpers.h |   3 +
 4 files changed, 249 insertions(+), 81 deletions(-)


base-commit: a0c109dcafb15b8bee187c49fb746779374f60f0
-- 
2.34.1

[PATCH bpf-next 1/2] selftests/bpf: extract insert_test from parse_test_list

[Stephen Veiss]

Split the logic to insert new tests into test filter sets out from
parse_test_list.

Fix the subtest insertion logic to reuse an existing top-level test
filter, which prevents the creation of duplicate top-level test filters
each with a single subtest.

Signed-off-by: Stephen Veiss <sveiss@meta.com>
---
 .../selftests/bpf/prog_tests/arg_parsing.c    |  13 ++
 tools/testing/selftests/bpf/testing_helpers.c | 176 +++++++++++-------
 2 files changed, 117 insertions(+), 72 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
index b17bfa0e0aac..3754cd5f8c0a 100644
--- a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
+++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
@@ -96,6 +96,19 @@ static void test_parse_test_list(void)
 		goto error;
 	ASSERT_OK(strcmp("*bpf_cookie*", set.tests[0].name), "test name");
 	ASSERT_OK(strcmp("*trace*", set.tests[0].subtests[0]), "subtest name");
+	free_test_filter_set(&set);
+
+	ASSERT_OK(parse_test_list("t/subtest1,t/subtest2", &set, true),
+		  "parsing");
+	if (!ASSERT_EQ(set.cnt, 1, "count of test filters"))
+		goto error;
+	if (!ASSERT_OK_PTR(set.tests, "test filters initialized"))
+		goto error;
+	if (!ASSERT_EQ(set.tests[0].subtest_cnt, 2, "subtest filters count"))
+		goto error;
+	ASSERT_OK(strcmp("t", set.tests[0].name), "test name");
+	ASSERT_OK(strcmp("subtest1", set.tests[0].subtests[0]), "subtest name");
+	ASSERT_OK(strcmp("subtest2", set.tests[0].subtests[1]), "subtest name");
 error:
 	free_test_filter_set(&set);
 }
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 0b5e0829e5be..14322371e1d8 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -70,92 +70,124 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len)
 	return 0;
 }
 
+static int do_insert_test(struct test_filter_set *set,
+			  char *test_str,
+			  char *subtest_str)
+{
+	struct test_filter *tmp, *test;
+	char **ctmp;
+	int i;
+
+	for (i = 0; i < set->cnt; i++) {
+		test = &set->tests[i];
+
+		if (strcmp(test_str, test->name) == 0) {
+			free(test_str);
+			goto subtest;
+		}
+	}
+
+	tmp = realloc(set->tests, sizeof(*test) * (set->cnt + 1));
+	if (!tmp)
+		return -ENOMEM;
+
+	set->tests = tmp;
+	test = &set->tests[set->cnt];
+
+	test->name = test_str;
+	test->subtests = NULL;
+	test->subtest_cnt = 0;
+
+	set->cnt++;
+
+subtest:
+	if (!subtest_str)
+		return 0;
+
+	for (i = 0; i < test->subtest_cnt; i++) {
+		if (strcmp(subtest_str, test->subtests[i]) == 0) {
+			free(subtest_str);
+			return 0;
+		}
+	}
+
+	ctmp = realloc(test->subtests,
+		       sizeof(*test->subtests) * (test->subtest_cnt + 1));
+	if (!ctmp)
+		return -ENOMEM;
+
+	test->subtests = ctmp;
+	test->subtests[test->subtest_cnt] = subtest_str;
+
+	test->subtest_cnt++;
+
+	return 0;
+}
+
+static int insert_test(struct test_filter_set *set,
+		       char *test_spec,
+		       bool is_glob_pattern)
+{
+	char *pattern, *subtest_str, *ext_test_str, *ext_subtest_str = NULL;
+	int glob_chars = 0;
+
+	if (is_glob_pattern) {
+		pattern = "%s";
+	} else {
+		pattern = "*%s*";
+		glob_chars = 2;
+	}
+
+	subtest_str = strchr(test_spec, '/');
+	if (subtest_str) {
+		*subtest_str = '\0';
+		subtest_str += 1;
+	}
+
+	ext_test_str = malloc(strlen(test_spec) + glob_chars + 1);
+	if (!ext_test_str)
+		goto err;
+
+	sprintf(ext_test_str, pattern, test_spec);
+
+	if (subtest_str) {
+		ext_subtest_str = malloc(strlen(subtest_str) + glob_chars + 1);
+		if (!ext_subtest_str)
+			goto err;
+
+		sprintf(ext_subtest_str, pattern, subtest_str);
+	}
+
+	return do_insert_test(set, ext_test_str, ext_subtest_str);
+
+err:
+	free(ext_test_str);
+	free(ext_subtest_str);
+
+	return -ENOMEM;
+}
+
 int parse_test_list(const char *s,
 		    struct test_filter_set *set,
 		    bool is_glob_pattern)
 {
-	char *input, *state = NULL, *next;
-	struct test_filter *tmp, *tests = NULL;
-	int i, j, cnt = 0;
+	char *input, *state = NULL, *test_spec;
+	int err;
 
 	input = strdup(s);
 	if (!input)
 		return -ENOMEM;
 
-	while ((next = strtok_r(state ? NULL : input, ",", &state))) {
-		char *subtest_str = strchr(next, '/');
-		char *pattern = NULL;
-		int glob_chars = 0;
-
-		tmp = realloc(tests, sizeof(*tests) * (cnt + 1));
-		if (!tmp)
-			goto err;
-		tests = tmp;
-
-		tests[cnt].subtest_cnt = 0;
-		tests[cnt].subtests = NULL;
-
-		if (is_glob_pattern) {
-			pattern = "%s";
-		} else {
-			pattern = "*%s*";
-			glob_chars = 2;
+	while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
+		err = insert_test(set, test_spec, is_glob_pattern);
+		if (err) {
+			free(input);
+			return err;
 		}
-
-		if (subtest_str) {
-			char **tmp_subtests = NULL;
-			int subtest_cnt = tests[cnt].subtest_cnt;
-
-			*subtest_str = '\0';
-			subtest_str += 1;
-			tmp_subtests = realloc(tests[cnt].subtests,
-					       sizeof(*tmp_subtests) *
-					       (subtest_cnt + 1));
-			if (!tmp_subtests)
-				goto err;
-			tests[cnt].subtests = tmp_subtests;
-
-			tests[cnt].subtests[subtest_cnt] =
-				malloc(strlen(subtest_str) + glob_chars + 1);
-			if (!tests[cnt].subtests[subtest_cnt])
-				goto err;
-			sprintf(tests[cnt].subtests[subtest_cnt],
-				pattern,
-				subtest_str);
-
-			tests[cnt].subtest_cnt++;
-		}
-
-		tests[cnt].name = malloc(strlen(next) + glob_chars + 1);
-		if (!tests[cnt].name)
-			goto err;
-		sprintf(tests[cnt].name, pattern, next);
-
-		cnt++;
 	}
 
-	tmp = realloc(set->tests, sizeof(*tests) * (cnt + set->cnt));
-	if (!tmp)
-		goto err;
-
-	memcpy(tmp +  set->cnt, tests, sizeof(*tests) * cnt);
-	set->tests = tmp;
-	set->cnt += cnt;
-
-	free(tests);
 	free(input);
 	return 0;
-
-err:
-	for (i = 0; i < cnt; i++) {
-		for (j = 0; j < tests[i].subtest_cnt; j++)
-			free(tests[i].subtests[j]);
-
-		free(tests[i].name);
-	}
-	free(tests);
-	free(input);
-	return -ENOMEM;
 }
 
 __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info)
-- 
2.34.1

[PATCH bpf-next 2/2] selftests/bpf: test_progs can read test lists from file

[Stephen Veiss]

Improve test selection logic when using -a/-b/-d/-t options.
The list of tests to include or exclude can now be read from a file,
specified as @<filename>.

The file contains one name (or wildcard pattern) per line, and
comments beginning with # are ignored.

These options can be passed multiple times to read more than one file.

Signed-off-by: Stephen Veiss <sveiss@meta.com>
---
 .../selftests/bpf/prog_tests/arg_parsing.c    | 50 +++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.c      | 39 +++++++++++----
 tools/testing/selftests/bpf/testing_helpers.c | 49 ++++++++++++++++++
 tools/testing/selftests/bpf/testing_helpers.h |  3 ++
 4 files changed, 132 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
index 3754cd5f8c0a..e0c6ef2dda70 100644
--- a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
+++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
@@ -113,8 +113,58 @@ static void test_parse_test_list(void)
 	free_test_filter_set(&set);
 }
 
+static void test_parse_test_list_file(void)
+{
+	char tmpfile[80];
+	int fd;
+	FILE *fp;
+	struct test_filter_set set;
+
+	snprintf(tmpfile, sizeof(tmpfile), "/tmp/bpf_arg_parsing_test.XXXXXX");
+	fd = mkstemp(tmpfile);
+	ASSERT_GE(fd, 0, "create tmp");
+
+	fp = fdopen(fd, "w");
+
+	fprintf(fp, "# comment\n");
+	fprintf(fp, "  test_with_spaces    \n");
+	fprintf(fp, "testA/subtest    # comment\n");
+	fprintf(fp, "testB#comment with no space\n");
+	fprintf(fp, "testB # duplicate\n");
+	fprintf(fp, "testA/subtest # subtest duplicate\n");
+	fprintf(fp, "testA/subtest2\n");
+	fprintf(fp, "testC_no_eof_newline");
+
+	if (!ASSERT_OK(ferror(fp), "prepare tmp"))
+		goto error;
+
+	if (!ASSERT_OK(fclose(fp), "close tmp"))
+		goto error;
+
+	init_test_filter_set(&set);
+
+	ASSERT_OK(parse_test_list_file(tmpfile, &set, true), "parse file");
+
+	ASSERT_EQ(set.cnt, 4, "test  count");
+	ASSERT_OK(strcmp("test_with_spaces", set.tests[0].name), "test 0 name");
+	ASSERT_EQ(set.tests[0].subtest_cnt, 0, "test 0 subtest count");
+	ASSERT_OK(strcmp("testA", set.tests[1].name), "test 1 name");
+	ASSERT_EQ(set.tests[1].subtest_cnt, 2, "test 1 subtest count");
+	ASSERT_OK(strcmp("subtest", set.tests[1].subtests[0]), "test 1 subtest 0");
+	ASSERT_OK(strcmp("subtest2", set.tests[1].subtests[1]), "test 1 subtest 1");
+	ASSERT_OK(strcmp("testB", set.tests[2].name), "test 2 name");
+	ASSERT_OK(strcmp("testC_no_eof_newline", set.tests[3].name), "test 3 name");
+
+	free_test_filter_set(&set);
+
+error:
+	remove(tmpfile);
+}
+
 void test_arg_parsing(void)
 {
 	if (test__start_subtest("test_parse_test_list"))
 		test_parse_test_list();
+	if (test__start_subtest("test_parse_test_list_file"))
+		test_parse_test_list_file();
 }
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index ea82921110da..cf80d28c76e8 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -714,7 +714,13 @@ static struct test_state test_states[ARRAY_SIZE(prog_test_defs)];
 
 const char *argp_program_version = "test_progs 0.1";
 const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
-static const char argp_program_doc[] = "BPF selftests test runner";
+static const char argp_program_doc[] =
+"BPF selftests test runner\v"
+"Options accepting the NAMES parameter take either a comma-separated list\n"
+"of test names, or a filename prefixed with @. The file contains one name\n"
+"(or wildcard pattern) per line, and comments beginning with # are ignored.\n"
+"\n"
+"These options can be passed repeatedly to read multiple files.\n";
 
 enum ARG_KEYS {
 	ARG_TEST_NUM = 'n',
@@ -797,6 +803,7 @@ extern int extra_prog_load_log_flags;
 static error_t parse_arg(int key, char *arg, struct argp_state *state)
 {
 	struct test_env *env = state->input;
+	int err;
 
 	switch (key) {
 	case ARG_TEST_NUM: {
@@ -821,18 +828,32 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	}
 	case ARG_TEST_NAME_GLOB_ALLOWLIST:
 	case ARG_TEST_NAME: {
-		if (parse_test_list(arg,
-				    &env->test_selector.whitelist,
-				    key == ARG_TEST_NAME_GLOB_ALLOWLIST))
-			return -ENOMEM;
+		if (arg[0] == '@')
+			err = parse_test_list_file(arg + 1,
+						   &env->test_selector.whitelist,
+						   key == ARG_TEST_NAME_GLOB_ALLOWLIST);
+		else
+			err = parse_test_list(arg,
+					      &env->test_selector.whitelist,
+					      key == ARG_TEST_NAME_GLOB_ALLOWLIST);
+
+		if (err)
+			return err;
 		break;
 	}
 	case ARG_TEST_NAME_GLOB_DENYLIST:
 	case ARG_TEST_NAME_BLACKLIST: {
-		if (parse_test_list(arg,
-				    &env->test_selector.blacklist,
-				    key == ARG_TEST_NAME_GLOB_DENYLIST))
-			return -ENOMEM;
+		if (arg[0] == '@')
+			err = parse_test_list_file(arg + 1,
+						   &env->test_selector.blacklist,
+						   key == ARG_TEST_NAME_GLOB_DENYLIST);
+		else
+			err = parse_test_list(arg,
+					      &env->test_selector.blacklist,
+					      key == ARG_TEST_NAME_GLOB_DENYLIST);
+
+		if (err)
+			return err;
 		break;
 	}
 	case ARG_VERIFIER_STATS:
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 14322371e1d8..d8bea5c2a4d6 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
 /* Copyright (C) 2019 Netronome Systems, Inc. */
 /* Copyright (C) 2020 Facebook, Inc. */
+#include <ctype.h>
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
@@ -167,6 +168,54 @@ static int insert_test(struct test_filter_set *set,
 	return -ENOMEM;
 }
 
+int parse_test_list_file(const char *path,
+			 struct test_filter_set *set,
+			 bool is_glob_pattern)
+{
+	FILE *f;
+	size_t buflen = 0;
+	char *buf = NULL, *capture_start, *capture_end, *scan_end;
+	int err;
+
+	f = fopen(path, "r");
+	if (!f) {
+		err = -errno;
+		fprintf(stderr, "Failed to open '%s': %d\n", path, err);
+		return err;
+	}
+
+	while (getline(&buf, &buflen, f) != -1) {
+		capture_start = buf;
+
+		while (isspace(*capture_start))
+			++capture_start;
+
+		capture_end = capture_start;
+		scan_end = capture_start;
+
+		while (*scan_end && *scan_end != '#') {
+			if (!isspace(*scan_end))
+				capture_end = scan_end;
+
+			++scan_end;
+		}
+
+		if (capture_end == capture_start)
+			continue;
+
+		*(++capture_end) = '\0';
+
+		err = insert_test(set, capture_start, is_glob_pattern);
+		if (err) {
+			fclose(f);
+			return err;
+		}
+	}
+
+	fclose(f);
+	return 0;
+}
+
 int parse_test_list(const char *s,
 		    struct test_filter_set *set,
 		    bool is_glob_pattern)
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index eb8790f928e4..98f09bbae86f 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -20,5 +20,8 @@ struct test_filter_set;
 int parse_test_list(const char *s,
 		    struct test_filter_set *test_set,
 		    bool is_glob_pattern);
+int parse_test_list_file(const char *path,
+			 struct test_filter_set *test_set,
+			 bool is_glob_pattern);
 
 __u64 read_perf_max_sample_freq(void);
-- 
2.34.1

Re: [PATCH bpf-next 1/2] selftests/bpf: extract insert_test from parse_test_list

[Yonghong Song]

On 4/25/23 3:54 PM, Stephen Veiss wrote:
> Split the logic to insert new tests into test filter sets out from
> parse_test_list.
> 
> Fix the subtest insertion logic to reuse an existing top-level test
> filter, which prevents the creation of duplicate top-level test filters
> each with a single subtest.
> 
> Signed-off-by: Stephen Veiss <sveiss@meta.com>
> ---
>   .../selftests/bpf/prog_tests/arg_parsing.c    |  13 ++
>   tools/testing/selftests/bpf/testing_helpers.c | 176 +++++++++++-------
>   2 files changed, 117 insertions(+), 72 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> index b17bfa0e0aac..3754cd5f8c0a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> +++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> @@ -96,6 +96,19 @@ static void test_parse_test_list(void)
>   		goto error;
>   	ASSERT_OK(strcmp("*bpf_cookie*", set.tests[0].name), "test name");
>   	ASSERT_OK(strcmp("*trace*", set.tests[0].subtests[0]), "subtest name");
> +	free_test_filter_set(&set);
> +
> +	ASSERT_OK(parse_test_list("t/subtest1,t/subtest2", &set, true),
> +		  "parsing");
> +	if (!ASSERT_EQ(set.cnt, 1, "count of test filters"))
> +		goto error;
> +	if (!ASSERT_OK_PTR(set.tests, "test filters initialized"))
> +		goto error;
> +	if (!ASSERT_EQ(set.tests[0].subtest_cnt, 2, "subtest filters count"))
> +		goto error;
> +	ASSERT_OK(strcmp("t", set.tests[0].name), "test name");
> +	ASSERT_OK(strcmp("subtest1", set.tests[0].subtests[0]), "subtest name");
> +	ASSERT_OK(strcmp("subtest2", set.tests[0].subtests[1]), "subtest name");
>   error:
>   	free_test_filter_set(&set);
>   }
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index 0b5e0829e5be..14322371e1d8 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -70,92 +70,124 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len)
>   	return 0;
>   }
>   
> +static int do_insert_test(struct test_filter_set *set,
> +			  char *test_str,
> +			  char *subtest_str)
> +{
> +	struct test_filter *tmp, *test;
> +	char **ctmp;
> +	int i;
> +
> +	for (i = 0; i < set->cnt; i++) {
> +		test = &set->tests[i];
> +
> +		if (strcmp(test_str, test->name) == 0) {
> +			free(test_str);
> +			goto subtest;
> +		}
> +	}
> +
> +	tmp = realloc(set->tests, sizeof(*test) * (set->cnt + 1));
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	set->tests = tmp;
> +	test = &set->tests[set->cnt];
> +
> +	test->name = test_str;
> +	test->subtests = NULL;
> +	test->subtest_cnt = 0;
> +
> +	set->cnt++;
> +
> +subtest:
> +	if (!subtest_str)
> +		return 0;
> +
> +	for (i = 0; i < test->subtest_cnt; i++) {
> +		if (strcmp(subtest_str, test->subtests[i]) == 0) {
> +			free(subtest_str);
> +			return 0;
> +		}
> +	}
> +
> +	ctmp = realloc(test->subtests,
> +		       sizeof(*test->subtests) * (test->subtest_cnt + 1));
> +	if (!ctmp)
> +		return -ENOMEM;
> +
> +	test->subtests = ctmp;
> +	test->subtests[test->subtest_cnt] = subtest_str;
> +
> +	test->subtest_cnt++;
> +
> +	return 0;
> +}
> +
> +static int insert_test(struct test_filter_set *set,
> +		       char *test_spec,
> +		       bool is_glob_pattern)
> +{
> +	char *pattern, *subtest_str, *ext_test_str, *ext_subtest_str = NULL;
> +	int glob_chars = 0;
> +
> +	if (is_glob_pattern) {
> +		pattern = "%s";
> +	} else {
> +		pattern = "*%s*";
> +		glob_chars = 2;
> +	}
> +
> +	subtest_str = strchr(test_spec, '/');
> +	if (subtest_str) {
> +		*subtest_str = '\0';
> +		subtest_str += 1;
> +	}
> +
> +	ext_test_str = malloc(strlen(test_spec) + glob_chars + 1);
> +	if (!ext_test_str)
> +		goto err;
> +
> +	sprintf(ext_test_str, pattern, test_spec);
> +
> +	if (subtest_str) {
> +		ext_subtest_str = malloc(strlen(subtest_str) + glob_chars + 1);
> +		if (!ext_subtest_str)
> +			goto err;
> +
> +		sprintf(ext_subtest_str, pattern, subtest_str);
> +	}
> +
> +	return do_insert_test(set, ext_test_str, ext_subtest_str);
> +
> +err:
> +	free(ext_test_str);
> +	free(ext_subtest_str);
> +
> +	return -ENOMEM;
> +}
> +
>   int parse_test_list(const char *s,
>   		    struct test_filter_set *set,
>   		    bool is_glob_pattern)
>   {
> -	char *input, *state = NULL, *next;
> -	struct test_filter *tmp, *tests = NULL;
> -	int i, j, cnt = 0;
> +	char *input, *state = NULL, *test_spec;
> +	int err;
>   
>   	input = strdup(s);
>   	if (!input)
>   		return -ENOMEM;
>   
> -	while ((next = strtok_r(state ? NULL : input, ",", &state))) {
> -		char *subtest_str = strchr(next, '/');
> -		char *pattern = NULL;
> -		int glob_chars = 0;
> -
> -		tmp = realloc(tests, sizeof(*tests) * (cnt + 1));
> -		if (!tmp)
> -			goto err;
> -		tests = tmp;
> -
> -		tests[cnt].subtest_cnt = 0;
> -		tests[cnt].subtests = NULL;
> -
> -		if (is_glob_pattern) {
> -			pattern = "%s";
> -		} else {
> -			pattern = "*%s*";
> -			glob_chars = 2;
> +	while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
> +		err = insert_test(set, test_spec, is_glob_pattern);
> +		if (err) {
> +			free(input);
> +			return err;
>   		}
> -
> -		if (subtest_str) {
> -			char **tmp_subtests = NULL;
> -			int subtest_cnt = tests[cnt].subtest_cnt;
> -
> -			*subtest_str = '\0';
> -			subtest_str += 1;
> -			tmp_subtests = realloc(tests[cnt].subtests,
> -					       sizeof(*tmp_subtests) *
> -					       (subtest_cnt + 1));
> -			if (!tmp_subtests)
> -				goto err;
> -			tests[cnt].subtests = tmp_subtests;
> -
> -			tests[cnt].subtests[subtest_cnt] =
> -				malloc(strlen(subtest_str) + glob_chars + 1);
> -			if (!tests[cnt].subtests[subtest_cnt])
> -				goto err;
> -			sprintf(tests[cnt].subtests[subtest_cnt],
> -				pattern,
> -				subtest_str);
> -
> -			tests[cnt].subtest_cnt++;
> -		}
> -
> -		tests[cnt].name = malloc(strlen(next) + glob_chars + 1);
> -		if (!tests[cnt].name)
> -			goto err;
> -		sprintf(tests[cnt].name, pattern, next);
> -
> -		cnt++;
>   	}
>   
> -	tmp = realloc(set->tests, sizeof(*tests) * (cnt + set->cnt));
> -	if (!tmp)
> -		goto err;
> -
> -	memcpy(tmp +  set->cnt, tests, sizeof(*tests) * cnt);
> -	set->tests = tmp;
> -	set->cnt += cnt;
> -
> -	free(tests);
>   	free(input);
>   	return 0;
> -
> -err:
> -	for (i = 0; i < cnt; i++) {
> -		for (j = 0; j < tests[i].subtest_cnt; j++)
> -			free(tests[i].subtests[j]);
> -
> -		free(tests[i].name);
> -	}
> -	free(tests);
> -	free(input);
> -	return -ENOMEM;
>   }

LGTM.
For parse_test_list() function, maybe using the following code which
is more in kernel coding style.

int parse_test_list(const char *s,
                     struct test_filter_set *set,
                     bool is_glob_pattern)
{
         char *input, *state = NULL, *test_spec;
         int err = 0;

         input = strdup(s);
         if (!input)
                 return -ENOMEM;

         while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
                 err = insert_test(set, test_spec, is_glob_pattern);
                 if (err)
                         break;
         }

         free(input);
         return err;
}

>   
>   __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info)

Re: [PATCH bpf-next 2/2] selftests/bpf: test_progs can read test lists from file

[Yonghong Song]

On 4/25/23 3:54 PM, Stephen Veiss wrote:
> Improve test selection logic when using -a/-b/-d/-t options.
> The list of tests to include or exclude can now be read from a file,
> specified as @<filename>.
> 
> The file contains one name (or wildcard pattern) per line, and
> comments beginning with # are ignored.
> 
> These options can be passed multiple times to read more than one file.
> 
> Signed-off-by: Stephen Veiss <sveiss@meta.com>

LGTM. A few nits below.

> ---
>   .../selftests/bpf/prog_tests/arg_parsing.c    | 50 +++++++++++++++++++
>   tools/testing/selftests/bpf/test_progs.c      | 39 +++++++++++----
>   tools/testing/selftests/bpf/testing_helpers.c | 49 ++++++++++++++++++
>   tools/testing/selftests/bpf/testing_helpers.h |  3 ++
>   4 files changed, 132 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> index 3754cd5f8c0a..e0c6ef2dda70 100644
> --- a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> +++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
> @@ -113,8 +113,58 @@ static void test_parse_test_list(void)
>   	free_test_filter_set(&set);
>   }
>   
> +static void test_parse_test_list_file(void)
> +{
> +	char tmpfile[80];
> +	int fd;
> +	FILE *fp;
> +	struct test_filter_set set;
> +
> +	snprintf(tmpfile, sizeof(tmpfile), "/tmp/bpf_arg_parsing_test.XXXXXX");
> +	fd = mkstemp(tmpfile);
> +	ASSERT_GE(fd, 0, "create tmp");

If ASSERT_GE(...) is not true, we should simply return.

> +
> +	fp = fdopen(fd, "w");

We should check whether fp is NULL or not, if it is we should close fd 
and return (basically go to 'error').

> +
> +	fprintf(fp, "# comment\n");
> +	fprintf(fp, "  test_with_spaces    \n");
> +	fprintf(fp, "testA/subtest    # comment\n");
> +	fprintf(fp, "testB#comment with no space\n");
> +	fprintf(fp, "testB # duplicate\n");
> +	fprintf(fp, "testA/subtest # subtest duplicate\n");
> +	fprintf(fp, "testA/subtest2\n");
> +	fprintf(fp, "testC_no_eof_newline");
> +
> +	if (!ASSERT_OK(ferror(fp), "prepare tmp"))
> +		goto error;
> +
> +	if (!ASSERT_OK(fclose(fp), "close tmp"))
> +		goto error;
> +
> +	init_test_filter_set(&set);
> +
> +	ASSERT_OK(parse_test_list_file(tmpfile, &set, true), "parse file");
> +
> +	ASSERT_EQ(set.cnt, 4, "test  count");
> +	ASSERT_OK(strcmp("test_with_spaces", set.tests[0].name), "test 0 name");
> +	ASSERT_EQ(set.tests[0].subtest_cnt, 0, "test 0 subtest count");
> +	ASSERT_OK(strcmp("testA", set.tests[1].name), "test 1 name");
> +	ASSERT_EQ(set.tests[1].subtest_cnt, 2, "test 1 subtest count");
> +	ASSERT_OK(strcmp("subtest", set.tests[1].subtests[0]), "test 1 subtest 0");
> +	ASSERT_OK(strcmp("subtest2", set.tests[1].subtests[1]), "test 1 subtest 1");
> +	ASSERT_OK(strcmp("testB", set.tests[2].name), "test 2 name");
> +	ASSERT_OK(strcmp("testC_no_eof_newline", set.tests[3].name), "test 3 name");
> +
> +	free_test_filter_set(&set);
> +
> +error:
> +	remove(tmpfile);

Maybe do 'close(fd)' instead which is more intuitive?

> +}
> +
>   void test_arg_parsing(void)
>   {
>   	if (test__start_subtest("test_parse_test_list"))
>   		test_parse_test_list();
> +	if (test__start_subtest("test_parse_test_list_file"))
> +		test_parse_test_list_file();
>   }
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index ea82921110da..cf80d28c76e8 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -714,7 +714,13 @@ static struct test_state test_states[ARRAY_SIZE(prog_test_defs)];
>   
>   const char *argp_program_version = "test_progs 0.1";
>   const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
> -static const char argp_program_doc[] = "BPF selftests test runner";
> +static const char argp_program_doc[] =
> +"BPF selftests test runner\v"

What does it mean to use "\v" here?

> +"Options accepting the NAMES parameter take either a comma-separated list\n"
> +"of test names, or a filename prefixed with @. The file contains one name\n"
> +"(or wildcard pattern) per line, and comments beginning with # are ignored.\n"
> +"\n"
> +"These options can be passed repeatedly to read multiple files.\n";
>   
>   enum ARG_KEYS {
>   	ARG_TEST_NUM = 'n',
> @@ -797,6 +803,7 @@ extern int extra_prog_load_log_flags;
>   static error_t parse_arg(int key, char *arg, struct argp_state *state)
>   {
>   	struct test_env *env = state->input;
> +	int err;

Maybe initialize 'err = 0' and change later 'return 0' to 'return err',
so we don't need 'if (err) return err' below?

>   
>   	switch (key) {
>   	case ARG_TEST_NUM: {
> @@ -821,18 +828,32 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>   	}
>   	case ARG_TEST_NAME_GLOB_ALLOWLIST:
>   	case ARG_TEST_NAME: {
> -		if (parse_test_list(arg,
> -				    &env->test_selector.whitelist,
> -				    key == ARG_TEST_NAME_GLOB_ALLOWLIST))
> -			return -ENOMEM;
> +		if (arg[0] == '@')
> +			err = parse_test_list_file(arg + 1,
> +						   &env->test_selector.whitelist,
> +						   key == ARG_TEST_NAME_GLOB_ALLOWLIST);
> +		else
> +			err = parse_test_list(arg,
> +					      &env->test_selector.whitelist,
> +					      key == ARG_TEST_NAME_GLOB_ALLOWLIST);
> +
> +		if (err)
> +			return err;
>   		break;
>   	}
>   	case ARG_TEST_NAME_GLOB_DENYLIST:
>   	case ARG_TEST_NAME_BLACKLIST: {
> -		if (parse_test_list(arg,
> -				    &env->test_selector.blacklist,
> -				    key == ARG_TEST_NAME_GLOB_DENYLIST))
> -			return -ENOMEM;
> +		if (arg[0] == '@')
> +			err = parse_test_list_file(arg + 1,
> +						   &env->test_selector.blacklist,
> +						   key == ARG_TEST_NAME_GLOB_DENYLIST);
> +		else
> +			err = parse_test_list(arg,
> +					      &env->test_selector.blacklist,
> +					      key == ARG_TEST_NAME_GLOB_DENYLIST);
> +
> +		if (err)
> +			return err;
>   		break;
>   	}
>   	case ARG_VERIFIER_STATS:
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index 14322371e1d8..d8bea5c2a4d6 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
>   /* Copyright (C) 2019 Netronome Systems, Inc. */
>   /* Copyright (C) 2020 Facebook, Inc. */
> +#include <ctype.h>
>   #include <stdlib.h>
>   #include <string.h>
>   #include <errno.h>
> @@ -167,6 +168,54 @@ static int insert_test(struct test_filter_set *set,
>   	return -ENOMEM;
>   }
>   
> +int parse_test_list_file(const char *path,
> +			 struct test_filter_set *set,
> +			 bool is_glob_pattern)
> +{
> +	FILE *f;
> +	size_t buflen = 0;
> +	char *buf = NULL, *capture_start, *capture_end, *scan_end;
> +	int err;
> +
> +	f = fopen(path, "r");
> +	if (!f) {
> +		err = -errno;
> +		fprintf(stderr, "Failed to open '%s': %d\n", path, err);
> +		return err;
> +	}
> +
> +	while (getline(&buf, &buflen, f) != -1) {
> +		capture_start = buf;
> +
> +		while (isspace(*capture_start))
> +			++capture_start;
> +
> +		capture_end = capture_start;
> +		scan_end = capture_start;
> +
> +		while (*scan_end && *scan_end != '#') {
> +			if (!isspace(*scan_end))
> +				capture_end = scan_end;
> +
> +			++scan_end;
> +		}
> +
> +		if (capture_end == capture_start)
> +			continue;
> +
> +		*(++capture_end) = '\0';
> +
> +		err = insert_test(set, capture_start, is_glob_pattern);
> +		if (err) {
> +			fclose(f);
> +			return err;

Set initial err = 0 and later 'return 0' => 'return err', so we
just do break here to avoid calling fclose(f) in two different
places?

> +		}
> +	}
> +
> +	fclose(f);
> +	return 0;
> +}
> +
>   int parse_test_list(const char *s,
>   		    struct test_filter_set *set,
>   		    bool is_glob_pattern)
> diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
> index eb8790f928e4..98f09bbae86f 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.h
> +++ b/tools/testing/selftests/bpf/testing_helpers.h
> @@ -20,5 +20,8 @@ struct test_filter_set;
>   int parse_test_list(const char *s,
>   		    struct test_filter_set *test_set,
>   		    bool is_glob_pattern);
> +int parse_test_list_file(const char *path,
> +			 struct test_filter_set *test_set,
> +			 bool is_glob_pattern);
>   
>   __u64 read_perf_max_sample_freq(void);

Re: [PATCH bpf-next 2/2] selftests/bpf: test_progs can read test lists from file

[Stephen Veiss]

On Tue, Apr 25, 2023 at 09:25:40PM -0700, Yonghong Song wrote:
> On 4/25/23 3:54 PM, Stephen Veiss wrote:
> > +static const char argp_program_doc[] =
> > +"BPF selftests test runner\v"
> 
> What does it mean to use "\v" here?

argp splits the documentation string on \v. The part before \v shows
up at the start of the --help output, while the part after appears
after the detailed help text for the arguments. [1]

Happy to take all your other suggestions; I'll revise and resend the
patch series later in the week.

Thanks,

Stephen

[1] https://www.gnu.org/software/libc/manual/html_node/Argp-Parsers.html

Re: [PATCH bpf-next 2/2] selftests/bpf: test_progs can read test lists from file

[Yonghong Song]

On 4/26/23 9:10 AM, Stephen Veiss wrote:
> On Tue, Apr 25, 2023 at 09:25:40PM -0700, Yonghong Song wrote:
>> On 4/25/23 3:54 PM, Stephen Veiss wrote:
>>> +static const char argp_program_doc[] =
>>> +"BPF selftests test runner\v"
>>
>> What does it mean to use "\v" here?
> 
> argp splits the documentation string on \v. The part before \v shows
> up at the start of the --help output, while the part after appears
> after the detailed help text for the arguments. [1]

sounds good. Thanks for explanations!

> 
> Happy to take all your other suggestions; I'll revise and resend the
> patch series later in the week.
> 
> Thanks,
> 
> Stephen
> 
> [1] https://www.gnu.org/software/libc/manual/html_node/Argp-Parsers.html