[PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Jesper Dangaard Brouer]

The program test_progs have some very useful ability to specify a list of
test name substrings for selecting which tests to run.

This patch add the ability to list the selected test names without running
them. This is practical for seeing which tests gets selected with given
select arguments (which can also contain a exclude list via --name-blacklist).

This output can also be used by shell-scripts in a for-loop:

 for N in $(./test_progs --list -t xdp); do \
   ./test_progs -t $N 2>&1 > result_test_${N}.log & \
 done ; wait

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/test_progs.c |   20 ++++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.h |    1 +
 2 files changed, 21 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 1aa5360c427f..36abc3d4a8e2 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -367,6 +367,7 @@ enum ARG_KEYS {
 	ARG_VERIFIER_STATS = 's',
 	ARG_VERBOSE = 'v',
 	ARG_GET_TEST_CNT = 'c',
+	ARG_LIST_TEST_NAMES = 'l',
 };
 
 static const struct argp_option opts[] = {
@@ -382,6 +383,8 @@ static const struct argp_option opts[] = {
 	  "Verbose output (use -vv or -vvv for progressively verbose output)" },
 	{ "count", ARG_GET_TEST_CNT, NULL, 0,
 	  "Get number of top-level tests " },
+	{ "list", ARG_LIST_TEST_NAMES, NULL, 0,
+	  "List test names that would run (without running them) " },
 	{},
 };
 
@@ -517,6 +520,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	case ARG_GET_TEST_CNT:
 		env->get_test_cnt = true;
 		break;
+	case ARG_LIST_TEST_NAMES:
+		env->list_test_names = true;
+		break;
 	case ARGP_KEY_ARG:
 		argp_usage(state);
 		break;
@@ -665,6 +671,12 @@ int main(int argc, char **argv)
 				test->test_num, test->test_name))
 			continue;
 
+		if (env.list_test_names) {
+			fprintf(env.stdout, "%s\n", test->test_name);
+			env.succ_cnt++;
+			continue;
+		}
+
 		test->run_test();
 		/* ensure last sub-test is finalized properly */
 		if (test->subtest_name)
@@ -688,9 +700,17 @@ int main(int argc, char **argv)
 			cleanup_cgroup_environment();
 	}
 	stdio_restore();
+
+	if (env.list_test_names) {
+		if (env.succ_cnt == 0)
+			env.fail_cnt = 1;
+		goto out;
+	}
+
 	fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
 		env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
 
+out:
 	free_str_set(&env.test_selector.blacklist);
 	free_str_set(&env.test_selector.whitelist);
 	free(env.test_selector.num_set);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 0030584619c3..ec31f382e7fd 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -67,6 +67,7 @@ struct test_env {
 
 	bool jit_enabled;
 	bool get_test_cnt;
+	bool list_test_names;
 
 	struct prog_test_def *test;
 	FILE *stdout;

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Andrii Nakryiko]

On Tue, Jun 30, 2020 at 8:40 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> The program test_progs have some very useful ability to specify a list of
> test name substrings for selecting which tests to run.
>
> This patch add the ability to list the selected test names without running
> them. This is practical for seeing which tests gets selected with given
> select arguments (which can also contain a exclude list via --name-blacklist).
>
> This output can also be used by shell-scripts in a for-loop:
>
>  for N in $(./test_progs --list -t xdp); do \
>    ./test_progs -t $N 2>&1 > result_test_${N}.log & \
>  done ; wait
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c |   20 ++++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |    1 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 1aa5360c427f..36abc3d4a8e2 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -367,6 +367,7 @@ enum ARG_KEYS {
>         ARG_VERIFIER_STATS = 's',
>         ARG_VERBOSE = 'v',
>         ARG_GET_TEST_CNT = 'c',
> +       ARG_LIST_TEST_NAMES = 'l',
>  };
>
>  static const struct argp_option opts[] = {
> @@ -382,6 +383,8 @@ static const struct argp_option opts[] = {
>           "Verbose output (use -vv or -vvv for progressively verbose output)" },
>         { "count", ARG_GET_TEST_CNT, NULL, 0,
>           "Get number of top-level tests " },
> +       { "list", ARG_LIST_TEST_NAMES, NULL, 0,
> +         "List test names that would run (without running them) " },
>         {},
>  };
>
> @@ -517,6 +520,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>         case ARG_GET_TEST_CNT:
>                 env->get_test_cnt = true;
>                 break;
> +       case ARG_LIST_TEST_NAMES:
> +               env->list_test_names = true;
> +               break;
>         case ARGP_KEY_ARG:
>                 argp_usage(state);
>                 break;
> @@ -665,6 +671,12 @@ int main(int argc, char **argv)
>                                 test->test_num, test->test_name))
>                         continue;
>
> +               if (env.list_test_names) {
> +                       fprintf(env.stdout, "%s\n", test->test_name);
> +                       env.succ_cnt++;
> +                       continue;
> +               }
> +
>                 test->run_test();
>                 /* ensure last sub-test is finalized properly */
>                 if (test->subtest_name)
> @@ -688,9 +700,17 @@ int main(int argc, char **argv)
>                         cleanup_cgroup_environment();
>         }
>         stdio_restore();
> +
> +       if (env.list_test_names) {
> +               if (env.succ_cnt == 0)
> +                       env.fail_cnt = 1;
> +               goto out;
> +       }
> +

Why failure if no test matched? Is that to catch bugs in whitelisting?

>         fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
>                 env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
>
> +out:
>         free_str_set(&env.test_selector.blacklist);
>         free_str_set(&env.test_selector.whitelist);
>         free(env.test_selector.num_set);
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index 0030584619c3..ec31f382e7fd 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -67,6 +67,7 @@ struct test_env {
>
>         bool jit_enabled;
>         bool get_test_cnt;
> +       bool list_test_names;
>
>         struct prog_test_def *test;
>         FILE *stdout;
>
>

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Jesper Dangaard Brouer]

On Tue, 30 Jun 2020 08:46:01 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> > @@ -688,9 +700,17 @@ int main(int argc, char **argv)
> >                         cleanup_cgroup_environment();
> >         }
> >         stdio_restore();
> > +
> > +       if (env.list_test_names) {
> > +               if (env.succ_cnt == 0)
> > +                       env.fail_cnt = 1;
> > +               goto out;
> > +       }
> > +  
> 
> Why failure if no test matched? Is that to catch bugs in whitelisting?

I would not call it catch bugs, but sort of.  The purpose is to know if
requested test is valid.  This can be used to e.g. run through all the
tests numbers, and stopping when a test number (-n) is no-longer valid,
by using this shell exit value as a test, like:

 n=1;
 while [ $(./test_progs --list -n $n) ] ; do \
   echo "./test_progs -n $n" ; n=$(( n+1 )); \
 done

Notice that this features that be used for looking up a test number,
and returning a testname, which was the original request from CI.  I
choose this implementation as it more generic and generally useful.

 $ ./test_progs --list -n 89
 xdp_adjust_tail


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Andrii Nakryiko]

On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Tue, 30 Jun 2020 08:46:01 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > > @@ -688,9 +700,17 @@ int main(int argc, char **argv)
> > >                         cleanup_cgroup_environment();
> > >         }
> > >         stdio_restore();
> > > +
> > > +       if (env.list_test_names) {
> > > +               if (env.succ_cnt == 0)
> > > +                       env.fail_cnt = 1;
> > > +               goto out;
> > > +       }
> > > +
> >
> > Why failure if no test matched? Is that to catch bugs in whitelisting?
>
> I would not call it catch bugs, but sort of.  The purpose is to know if
> requested test is valid.  This can be used to e.g. run through all the
> tests numbers, and stopping when a test number (-n) is no-longer valid,
> by using this shell exit value as a test, like:
>
>  n=1;
>  while [ $(./test_progs --list -n $n) ] ; do \
>    echo "./test_progs -n $n" ; n=$(( n+1 )); \
>  done
>
> Notice that this features that be used for looking up a test number,
> and returning a testname, which was the original request from CI.  I
> choose this implementation as it more generic and generally useful.
>
>  $ ./test_progs --list -n 89
>  xdp_adjust_tail
>

Yeah, it has a nice querying effect. Makes sense.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer
>

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Alexei Starovoitov]

On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> >
> > On Tue, 30 Jun 2020 08:46:01 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv)
> > > >                         cleanup_cgroup_environment();
> > > >         }
> > > >         stdio_restore();
> > > > +
> > > > +       if (env.list_test_names) {
> > > > +               if (env.succ_cnt == 0)
> > > > +                       env.fail_cnt = 1;
> > > > +               goto out;
> > > > +       }
> > > > +
> > >
> > > Why failure if no test matched? Is that to catch bugs in whitelisting?
> >
> > I would not call it catch bugs, but sort of.  The purpose is to know if
> > requested test is valid.  This can be used to e.g. run through all the
> > tests numbers, and stopping when a test number (-n) is no-longer valid,
> > by using this shell exit value as a test, like:
> >
> >  n=1;
> >  while [ $(./test_progs --list -n $n) ] ; do \
> >    echo "./test_progs -n $n" ; n=$(( n+1 )); \
> >  done
> >
> > Notice that this features that be used for looking up a test number,
> > and returning a testname, which was the original request from CI.  I
> > choose this implementation as it more generic and generally useful.
> >
> >  $ ./test_progs --list -n 89
> >  xdp_adjust_tail
> >
>
> Yeah, it has a nice querying effect. Makes sense.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>

hmm. it doesn't apply.
Applying: selftests/bpf: Test_progs option for listing test names
error: sha1 information is lacking or useless
(tools/testing/selftests/bpf/test_progs.c).
error: could not build fake ancestor
Patch failed at 0001 selftests/bpf: Test_progs option for listing test names

Could you please respin.
Thanks

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Jesper Dangaard Brouer]

On Wed, 1 Jul 2020 08:36:08 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer
> > <brouer@redhat.com> wrote:  
> > >
> > > On Tue, 30 Jun 2020 08:46:01 -0700
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > >  
> > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv)
> > > > >                         cleanup_cgroup_environment();
> > > > >         }
> > > > >         stdio_restore();
> > > > > +
> > > > > +       if (env.list_test_names) {
> > > > > +               if (env.succ_cnt == 0)
> > > > > +                       env.fail_cnt = 1;
> > > > > +               goto out;
> > > > > +       }
> > > > > +  
> > > >
> > > > Why failure if no test matched? Is that to catch bugs in whitelisting?  
> > >
> > > I would not call it catch bugs, but sort of.  The purpose is to know if
> > > requested test is valid.  This can be used to e.g. run through all the
> > > tests numbers, and stopping when a test number (-n) is no-longer valid,
> > > by using this shell exit value as a test, like:
> > >
> > >  n=1;
> > >  while [ $(./test_progs --list -n $n) ] ; do \
> > >    echo "./test_progs -n $n" ; n=$(( n+1 )); \
> > >  done
> > >
> > > Notice that this features that be used for looking up a test number,
> > > and returning a testname, which was the original request from CI.  I
> > > choose this implementation as it more generic and generally useful.
> > >
> > >  $ ./test_progs --list -n 89
> > >  xdp_adjust_tail
> > >  
> >
> > Yeah, it has a nice querying effect. Makes sense.
> >
> > Acked-by: Andrii Nakryiko <andriin@fb.com>  
> 
> hmm. it doesn't apply.
> Applying: selftests/bpf: Test_progs option for listing test names
> error: sha1 information is lacking or useless
> (tools/testing/selftests/bpf/test_progs.c).
> error: could not build fake ancestor
> Patch failed at 0001 selftests/bpf: Test_progs option for listing test names

It doesn't apply because it depend on my previous changes, that Daniel
said he applied:

 https://lore.kernel.org/bpf/6e7543fa-f496-a6d2-a6d5-70dff9f84090@iogearbox.net/

But I can see that it is not in the net-next git tree.
 
> Could you please respin.

I will respin together with the other unapplied patch.  Which is
actually fine, as I have an improvement for the previous patch, that I
can squash.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH bpf-next] selftests/bpf: test_progs option for listing test names

[Alexei Starovoitov]

On Wed, Jul 1, 2020 at 9:23 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> On Wed, 1 Jul 2020 08:36:08 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer
> > > <brouer@redhat.com> wrote:
> > > >
> > > > On Tue, 30 Jun 2020 08:46:01 -0700
> > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv)
> > > > > >                         cleanup_cgroup_environment();
> > > > > >         }
> > > > > >         stdio_restore();
> > > > > > +
> > > > > > +       if (env.list_test_names) {
> > > > > > +               if (env.succ_cnt == 0)
> > > > > > +                       env.fail_cnt = 1;
> > > > > > +               goto out;
> > > > > > +       }
> > > > > > +
> > > > >
> > > > > Why failure if no test matched? Is that to catch bugs in whitelisting?
> > > >
> > > > I would not call it catch bugs, but sort of.  The purpose is to know if
> > > > requested test is valid.  This can be used to e.g. run through all the
> > > > tests numbers, and stopping when a test number (-n) is no-longer valid,
> > > > by using this shell exit value as a test, like:
> > > >
> > > >  n=1;
> > > >  while [ $(./test_progs --list -n $n) ] ; do \
> > > >    echo "./test_progs -n $n" ; n=$(( n+1 )); \
> > > >  done
> > > >
> > > > Notice that this features that be used for looking up a test number,
> > > > and returning a testname, which was the original request from CI.  I
> > > > choose this implementation as it more generic and generally useful.
> > > >
> > > >  $ ./test_progs --list -n 89
> > > >  xdp_adjust_tail
> > > >
> > >
> > > Yeah, it has a nice querying effect. Makes sense.
> > >
> > > Acked-by: Andrii Nakryiko <andriin@fb.com>
> >
> > hmm. it doesn't apply.
> > Applying: selftests/bpf: Test_progs option for listing test names
> > error: sha1 information is lacking or useless
> > (tools/testing/selftests/bpf/test_progs.c).
> > error: could not build fake ancestor
> > Patch failed at 0001 selftests/bpf: Test_progs option for listing test names
>
> It doesn't apply because it depend on my previous changes, that Daniel
> said he applied:
>
>  https://lore.kernel.org/bpf/6e7543fa-f496-a6d2-a6d5-70dff9f84090@iogearbox.net/
>
> But I can see that it is not in the net-next git tree.

oops. sorry about this.
I guess to due taking out Jakub's set out of bpf-next and moving into bpf
some patches got lost. :(

> > Could you please respin.
>
> I will respin together with the other unapplied patch.  Which is
> actually fine, as I have an improvement for the previous patch, that I
> can squash.

Awesome. thanks