| Message ID | 159405481655.1091613.6475075949369245359.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BPF selftests test runner 'test_progs' use proper shell exit codes | expand |
On Mon, Jul 6, 2020 at 10:00 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > There are a number of places in test_progs that use minus-1 as the argument > to exit(). This improper use as a process exit status is masked to be a > number between 0 and 255 as defined in man exit(3). nit: I wouldn't call it improper use, as it's a well defined behavior (lower byte of returned integer). > > This patch use two different positive exit codes instead, to allow a shell typo: uses > script to tell the two error cases apart. > > Fixes: fd27b1835e70 ("selftests/bpf: Reset process and thread affinity after each test/sub-test") > Fixes: 811d7e375d08 ("bpf: selftests: Restore netns after each test") > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > tools/testing/selftests/bpf/test_progs.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index e8f7cd5dbae4..50803b080593 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -12,7 +12,9 @@ > #include <string.h> > #include <execinfo.h> /* backtrace */ > > -#define EXIT_NO_TEST 2 > +#define EXIT_NO_TEST 2 > +#define EXIT_ERR_NETNS 3 > +#define EXIT_ERR_RESET_AFFINITY 4 Let's not overdo this with too granular error codes? All of those seem to be just a failure, is there any practical need to differentiate between NETNS vs RESET_AFFINITY failure? I'd go with 3 values: 1 - at least one test failed 2 - no tests were selected 3 - "infra" (not a test-specific failure) error (like netns or affinity failed). Thoughts? [...]
On Mon, 6 Jul 2020 15:17:57 -0700 Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Mon, Jul 6, 2020 at 10:00 AM Jesper Dangaard Brouer > <brouer@redhat.com> wrote: > > > > There are a number of places in test_progs that use minus-1 as the argument > > to exit(). This improper use as a process exit status is masked to be a > > number between 0 and 255 as defined in man exit(3). > > nit: I wouldn't call it improper use, as it's a well defined behavior > (lower byte of returned integer). > > > > > This patch use two different positive exit codes instead, to allow a shell > > typo: uses > > > script to tell the two error cases apart. > > > > Fixes: fd27b1835e70 ("selftests/bpf: Reset process and thread affinity after each test/sub-test") > > Fixes: 811d7e375d08 ("bpf: selftests: Restore netns after each test") > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > tools/testing/selftests/bpf/test_progs.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > > index e8f7cd5dbae4..50803b080593 100644 > > --- a/tools/testing/selftests/bpf/test_progs.c > > +++ b/tools/testing/selftests/bpf/test_progs.c > > @@ -12,7 +12,9 @@ > > #include <string.h> > > #include <execinfo.h> /* backtrace */ > > > > -#define EXIT_NO_TEST 2 > > +#define EXIT_NO_TEST 2 > > +#define EXIT_ERR_NETNS 3 > > +#define EXIT_ERR_RESET_AFFINITY 4 > > Let's not overdo this with too granular error codes? All of those seem > to be just a failure, is there any practical need to differentiate > between NETNS vs RESET_AFFINITY failure? I agree, because both cases (NETNS vs RESET_AFFINITY) print to stderr, which makes it possible to troubleshoot for a human afterwards. The shell script just need to differentiate that is an "infra" setup issue, as we e.g. might want to allow the RPM build to continue in those cases. > I'd go with 3 values: > > 1 - at least one test failed > 2 - no tests were selected > 3 - "infra" (not a test-specific failure) error (like netns or affinity failed). > > Thoughts? Sure, I can do this. What define name reflect this best: EXIT_ERR_SETUP ? EXIT_ERR_INFRA ? EXIT_ERR_SETUP_INFRA ?
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index e8f7cd5dbae4..50803b080593 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -12,7 +12,9 @@ #include <string.h> #include <execinfo.h> /* backtrace */ -#define EXIT_NO_TEST 2 +#define EXIT_NO_TEST 2 +#define EXIT_ERR_NETNS 3 +#define EXIT_ERR_RESET_AFFINITY 4 /* defined in test_progs.h */ struct test_env env = {}; @@ -113,13 +115,13 @@ static void reset_affinity() { if (err < 0) { stdio_restore(); fprintf(stderr, "Failed to reset process affinity: %d!\n", err); - exit(-1); + exit(EXIT_ERR_RESET_AFFINITY); } err = pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset); if (err < 0) { stdio_restore(); fprintf(stderr, "Failed to reset thread affinity: %d!\n", err); - exit(-1); + exit(EXIT_ERR_RESET_AFFINITY); } } @@ -128,7 +130,7 @@ static void save_netns(void) env.saved_netns_fd = open("/proc/self/ns/net", O_RDONLY); if (env.saved_netns_fd == -1) { perror("open(/proc/self/ns/net)"); - exit(-1); + exit(EXIT_ERR_NETNS); } } @@ -137,7 +139,7 @@ static void restore_netns(void) if (setns(env.saved_netns_fd, CLONE_NEWNET) == -1) { stdio_restore(); perror("setns(CLONE_NEWNS)"); - exit(-1); + exit(EXIT_ERR_NETNS); } }
There are a number of places in test_progs that use minus-1 as the argument to exit(). This improper use as a process exit status is masked to be a number between 0 and 255 as defined in man exit(3). This patch use two different positive exit codes instead, to allow a shell script to tell the two error cases apart. Fixes: fd27b1835e70 ("selftests/bpf: Reset process and thread affinity after each test/sub-test") Fixes: 811d7e375d08 ("bpf: selftests: Restore netns after each test") Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- tools/testing/selftests/bpf/test_progs.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)