[bpf-next,v4,1/3] selftests/bpf: Clean up call sites of stdio_restore()

Commit Message

Amery Hung March 4, 2025, 4:36 p.m. UTC

There is no need to call a bunch of stdio_restore() in test_progs if the
scope of stdio redirection is reduced to what it needs to be: only
hijacking tests/subtests' stdio.

Also remove an unnecessary check of env.stdout_saved in the crash handler.

Signed-off-by: Amery Hung <[e-mail]>
---
 tools/testing/selftests/bpf/test_progs.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Comments

Eduard Zingerman March 4, 2025, 9:48 p.m. UTC | #1

On Tue, 2025-03-04 at 08:36 -0800, Amery Hung wrote:
> There is no need to call a bunch of stdio_restore() in test_progs if the
> scope of stdio redirection is reduced to what it needs to be: only
> hijacking tests/subtests' stdio.
> 
> Also remove an unnecessary check of env.stdout_saved in the crash handler.
> 
> Signed-off-by: Amery Hung <[e-mail]>
> ---

If anyone else would look at this commit, here is an alternative
description:
- functions reset_affinity() and restore_netns() are only called from
  run_one_test();
- beside other places stdio_restore() is called from reset_affinity(),
  restore_netns() and run_one_test() itself;
- this commit moves stdio_restore() call in run_one_test() so that
  it executes before reset_affinity() and restore_netns().

Acked-by: Eduard Zingerman <[e-mail]>

[...]

> @@ -1943,6 +1938,9 @@ int main(int argc, char **argv)
>  
>  	sigaction(SIGSEGV, &sigact, NULL);
>  
> +	env.stdout_saved = stdout;
> +	env.stderr_saved = stderr;
> +

Nit: why moving these?

>  	env.secs_till_notify = 10;
>  	env.secs_till_kill = 120;
>  	err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> @@ -1969,9 +1967,6 @@ int main(int argc, char **argv)
>  		return -1;
>  	}
>  
> -	env.stdout_saved = stdout;
> -	env.stderr_saved = stderr;
> -
>  	env.has_testmod = true;
>  	if (!env.list_test_names) {
>  		/* ensure previous instance of the module is unloaded */

Amery Hung March 4, 2025, 11:39 p.m. UTC | #2

On Tue, Mar 4, 2025 at 1:48 PM Eduard Zingerman <[e-mail]> wrote:
>
> On Tue, 2025-03-04 at 08:36 -0800, Amery Hung wrote:
> > There is no need to call a bunch of stdio_restore() in test_progs if the
> > scope of stdio redirection is reduced to what it needs to be: only
> > hijacking tests/subtests' stdio.
> >
> > Also remove an unnecessary check of env.stdout_saved in the crash handler.
> >
> > Signed-off-by: Amery Hung <[e-mail]>
> > ---
>
> If anyone else would look at this commit, here is an alternative
> description:
> - functions reset_affinity() and restore_netns() are only called from
>   run_one_test();
> - beside other places stdio_restore() is called from reset_affinity(),
>   restore_netns() and run_one_test() itself;
> - this commit moves stdio_restore() call in run_one_test() so that
>   it executes before reset_affinity() and restore_netns().
>
> Acked-by: Eduard Zingerman <[e-mail]>
>

I can improve the commit message in the next respin.

Thanks,
Amery

> [...]
>
> > @@ -1943,6 +1938,9 @@ int main(int argc, char **argv)
> >
> >       sigaction(SIGSEGV, &sigact, NULL);
> >
> > +     env.stdout_saved = stdout;
> > +     env.stderr_saved = stderr;
> > +
>
> Nit: why moving these?

If we assign env.stdout_saved at the very beginning, crash_handler()
can just call stdio_restore() without checking if env.stdout_saved is
set or not.

>
> >       env.secs_till_notify = 10;
> >       env.secs_till_kill = 120;
> >       err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> > @@ -1969,9 +1967,6 @@ int main(int argc, char **argv)
> >               return -1;
> >       }
> >
> > -     env.stdout_saved = stdout;
> > -     env.stderr_saved = stderr;
> > -
> >       env.has_testmod = true;
> >       if (!env.list_test_names) {
> >               /* ensure previous instance of the module is unloaded */
>
>

Eduard Zingerman March 5, 2025, 12:14 a.m. UTC | #3

On Tue, 2025-03-04 at 15:39 -0800, Amery Hung wrote:
> On Tue, Mar 4, 2025 at 1:48 PM Eduard Zingerman <[e-mail]> wrote:
> > 
> > On Tue, 2025-03-04 at 08:36 -0800, Amery Hung wrote:
> > > There is no need to call a bunch of stdio_restore() in test_progs if the
> > > scope of stdio redirection is reduced to what it needs to be: only
> > > hijacking tests/subtests' stdio.
> > > 
> > > Also remove an unnecessary check of env.stdout_saved in the crash handler.
> > > 
> > > Signed-off-by: Amery Hung <[e-mail]>
> > > ---
> > 
> > If anyone else would look at this commit, here is an alternative
> > description:
> > - functions reset_affinity() and restore_netns() are only called from
> >   run_one_test();
> > - beside other places stdio_restore() is called from reset_affinity(),
> >   restore_netns() and run_one_test() itself;
> > - this commit moves stdio_restore() call in run_one_test() so that
> >   it executes before reset_affinity() and restore_netns().
> > 
> > Acked-by: Eduard Zingerman <[e-mail]>
> > 
> 
> I can improve the commit message in the next respin.

If there would be a respin, otherwise no need to worry.

[...]

> > > @@ -1943,6 +1938,9 @@ int main(int argc, char **argv)
> > > 
> > >       sigaction(SIGSEGV, &sigact, NULL);
> > > 
> > > +     env.stdout_saved = stdout;
> > > +     env.stderr_saved = stderr;
> > > +
> > 
> > Nit: why moving these?
> 
> If we assign env.stdout_saved at the very beginning, crash_handler()
> can just call stdio_restore() without checking if env.stdout_saved is
> set or not.

Understood, thank you.

[...]

Patch

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 0cb759632225..ab0f2fed3c58 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -474,8 +474,6 @@  static void dump_test_log(const struct prog_test_def *test,
 	print_test_result(test, test_state);
 }
 
-static void stdio_restore(void);
-
 /* A bunch of tests set custom affinity per-thread and/or per-process. Reset
  * it after each test/sub-test.
  */
@@ -490,13 +488,11 @@  static void reset_affinity(void)
 
 	err = sched_setaffinity(0, sizeof(cpuset), &cpuset);
 	if (err < 0) {
-		stdio_restore();
 		fprintf(stderr, "Failed to reset process affinity: %d!\n", err);
 		exit(EXIT_ERR_SETUP_INFRA);
 	}
 	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(EXIT_ERR_SETUP_INFRA);
 	}
@@ -514,7 +510,6 @@  static void save_netns(void)
 static void restore_netns(void)
 {
 	if (setns(env.saved_netns_fd, CLONE_NEWNET) == -1) {
-		stdio_restore();
 		perror("setns(CLONE_NEWNS)");
 		exit(EXIT_ERR_SETUP_INFRA);
 	}
@@ -1270,8 +1265,7 @@  void crash_handler(int signum)
 
 	sz = backtrace(bt, ARRAY_SIZE(bt));
 
-	if (env.stdout_saved)
-		stdio_restore();
+	stdio_restore();
 	if (env.test) {
 		env.test_state->error_cnt++;
 		dump_test_log(env.test, env.test_state, true, false, NULL);
@@ -1400,6 +1394,8 @@  static void run_one_test(int test_num)
 
 	state->tested = true;
 
+	stdio_restore();
+
 	if (verbose() && env.worker_id == -1)
 		print_test_result(test, state);
 
@@ -1408,7 +1404,6 @@  static void run_one_test(int test_num)
 	if (test->need_cgroup_cleanup)
 		cleanup_cgroup_environment();
 
-	stdio_restore();
 	free(stop_libbpf_log_capture());
 
 	dump_test_log(test, state, false, false, NULL);
@@ -1943,6 +1938,9 @@  int main(int argc, char **argv)
 
 	sigaction(SIGSEGV, &sigact, NULL);
 
+	env.stdout_saved = stdout;
+	env.stderr_saved = stderr;
+
 	env.secs_till_notify = 10;
 	env.secs_till_kill = 120;
 	err = argp_parse(&argp, argc, argv, 0, NULL, &env);
@@ -1969,9 +1967,6 @@  int main(int argc, char **argv)
 		return -1;
 	}
 
-	env.stdout_saved = stdout;
-	env.stderr_saved = stderr;
-
 	env.has_testmod = true;
 	if (!env.list_test_names) {
 		/* ensure previous instance of the module is unloaded */