| Message ID | 159371277981.942611.89883359210042038.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: test_progs use another shell exit on non-actions | expand |
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; > } > >
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; > > } > >
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; >>> } >>> >
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; }
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(-)