[PATCH v3 bpf-next 0/4] selftests/bpf: Improve the usability of test_progs

[PATCH v3 bpf-next 0/4] selftests/bpf: Improve the usability of test_progs

[Yucong Sun]

This short series adds two new "-a", "-d" switch to test_progs,
supporting exact string match, as well as '*' wildchar. It also cleans
up the output to make it possible to generate allowlist/denylist using
grep.

Yucong Sun (4):
  selftests/bpf: skip loading bpf_testmod when using -l to list tests.
  selftests/bpf: correctly display subtest skip status
  selftests/bpf: Support glob matching for test selector.
  selftests/bpf: also print test name in subtest status message

 tools/testing/selftests/bpf/test_progs.c | 100 +++++++++++++++++------
 tools/testing/selftests/bpf/test_progs.h |   1 +
 2 files changed, 78 insertions(+), 23 deletions(-)

-- 
2.30.2

[PATCH v3 bpf-next 1/4] selftests/bpf: skip loading bpf_testmod when using -l to list tests.

[Yucong Sun]

When using "-l", test_progs often is executed as non-root user,
load_bpf_testmod() will fail and output errors. This patch skip loading bpf
testmod when "-l" is specified, making output cleaner.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6f103106a39b..532af3353edf 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -755,7 +755,7 @@ int main(int argc, char **argv)
 	save_netns();
 	stdio_hijack();
 	env.has_testmod = true;
-	if (load_bpf_testmod()) {
+	if (!env.list_test_names && load_bpf_testmod()) {
 		fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
 		env.has_testmod = false;
 	}
@@ -803,7 +803,7 @@ int main(int argc, char **argv)
 		if (test->need_cgroup_cleanup)
 			cleanup_cgroup_environment();
 	}
-	if (env.has_testmod)
+	if (!env.list_test_names && env.has_testmod)
 		unload_bpf_testmod();
 	stdio_restore();
 
-- 
2.30.2

[PATCH v3 bpf-next 2/4] selftests/bpf: correctly display subtest skip status

[Yucong Sun]

In skip_account(), test->skip_cnt is set to 0 at the end, this makes next print
statement never display SKIP status for the subtest. This patch moves the
accounting logic after the print statement, fixing the issue.

This patch also added SKIP status display for normal tests.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 532af3353edf..f0fbead40883 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -148,18 +148,18 @@ void test__end_subtest()
 	struct prog_test_def *test = env.test;
 	int sub_error_cnt = test->error_cnt - test->old_error_cnt;
 
-	if (sub_error_cnt)
-		env.fail_cnt++;
-	else if (test->skip_cnt == 0)
-		env.sub_succ_cnt++;
-	skip_account();
-
 	dump_test_log(test, sub_error_cnt);
 
 	fprintf(env.stdout, "#%d/%d %s:%s\n",
 	       test->test_num, test->subtest_num, test->subtest_name,
 	       sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
 
+	if (sub_error_cnt)
+		env.fail_cnt++;
+	else if (test->skip_cnt == 0)
+		env.sub_succ_cnt++;
+	skip_account();
+
 	free(test->subtest_name);
 	test->subtest_name = NULL;
 }
@@ -786,17 +786,18 @@ int main(int argc, char **argv)
 			test__end_subtest();
 
 		test->tested = true;
-		if (test->error_cnt)
-			env.fail_cnt++;
-		else
-			env.succ_cnt++;
-		skip_account();
 
 		dump_test_log(test, test->error_cnt);
 
 		fprintf(env.stdout, "#%d %s:%s\n",
 			test->test_num, test->test_name,
-			test->error_cnt ? "FAIL" : "OK");
+			test->error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
+
+		if (test->error_cnt)
+			env.fail_cnt++;
+		else
+			env.succ_cnt++;
+		skip_account();
 
 		reset_affinity();
 		restore_netns();
-- 
2.30.2

[PATCH v3 bpf-next 3/4] selftests/bpf: Support glob matching for test selector.

[Yucong Sun]

This patch adds '-a' and '-b' arguments, supporting exact string match, as well
as using '*' wildchar in test/subtests selection.

Caveat: As same as the current substring matching mechanism, test and subtest
selector applies independently, 'a*/b*' will execute all tests matches "a*",
and with subtest name matches "b*", but tests matches "a*" but has no subtests
will also be executed.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 71 +++++++++++++++++++++---
 tools/testing/selftests/bpf/test_progs.h |  1 +
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index f0fbead40883..af43e206a806 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -13,6 +13,28 @@
 #include <execinfo.h> /* backtrace */
 #include <linux/membarrier.h>
 
+// Adapted from perf/util/string.c
+static bool __match_glob(const char *str, const char *pat)
+{
+	while (*str && *pat && *pat != '*') {
+		if (*str != *pat)
+			return false;
+		str++;
+		pat++;
+	}
+	/* Check wild card */
+	if (*pat == '*') {
+		while (*pat == '*')
+			pat++;
+		if (!*pat) /* Tail wild card matches all */
+			return true;
+		while (*str)
+			if (__match_glob(str++, pat))
+				return true;
+	}
+	return !*str && !*pat;
+}
+
 #define EXIT_NO_TEST		2
 #define EXIT_ERR_SETUP_INFRA	3
 
@@ -55,13 +77,23 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
 	int i;
 
 	for (i = 0; i < sel->blacklist.cnt; i++) {
-		if (strstr(name, sel->blacklist.strs[i]))
-			return false;
+		if (sel->blacklist.is_glob_pattern) {
+			if (__match_glob(name, sel->blacklist.strs[i]))
+				return false;
+		} else {
+			if (strstr(name, sel->blacklist.strs[i]))
+				return false;
+		}
 	}
 
 	for (i = 0; i < sel->whitelist.cnt; i++) {
-		if (strstr(name, sel->whitelist.strs[i]))
-			return true;
+		if (sel->whitelist.is_glob_pattern) {
+			if (__match_glob(name, sel->whitelist.strs[i]))
+				return true;
+		} else {
+			if (strstr(name, sel->whitelist.strs[i]))
+				return true;
+		}
 	}
 
 	if (!sel->whitelist.cnt && !sel->num_set)
@@ -450,6 +482,8 @@ enum ARG_KEYS {
 	ARG_VERBOSE = 'v',
 	ARG_GET_TEST_CNT = 'c',
 	ARG_LIST_TEST_NAMES = 'l',
+	ARG_TEST_NAME_GLOB_ALLOWLIST = 'a',
+	ARG_TEST_NAME_GLOB_DENYLIST = 'd',
 };
 
 static const struct argp_option opts[] = {
@@ -467,6 +501,10 @@ static const struct argp_option opts[] = {
 	  "Get number of selected top-level tests " },
 	{ "list", ARG_LIST_TEST_NAMES, NULL, 0,
 	  "List test names that would run (without running them) " },
+	{ "allow", ARG_TEST_NAME_GLOB_ALLOWLIST, "NAMES", 0,
+	  "Run tests with name matching the pattern (support *)." },
+	{ "deny", ARG_TEST_NAME_GLOB_DENYLIST, "NAMES", 0,
+	  "Don't run tests with name matching the pattern (support *)." },
 	{},
 };
 
@@ -491,7 +529,7 @@ static void free_str_set(const struct str_set *set)
 	free(set->strs);
 }
 
-static int parse_str_list(const char *s, struct str_set *set)
+static int parse_str_list(const char *s, struct str_set *set, bool is_glob_pattern)
 {
 	char *input, *state = NULL, *next, **tmp, **strs = NULL;
 	int cnt = 0;
@@ -516,6 +554,7 @@ static int parse_str_list(const char *s, struct str_set *set)
 		cnt++;
 	}
 
+	set->is_glob_pattern = is_glob_pattern;
 	set->cnt = cnt;
 	set->strs = (const char **)strs;
 	free(input);
@@ -553,29 +592,43 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 		}
 		break;
 	}
+	case ARG_TEST_NAME_GLOB_ALLOWLIST:
 	case ARG_TEST_NAME: {
+		if (env->test_selector.whitelist.cnt || env->subtest_selector.whitelist.cnt) {
+			fprintf(stderr, "-a and -t are mutually exclusive, you can only specific one.\n");
+			return -EINVAL;
+		}
 		char *subtest_str = strchr(arg, '/');
 
 		if (subtest_str) {
 			*subtest_str = '\0';
 			if (parse_str_list(subtest_str + 1,
-					   &env->subtest_selector.whitelist))
+					   &env->subtest_selector.whitelist,
+					   key == ARG_TEST_NAME_GLOB_ALLOWLIST))
 				return -ENOMEM;
 		}
-		if (parse_str_list(arg, &env->test_selector.whitelist))
+		if (parse_str_list(arg, &env->test_selector.whitelist,
+				   key == ARG_TEST_NAME_GLOB_ALLOWLIST))
 			return -ENOMEM;
 		break;
 	}
+	case ARG_TEST_NAME_GLOB_DENYLIST:
 	case ARG_TEST_NAME_BLACKLIST: {
+		if (env->test_selector.blacklist.cnt || env->subtest_selector.blacklist.cnt) {
+			fprintf(stderr, "-d and -b are mutually exclusive, you can only specific one.\n");
+			return -EINVAL;
+		}
 		char *subtest_str = strchr(arg, '/');
 
 		if (subtest_str) {
 			*subtest_str = '\0';
 			if (parse_str_list(subtest_str + 1,
-					   &env->subtest_selector.blacklist))
+					   &env->subtest_selector.blacklist,
+					   key == ARG_TEST_NAME_GLOB_DENYLIST))
 				return -ENOMEM;
 		}
-		if (parse_str_list(arg, &env->test_selector.blacklist))
+		if (parse_str_list(arg, &env->test_selector.blacklist,
+				   key == ARG_TEST_NAME_GLOB_DENYLIST))
 			return -ENOMEM;
 		break;
 	}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index c8c2bf878f67..c475d65dce4f 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -49,6 +49,7 @@ enum verbosity {
 struct str_set {
 	const char **strs;
 	int cnt;
+	bool is_glob_pattern;
 };
 
 struct test_selector {
-- 
2.30.2

[PATCH v3 bpf-next 4/4] selftests/bpf: also print test name in subtest status message

[Yucong Sun]

This patch add test name in subtest status message line, making it possible to
grep ':OK' in the output to generate a list of passed test+subtest names, which
can be processed to generate argument list to be used with "-a", "-d" exact
string matching.

Example: $sudo ./test_progs -a 'xdp*' 2>/dev/null | grep ":OK" | cut -d":" -f 1
| cut -d" " -f2- | paste -s -d,
xdp_adjust_tail/xdp_adjust_tail_shrink,xdp,xdp_devmap_attach/Verifier check of
DEVMAP programs,xdp_info,xdp_noinline,xdp_perf

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index af43e206a806..23e4ea51f9e7 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -182,8 +182,8 @@ void test__end_subtest()
 
 	dump_test_log(test, sub_error_cnt);
 
-	fprintf(env.stdout, "#%d/%d %s:%s\n",
-	       test->test_num, test->subtest_num, test->subtest_name,
+	fprintf(env.stdout, "#%d/%d %s/%s:%s\n",
+	       test->test_num, test->subtest_num, test->test_name, test->subtest_name,
 	       sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
 
 	if (sub_error_cnt)
-- 
2.30.2

Re: [PATCH v3 bpf-next 3/4] selftests/bpf: Support glob matching for test selector.

[Andrii Nakryiko]

On Tue, Aug 10, 2021 at 2:21 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch adds '-a' and '-b' arguments, supporting exact string match, as well

typo: you probably meant '-d'

> as using '*' wildchar in test/subtests selection.

typo: wildcard

>
> Caveat: As same as the current substring matching mechanism, test and subtest
> selector applies independently, 'a*/b*' will execute all tests matches "a*",
> and with subtest name matches "b*", but tests matches "a*" but has no subtests
> will also be executed.
>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---

This looks good, see proposal at the end to simplify internals by
always using glob form.

But there is another problem with test/subtest selection. While you
are looking at this topic, maybe you can deal with that as well?

Basically:

[vmuser@archvm bpf]$ sudo ./test_progs -t core_reloc/arrays -t
core_reloc/enumval
#32/68 enumval:OK
#32/69 enumval___diff:OK
#32/70 enumval___val3_missing:OK
#32/71 enumval___err_missing:OK
#32 core_reloc:OK
Summary: 1/4 PASSED, 0 SKIPPED, 0 FAILED
[vmuser@archvm bpf]$ sudo ./test_progs -t core_reloc/arrays,core_reloc/enumval
#32/19 arrays:OK
#32/20 arrays___diff_arr_dim:OK
#32/21 arrays___diff_arr_val_sz:OK
#32/22 arrays___equiv_zero_sz_arr:OK
#32/23 arrays___fixed_arr:OK
#32/24 arrays___err_too_small:OK
#32/25 arrays___err_too_shallow:OK
#32/26 arrays___err_non_array:OK
#32/27 arrays___err_wrong_val_type:OK
#32/28 arrays___err_bad_zero_sz_arr:OK
#32 core_reloc:OK
Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED

The intent was to run two sets of subtests, but we get either one or another...

For test selection it's a bit better. sudo ./test_progs -t
core_reloc,core_extern will run both tests, but ./test_progs -t
core_reloc and -t core_extern will run just core_extern.

This is not a theoretical problem, I ran into these limitations when
trying to disable only some of the subtests on older kernels (see
[0]).

It would be great to support both -t abc -t def to be interpreted as
-t abc,def (concatenation of specifications), and fix multiple
specifiers with subtests.

It's actually ambiguous, because -t foo/bar,baz can be interpreted as
either "test foo, subtests bar and baz" (i.e., -t foo/bar,foo/baz) or
"test foo and subtest bar within it, and separately test baz" (so, -t
foo/bar -t baz). I think the second one is less surprising and it
still allows to specify multiple subtests with more verbose form, if
necessary.

See if you can tackle that, but it doesn't have to be in the same patch set.

  [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/blacklist/BLACKLIST-5.5.0#L106-L108

>  tools/testing/selftests/bpf/test_progs.c | 71 +++++++++++++++++++++---
>  tools/testing/selftests/bpf/test_progs.h |  1 +
>  2 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index f0fbead40883..af43e206a806 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -13,6 +13,28 @@
>  #include <execinfo.h> /* backtrace */
>  #include <linux/membarrier.h>
>
> +// Adapted from perf/util/string.c

no C++ comments, please

> +static bool __match_glob(const char *str, const char *pat)

we (almost) never use __funcname in selftest, is there anything wrong
with just "match_glob"? Better still "matches_glob" reads more
meaningfully.

> +{
> +       while (*str && *pat && *pat != '*') {
> +               if (*str != *pat)
> +                       return false;
> +               str++;
> +               pat++;
> +       }

[...]

> @@ -553,29 +592,43 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>                 }
>                 break;
>         }
> +       case ARG_TEST_NAME_GLOB_ALLOWLIST:
>         case ARG_TEST_NAME: {
> +               if (env->test_selector.whitelist.cnt || env->subtest_selector.whitelist.cnt) {
> +                       fprintf(stderr, "-a and -t are mutually exclusive, you can only specific one.\n");

typo: specify

but also, it just occurred to me. Isn't `-t whatever` same as `-a
'*whatever*'`? We can do that transparently and make -a and -t
co-exist nicely. Basically, let's do globs only internally.

> +                       return -EINVAL;
> +               }
>                 char *subtest_str = strchr(arg, '/');
>

[...]

Re: [PATCH v3 bpf-next 4/4] selftests/bpf: also print test name in subtest status message

[Andrii Nakryiko]

On Tue, Aug 10, 2021 at 2:21 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch add test name in subtest status message line, making it possible to
> grep ':OK' in the output to generate a list of passed test+subtest names, which
> can be processed to generate argument list to be used with "-a", "-d" exact
> string matching.
>
> Example: $sudo ./test_progs -a 'xdp*' 2>/dev/null | grep ":OK" | cut -d":" -f 1
> | cut -d" " -f2- | paste -s -d,
> xdp_adjust_tail/xdp_adjust_tail_shrink,xdp,xdp_devmap_attach/Verifier check of
> DEVMAP programs,xdp_info,xdp_noinline,xdp_perf

This looks a bit like a mess in the commit log. I think it's a bit
more useful if you just paste a few lines of the new output format as
is, with no post-processing.

But overall it makes sense. I remember Alexei thought that it's
unnecessary verbosity, but since then I often found it useful to be
able to know the full test/subtest specifier, so I'm all for it.

>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index af43e206a806..23e4ea51f9e7 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -182,8 +182,8 @@ void test__end_subtest()
>
>         dump_test_log(test, sub_error_cnt);
>
> -       fprintf(env.stdout, "#%d/%d %s:%s\n",
> -              test->test_num, test->subtest_num, test->subtest_name,
> +       fprintf(env.stdout, "#%d/%d %s/%s:%s\n",
> +              test->test_num, test->subtest_num, test->test_name, test->subtest_name,
>                sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
>
>         if (sub_error_cnt)
> --
> 2.30.2
>