diff mbox series

[bpf-next,4/4] selftests/bpf: test_progs: remove asserts from subtests

Message ID 20190814164742.208909-5-sdf@google.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series selftests/bpf: test_progs: misc fixes | expand

Commit Message

Stanislav Fomichev Aug. 14, 2019, 4:47 p.m. UTC 
Otherwise they can bring the whole process down.

Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/prog_tests/bpf_obj_id.c     | 30 ++++++++++++++-----
 .../selftests/bpf/prog_tests/map_lock.c       | 20 ++++++++-----
 .../selftests/bpf/prog_tests/spinlock.c       | 10 ++++---
 .../bpf/prog_tests/stacktrace_build_id.c      | 11 +++++--
 .../bpf/prog_tests/stacktrace_build_id_nmi.c  | 11 +++++--
 5 files changed, 57 insertions(+), 25 deletions(-)

Comments

Andrii Nakryiko Aug. 14, 2019, 7:52 p.m. UTC | #1 
On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Otherwise they can bring the whole process down.
>
> Cc: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

This is probably why you added all that extra logging in __test__fail(), right?

So had a low-priority TODO item to add another CHECK()-like macro that
would only report failure (but won't bump/log success). Seems like
this is something that would be useful for these asserts?

What do you think about either QCHECK() (for "quiet" check) or surely
we can also do ASSERT (but it's less obvious that it won't log success
and it's also not obvious that it won't actually terminate test
immediately).

Then inside that QCHECK() you can log file:line number, similar to
CHECK(), but only for failure case.

Thoughts?
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index f57e0c625de3..4ec8c4e9e9a1 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -48,16 +48,23 @@  void test_bpf_obj_id(void)
 		/* test_obj_id.o is a dumb prog. It should never fail
 		 * to load.
 		 */
-		if (err)
+		if (err) {
 			test__fail();
-		assert(!err);
+			continue;
+		}
 
 		/* Insert a magic value to the map */
 		map_fds[i] = bpf_find_map(__func__, objs[i], "test_map_id");
-		assert(map_fds[i] >= 0);
+		if (map_fds[i] < 0) {
+			test__fail();
+			goto done;
+		}
 		err = bpf_map_update_elem(map_fds[i], &array_key,
 					  &array_magic_value, 0);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 
 		/* Check getting map info */
 		info_len = sizeof(struct bpf_map_info) * 2;
@@ -96,9 +103,15 @@  void test_bpf_obj_id(void)
 		prog_infos[i].map_ids = ptr_to_u64(map_ids + i);
 		prog_infos[i].nr_map_ids = 2;
 		err = clock_gettime(CLOCK_REALTIME, &real_time_ts);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 		err = clock_gettime(CLOCK_BOOTTIME, &boot_time_ts);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 		err = bpf_obj_get_info_by_fd(prog_fds[i], &prog_infos[i],
 					     &info_len);
 		load_time = (real_time_ts.tv_sec - boot_time_ts.tv_sec)
@@ -224,7 +237,10 @@  void test_bpf_obj_id(void)
 		nr_id_found++;
 
 		err = bpf_map_lookup_elem(map_fd, &array_key, &array_value);
-		assert(!err);
+		if (err) {
+			test__fail();
+			goto done;
+		}
 
 		err = bpf_obj_get_info_by_fd(map_fd, &map_info, &info_len);
 		CHECK(err || info_len != sizeof(struct bpf_map_info) ||
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index 12123ff1f31f..e7663721fb57 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -56,17 +56,21 @@  void test_map_lock(void)
 	bpf_map_update_elem(map_fd[0], &key, vars, BPF_F_LOCK);
 
 	for (i = 0; i < 4; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &spin_lock_thread, &prog_fd) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &spin_lock_thread, &prog_fd))
+			goto close_prog;
 	for (i = 4; i < 6; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &parallel_map_access, &map_fd[i - 4]) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &parallel_map_access, &map_fd[i - 4]))
+			goto close_prog;
 	for (i = 0; i < 4; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&prog_fd);
+		if (pthread_join(thread_id[i], &ret) ||
+		    ret != (void *)&prog_fd)
+			goto close_prog;
 	for (i = 4; i < 6; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&map_fd[i - 4]);
+		if (pthread_join(thread_id[i], &ret) ||
+		    ret != (void *)&map_fd[i - 4])
+			goto close_prog;
 	goto close_prog_noerr;
 close_prog:
 	test__fail();
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index e843336713e8..5f32a913f732 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -16,11 +16,13 @@  void test_spinlock(void)
 		goto close_prog;
 	}
 	for (i = 0; i < 4; i++)
-		assert(pthread_create(&thread_id[i], NULL,
-				      &spin_lock_thread, &prog_fd) == 0);
+		if (pthread_create(&thread_id[i], NULL,
+				   &spin_lock_thread, &prog_fd))
+			goto close_prog;
+
 	for (i = 0; i < 4; i++)
-		assert(pthread_join(thread_id[i], &ret) == 0 &&
-		       ret == (void *)&prog_fd);
+		if (pthread_join(thread_id[i], &ret) || ret != (void *)&prog_fd)
+			goto close_prog;
 	goto close_prog_noerr;
 close_prog:
 	test__fail();
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index ac44fda84833..d74464faebd7 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -51,9 +51,14 @@  void test_stacktrace_build_id(void)
 		  "err %d errno %d\n", err, errno))
 		goto disable_pmu;
 
-	assert(system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")
-	       == 0);
-	assert(system("./urandom_read") == 0);
+	if (system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")) {
+		test__fail();
+		goto disable_pmu;
+	}
+	if (system("./urandom_read")) {
+		test__fail();
+		goto disable_pmu;
+	}
 	/* disable stack trace collection */
 	key = 0;
 	val = 1;
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index 9557b7dfb782..e886911928bc 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -82,9 +82,14 @@  void test_stacktrace_build_id_nmi(void)
 		  "err %d errno %d\n", err, errno))
 		goto disable_pmu;
 
-	assert(system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")
-	       == 0);
-	assert(system("taskset 0x1 ./urandom_read 100000") == 0);
+	if (system("dd if=/dev/urandom of=/dev/zero count=4 2> /dev/null")) {
+		test__fail();
+		goto disable_pmu;
+	}
+	if (system("taskset 0x1 ./urandom_read 100000")) {
+		test__fail();
+		goto disable_pmu;
+	}
 	/* disable stack trace collection */
 	key = 0;
 	val = 1;