All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting
@ 2020-07-01 21:44 Jesper Dangaard Brouer
  2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-01 21:44 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc

V3: Reorder patches to cause less code churn.

The BPF selftest 'test_progs' contains many tests, that cover all the
different areas of the kernel where BPF is used.  The CI system sees this
as one test, which is impractical for identifying what team/engineer is
responsible for debugging the problem.

This patchset add some options that makes it easier to create a shell
for-loop that invoke each (top-level) test avail in test_progs. Then each
test FAIL/PASS result can be presented the CI system to have a separate
bullet. (For Red Hat use-case in Beaker https://beaker-project.org/)

Created a public script[1] that uses these features in an advanced way.
Demonstrating howto reduce the number of (top-level) tests by grouping tests
together via using the existing test pattern selection feature, and then
using the new --list feature combined with exclude (-b) to get a list of
remaining test names that was not part of the groups.

[1] https://github.com/netoptimizer/prototype-kernel/blob/master/scripts/bpf_selftests_grouping.sh

---

Jesper Dangaard Brouer (3):
      selftests/bpf: test_progs indicate to shell on non-actions
      selftests/bpf: test_progs option for getting number of tests
      selftests/bpf: test_progs option for listing test names


 tools/testing/selftests/bpf/test_progs.c |   36 ++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/test_progs.h |    2 ++
 2 files changed, 38 insertions(+)

--


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions
  2020-07-01 21:44 [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Jesper Dangaard Brouer
@ 2020-07-01 21:44 ` Jesper Dangaard Brouer
  2020-07-01 21:46   ` Andrii Nakryiko
  2020-07-02 13:47   ` Jesper Dangaard Brouer
  2020-07-01 21:44 ` [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests Jesper Dangaard Brouer
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-01 21:44 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc

When a user selects a non-existing test the summary is printed with
indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
indicated. This can be understood by a human end-user, but for shell
scripting is it useful to indicate a shell failure (EXIT_FAILURE).

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

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 54fa5fa688ce..da70a4f72f54 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -687,5 +687,8 @@ int main(int argc, char **argv)
 	free_str_set(&env.subtest_selector.whitelist);
 	free(env.subtest_selector.num_set);
 
+	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
+		return EXIT_FAILURE;
+
 	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
 }



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests
  2020-07-01 21:44 [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Jesper Dangaard Brouer
  2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
@ 2020-07-01 21:44 ` Jesper Dangaard Brouer
  2020-07-01 21:47   ` Andrii Nakryiko
  2020-07-01 21:44 ` [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names Jesper Dangaard Brouer
  2020-07-01 22:22 ` [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Alexei Starovoitov
  3 siblings, 1 reply; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-01 21:44 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc

It can be practial to get the number of tests that test_progs contain.
This could for example be used to create a shell for-loop construct that
runs the individual tests.

Like:
 for N in $(seq 1 $(./test_progs -c)); do
   ./test_progs -n $N 2>&1 > result_test_${N}.log &
 done ; wait

V2: Add the ability to return the count for the selected tests. This is
useful for getting a count e.g. after excluding some tests with option -b.
The current beakers test script like to report the max test count upfront.

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

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index da70a4f72f54..a5dba14b2025 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -366,6 +366,7 @@ enum ARG_KEYS {
 	ARG_TEST_NAME_BLACKLIST = 'b',
 	ARG_VERIFIER_STATS = 's',
 	ARG_VERBOSE = 'v',
+	ARG_GET_TEST_CNT = 'c',
 };
 
 static const struct argp_option opts[] = {
@@ -379,6 +380,8 @@ static const struct argp_option opts[] = {
 	  "Output verifier statistics", },
 	{ "verbose", ARG_VERBOSE, "LEVEL", OPTION_ARG_OPTIONAL,
 	  "Verbose output (use -vv or -vvv for progressively verbose output)" },
+	{ "count", ARG_GET_TEST_CNT, NULL, 0,
+	  "Get number of selected top-level tests " },
 	{},
 };
 
@@ -511,6 +514,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 			}
 		}
 		break;
+	case ARG_GET_TEST_CNT:
+		env->get_test_cnt = true;
+		break;
 	case ARGP_KEY_ARG:
 		argp_usage(state);
 		break;
@@ -654,6 +660,11 @@ int main(int argc, char **argv)
 				test->test_num, test->test_name))
 			continue;
 
+		if (env.get_test_cnt) {
+			env.succ_cnt++;
+			continue;
+		}
+
 		test->run_test();
 		/* ensure last sub-test is finalized properly */
 		if (test->subtest_name)
@@ -677,9 +688,16 @@ int main(int argc, char **argv)
 			cleanup_cgroup_environment();
 	}
 	stdio_restore();
+
+	if (env.get_test_cnt) {
+		printf("%d\n", env.succ_cnt);
+		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 f4503c926aca..0030584619c3 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -66,6 +66,7 @@ struct test_env {
 	enum verbosity verbosity;
 
 	bool jit_enabled;
+	bool get_test_cnt;
 
 	struct prog_test_def *test;
 	FILE *stdout;



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names
  2020-07-01 21:44 [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Jesper Dangaard Brouer
  2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
  2020-07-01 21:44 ` [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests Jesper Dangaard Brouer
@ 2020-07-01 21:44 ` Jesper Dangaard Brouer
  2020-07-01 21:48   ` Andrii Nakryiko
  2020-07-01 22:22 ` [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Alexei Starovoitov
  3 siblings, 1 reply; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-01 21:44 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc

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

This features can also be used for looking up a test number and returning
a testname. If the selection was empty then a shell EXIT_FAILURE is
returned.  This is useful for scripting. e.g. like this:

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

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

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index a5dba14b2025..ef05d2f0e7bb 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 selected 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)
 			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)
@@ -694,6 +706,9 @@ int main(int argc, char **argv)
 		goto out;
 	}
 
+	if (env.list_test_names)
+		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);
 
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;



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions
  2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
@ 2020-07-01 21:46   ` Andrii Nakryiko
  2020-07-02 13:47   ` Jesper Dangaard Brouer
  1 sibling, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2020-07-01 21:46 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Hangbin Liu, Daniel Borkmann, Alexei Starovoitov,
	Veronika Kabatova, Jiri Benc

On Wed, Jul 1, 2020 at 2:44 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> When a user selects a non-existing test the summary is printed with
> indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
> indicated. This can be understood by a human end-user, but for shell
> scripting is it useful to indicate a shell failure (EXIT_FAILURE).
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---

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

>  tools/testing/selftests/bpf/test_progs.c |    3 +++
>  1 file changed, 3 insertions(+)
>

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests
  2020-07-01 21:44 ` [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests Jesper Dangaard Brouer
@ 2020-07-01 21:47   ` Andrii Nakryiko
  0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2020-07-01 21:47 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Hangbin Liu, Daniel Borkmann, Alexei Starovoitov,
	Veronika Kabatova, Jiri Benc

On Wed, Jul 1, 2020 at 2:44 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> It can be practial to get the number of tests that test_progs contain.
> This could for example be used to create a shell for-loop construct that
> runs the individual tests.
>
> Like:
>  for N in $(seq 1 $(./test_progs -c)); do
>    ./test_progs -n $N 2>&1 > result_test_${N}.log &
>  done ; wait
>
> V2: Add the ability to return the count for the selected tests. This is
> useful for getting a count e.g. after excluding some tests with option -b.
> The current beakers test script like to report the max test count upfront.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---

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

>  tools/testing/selftests/bpf/test_progs.c |   18 ++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |    1 +
>  2 files changed, 19 insertions(+)
>

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names
  2020-07-01 21:44 ` [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names Jesper Dangaard Brouer
@ 2020-07-01 21:48   ` Andrii Nakryiko
  0 siblings, 0 replies; 13+ messages in thread
From: Andrii Nakryiko @ 2020-07-01 21:48 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Hangbin Liu, Daniel Borkmann, Alexei Starovoitov,
	Veronika Kabatova, Jiri Benc

On Wed, Jul 1, 2020 at 2:44 PM 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
>
> This features can also be used for looking up a test number and returning
> a testname. If the selection was empty then a shell EXIT_FAILURE is
> returned.  This is useful for scripting. e.g. like this:
>
>  n=1;
>  while [ $(./test_progs --list -n $n) ] ; do \
>    ./test_progs -n $n ; n=$(( n+1 )); \
>  done
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---

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

>  tools/testing/selftests/bpf/test_progs.c |   15 +++++++++++++++
>  tools/testing/selftests/bpf/test_progs.h |    1 +
>  2 files changed, 16 insertions(+)
>

[...]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting
  2020-07-01 21:44 [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Jesper Dangaard Brouer
                   ` (2 preceding siblings ...)
  2020-07-01 21:44 ` [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names Jesper Dangaard Brouer
@ 2020-07-01 22:22 ` Alexei Starovoitov
  3 siblings, 0 replies; 13+ messages in thread
From: Alexei Starovoitov @ 2020-07-01 22:22 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Andrii Nakryiko, Hangbin Liu, Daniel Borkmann,
	Veronika Kabatova, Jiri Benc

On Wed, Jul 1, 2020 at 2:44 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> V3: Reorder patches to cause less code churn.
>
> The BPF selftest 'test_progs' contains many tests, that cover all the
> different areas of the kernel where BPF is used.  The CI system sees this
> as one test, which is impractical for identifying what team/engineer is
> responsible for debugging the problem.
>
> This patchset add some options that makes it easier to create a shell
> for-loop that invoke each (top-level) test avail in test_progs. Then each
> test FAIL/PASS result can be presented the CI system to have a separate
> bullet. (For Red Hat use-case in Beaker https://beaker-project.org/)
>
> Created a public script[1] that uses these features in an advanced way.
> Demonstrating howto reduce the number of (top-level) tests by grouping tests
> together via using the existing test pattern selection feature, and then
> using the new --list feature combined with exclude (-b) to get a list of
> remaining test names that was not part of the groups.
>
> [1] https://github.com/netoptimizer/prototype-kernel/blob/master/scripts/bpf_selftests_grouping.sh

Applied. Thanks

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions
  2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
  2020-07-01 21:46   ` Andrii Nakryiko
@ 2020-07-02 13:47   ` Jesper Dangaard Brouer
  2020-07-02 17:59     ` [PATCH bpf-next] selftests/bpf: test_progs use another shell exit " Jesper Dangaard Brouer
  1 sibling, 1 reply; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-02 13:47 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Hangbin Liu, Daniel Borkmann, Alexei Starovoitov, vkabatov,
	jbenc, brouer

On Wed, 01 Jul 2020 23:44:07 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> When a user selects a non-existing test the summary is printed with
> indication 0 for all info types, and shell "success" (EXIT_SUCCESS) is
> indicated. This can be understood by a human end-user, but for shell
> scripting is it useful to indicate a shell failure (EXIT_FAILURE).
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 54fa5fa688ce..da70a4f72f54 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -687,5 +687,8 @@ int main(int argc, char **argv)
>  	free_str_set(&env.subtest_selector.whitelist);
>  	free(env.subtest_selector.num_set);
>  
> +	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
> +		return EXIT_FAILURE;

We should use another return code as indication, else the shell script
cannot tell the difference between no-test-selected and failed-test.
Normally I would request a V4 (from myself I guess), but this have
already been merged, so I'll send a followup patch.

> +
>  	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>  }
> 

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH bpf-next] selftests/bpf: test_progs use another shell exit on non-actions
  2020-07-02 13:47   ` Jesper Dangaard Brouer
@ 2020-07-02 17:59     ` Jesper Dangaard Brouer
  2020-07-02 21:24       ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-02 17:59 UTC (permalink / raw)
  To: bpf, Andrii Nakryiko
  Cc: Jesper Dangaard Brouer, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc

This is a follow up adjustment to commit 6c92bd5cd465 ("selftests/bpf:
Test_progs indicate to shell on non-actions"), that returns shell exit
indication EXIT_FAILURE (value 1) when user selects a non-existing test.

The problem with using EXIT_FAILURE is that a shell script cannot tell
the difference between a non-existing test and the test failing.

This patch uses value 2 as shell exit indication.
(Aside note unrecognized option parameters use value 64).

Fixes: 6c92bd5cd465 ("selftests/bpf: Test_progs indicate to shell on non-actions")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 tools/testing/selftests/bpf/test_progs.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 104e833d0087..e8f7cd5dbae4 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -12,6 +12,8 @@
 #include <string.h>
 #include <execinfo.h> /* backtrace */
 
+#define EXIT_NO_TEST 2
+
 /* defined in test_progs.h */
 struct test_env env = {};
 
@@ -740,7 +742,7 @@ int main(int argc, char **argv)
 	close(env.saved_netns_fd);
 
 	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
-		return EXIT_FAILURE;
+		return EXIT_NO_TEST;
 
 	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
 }



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: test_progs use another shell exit on non-actions
  2020-07-02 17:59     ` [PATCH bpf-next] selftests/bpf: test_progs use another shell exit " Jesper Dangaard Brouer
@ 2020-07-02 21:24       ` Yonghong Song
  2020-07-03 11:59         ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2020-07-02 21:24 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, bpf, Andrii Nakryiko
  Cc: Hangbin Liu, Daniel Borkmann, Alexei Starovoitov, vkabatov, jbenc



On 7/2/20 10:59 AM, Jesper Dangaard Brouer wrote:
> This is a follow up adjustment to commit 6c92bd5cd465 ("selftests/bpf:
> Test_progs indicate to shell on non-actions"), that returns shell exit
> indication EXIT_FAILURE (value 1) when user selects a non-existing test.
> 
> The problem with using EXIT_FAILURE is that a shell script cannot tell
> the difference between a non-existing test and the test failing.
> 
> This patch uses value 2 as shell exit indication.
> (Aside note unrecognized option parameters use value 64).
> 
> Fixes: 6c92bd5cd465 ("selftests/bpf: Test_progs indicate to shell on non-actions")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>   tools/testing/selftests/bpf/test_progs.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 104e833d0087..e8f7cd5dbae4 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -12,6 +12,8 @@
>   #include <string.h>
>   #include <execinfo.h> /* backtrace */
>   
> +#define EXIT_NO_TEST 2
How do you ensure this won't collide with other exit code
from other library functions (e.g., error code 64 is used
for unrecognized option which I have no idea what 64 means)?

Maybe -2 for the exit code? test_progs already uses -1.

> +
>   /* defined in test_progs.h */
>   struct test_env env = {};
>   
> @@ -740,7 +742,7 @@ int main(int argc, char **argv)
>   	close(env.saved_netns_fd);
>   
>   	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
> -		return EXIT_FAILURE;
> +		return EXIT_NO_TEST;
>   
>   	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>   }
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: test_progs use another shell exit on non-actions
  2020-07-02 21:24       ` Yonghong Song
@ 2020-07-03 11:59         ` Jesper Dangaard Brouer
  2020-07-04 18:15           ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: Jesper Dangaard Brouer @ 2020-07-03 11:59 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Andrii Nakryiko, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc, brouer

On Thu, 2 Jul 2020 14:24:14 -0700
Yonghong Song <yhs@fb.com> wrote:

> On 7/2/20 10:59 AM, Jesper Dangaard Brouer wrote:
> > This is a follow up adjustment to commit 6c92bd5cd465 ("selftests/bpf:
> > Test_progs indicate to shell on non-actions"), that returns shell exit
> > indication EXIT_FAILURE (value 1) when user selects a non-existing test.
> > 
> > The problem with using EXIT_FAILURE is that a shell script cannot tell
> > the difference between a non-existing test and the test failing.
> > 
> > This patch uses value 2 as shell exit indication.
> > (Aside note unrecognized option parameters use value 64).
> > 
> > Fixes: 6c92bd5cd465 ("selftests/bpf: Test_progs indicate to shell on non-actions")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >   tools/testing/selftests/bpf/test_progs.c |    4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 104e833d0087..e8f7cd5dbae4 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -12,6 +12,8 @@
> >   #include <string.h>
> >   #include <execinfo.h> /* backtrace */
> >   
> > +#define EXIT_NO_TEST 2  
>
> How do you ensure this won't collide with other exit code
> from other library functions (e.g., error code 64 is used
> for unrecognized option which I have no idea what 64 means)?

I expect 64 comes from: /usr/include/sysexits.h
 #define EX_USAGE        64      /* command line usage error */


> Maybe -2 for the exit code?

No. The process's exit status must be a number between 0 and 255, as
defined in man exit(3). (run: 'man 3 exit' as there are many manpages
named exit).

But don't use above 127, because that is usually used for indicating
signals.  E.g. 139 means 11=SIGSEGV $((139 & 127))=11.  POSIX defines
in man wait(3p) check WIFSIGNALED(STATUS) and WTERMSIG(139)=11.
(Hint: cmd 'kill -l' list signals and their numbers).

I bring up Segmentation fault explicitly, as we are seeing these happen
with different tests (that are part of test_progs).  CI people writing
these shell-scripts could pickup these hints and report them, if that
makes sense.


> test_progs already uses -1.

Well that is a bug then.  This will be seen by the shell (parent
process) as 255.

 
> > +
> >   /* defined in test_progs.h */
> >   struct test_env env = {};
> >   
> > @@ -740,7 +742,7 @@ int main(int argc, char **argv)
> >   	close(env.saved_netns_fd);
> >   
> >   	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
> > -		return EXIT_FAILURE;
> > +		return EXIT_NO_TEST;
> >   
> >   	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
> >   }
> > 

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: test_progs use another shell exit on non-actions
  2020-07-03 11:59         ` Jesper Dangaard Brouer
@ 2020-07-04 18:15           ` Yonghong Song
  0 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2020-07-04 18:15 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: bpf, Andrii Nakryiko, Hangbin Liu, Daniel Borkmann,
	Alexei Starovoitov, vkabatov, jbenc



On 7/3/20 4:59 AM, Jesper Dangaard Brouer wrote:
> On Thu, 2 Jul 2020 14:24:14 -0700
> Yonghong Song <yhs@fb.com> wrote:
> 
>> On 7/2/20 10:59 AM, Jesper Dangaard Brouer wrote:
>>> This is a follow up adjustment to commit 6c92bd5cd465 ("selftests/bpf:
>>> Test_progs indicate to shell on non-actions"), that returns shell exit
>>> indication EXIT_FAILURE (value 1) when user selects a non-existing test.
>>>
>>> The problem with using EXIT_FAILURE is that a shell script cannot tell
>>> the difference between a non-existing test and the test failing.
>>>
>>> This patch uses value 2 as shell exit indication.
>>> (Aside note unrecognized option parameters use value 64).
>>>
>>> Fixes: 6c92bd5cd465 ("selftests/bpf: Test_progs indicate to shell on non-actions")
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>>    tools/testing/selftests/bpf/test_progs.c |    4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
>>> index 104e833d0087..e8f7cd5dbae4 100644
>>> --- a/tools/testing/selftests/bpf/test_progs.c
>>> +++ b/tools/testing/selftests/bpf/test_progs.c
>>> @@ -12,6 +12,8 @@
>>>    #include <string.h>
>>>    #include <execinfo.h> /* backtrace */
>>>    
>>> +#define EXIT_NO_TEST 2
>>
>> How do you ensure this won't collide with other exit code
>> from other library functions (e.g., error code 64 is used
>> for unrecognized option which I have no idea what 64 means)?
> 
> I expect 64 comes from: /usr/include/sysexits.h
>   #define EX_USAGE        64      /* command line usage error */

Thanks for the pointer.

> 
> 
>> Maybe -2 for the exit code?
> 
> No. The process's exit status must be a number between 0 and 255, as
> defined in man exit(3). (run: 'man 3 exit' as there are many manpages
> named exit).

Yes, if user app exits with -2, it actually prints 254, -1 for 255...

> 
> But don't use above 127, because that is usually used for indicating
> signals.  E.g. 139 means 11=SIGSEGV $((139 & 127))=11.  POSIX defines
> in man wait(3p) check WIFSIGNALED(STATUS) and WTERMSIG(139)=11.
> (Hint: cmd 'kill -l' list signals and their numbers).
> 
> I bring up Segmentation fault explicitly, as we are seeing these happen
> with different tests (that are part of test_progs).  CI people writing
> these shell-scripts could pickup these hints and report them, if that
> makes sense.

Make sense to use from 1 - 127 range for normal exit.

> 
> 
>> test_progs already uses -1.
> 
> Well that is a bug then.  This will be seen by the shell (parent
> process) as 255.

I think previously people may just check test_progs return 0 or non-0.
Since here you will try to check different error return code, It makes
sense to do an audit to explicitly define all return values. So this
way, tools can have a reliable way to check exit code.

> 
>   
>>> +
>>>    /* defined in test_progs.h */
>>>    struct test_env env = {};
>>>    
>>> @@ -740,7 +742,7 @@ int main(int argc, char **argv)
>>>    	close(env.saved_netns_fd);
>>>    
>>>    	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
>>> -		return EXIT_FAILURE;
>>> +		return EXIT_NO_TEST;
>>>    
>>>    	return env.fail_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
>>>    }
>>>
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-07-04 18:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 21:44 [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Jesper Dangaard Brouer
2020-07-01 21:44 ` [PATCH bpf-next V3 1/3] selftests/bpf: test_progs indicate to shell on non-actions Jesper Dangaard Brouer
2020-07-01 21:46   ` Andrii Nakryiko
2020-07-02 13:47   ` Jesper Dangaard Brouer
2020-07-02 17:59     ` [PATCH bpf-next] selftests/bpf: test_progs use another shell exit " Jesper Dangaard Brouer
2020-07-02 21:24       ` Yonghong Song
2020-07-03 11:59         ` Jesper Dangaard Brouer
2020-07-04 18:15           ` Yonghong Song
2020-07-01 21:44 ` [PATCH bpf-next V3 2/3] selftests/bpf: test_progs option for getting number of tests Jesper Dangaard Brouer
2020-07-01 21:47   ` Andrii Nakryiko
2020-07-01 21:44 ` [PATCH bpf-next V3 3/3] selftests/bpf: test_progs option for listing test names Jesper Dangaard Brouer
2020-07-01 21:48   ` Andrii Nakryiko
2020-07-01 22:22 ` [PATCH bpf-next V3 0/3] BPF selftests test runner test_progs improvement for scripting Alexei Starovoitov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.