On Tue, Mar 4, 2025 at 5:36 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 3/4/25 8:36 AM, Amery Hung wrote:
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index ab0f2fed3c58..5b89f6ca5a0a 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -88,7 +88,11 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt)
> > #endif
> > }
> >
> > -static void stdio_restore_cleanup(void)
> > +static pthread_mutex_t stdout_lock = PTHREAD_MUTEX_INITIALIZER;
> > +
> > +static bool in_crash_handler(void);
> > +
> > +static void stdio_restore(void)
> > {
> > #ifdef __GLIBC__
> > if (verbose() && env.worker_id == -1) {
> > @@ -98,34 +102,34 @@ static void stdio_restore_cleanup(void)
> >
> > fflush(stdout);
> >
> > - if (env.subtest_state) {
> > + pthread_mutex_lock(&stdout_lock);
> > +
> > + if (!env.subtest_state || in_crash_handler()) {
>
> Can the stdio restore be done in the crash_handler() itself instead of having a
> special case here and adding another in_crash_handler()?
>
> Theoretically, the crash_handler() only needs to
> fflush(stdout /* whatever the current stdout is */) and...
>
> > + if (stdout == env.stdout_saved)
> > + goto out;
> > +
> > + fclose(env.test_state->stdout_saved);
> > + env.test_state->stdout_saved = NULL;
> > + stdout = env.stdout_saved;
> > + stderr = env.stderr_saved;
>
> ... restore std{out,err} = env.std{out,err}_saved.
>
> At the crash point, it does not make a big difference to
> fclose(evn.test_state->stdout_saved) or not?
>
> If the crash_handler() does not close the stdout that the traffic monitor might
> potentially be using, then crash_handler() does not need to take mutex, right?
>
You are right. I think it is simpler to not let stdio_restore() handle
the crash case, and just do the following in the crash handler.
fflush(stdout);
stdout = env.stdout_saved;
stderr = env.stderr_saved;
> > + } else {
> > fclose(env.subtest_state->stdout_saved);
> > env.subtest_state->stdout_saved = NULL;
> > stdout = env.test_state->stdout_saved;
> > stderr = env.test_state->stdout_saved;
> > - } else {
> > - fclose(env.test_state->stdout_saved);
> > - env.test_state->stdout_saved = NULL;
> > }
> > +out:
> > + pthread_mutex_unlock(&stdout_lock);
> > #endif
> > }
> >
>
> [ ... ]
>
> > +static bool in_crash_handler(void)
> > +{
> > + struct sigaction sigact;
> > +
> > + /* sa_handler will be cleared if invoked since crash_handler is
> > + * registered with SA_RESETHAND
> > + */
> > + sigaction(SIGSEGV, NULL, &sigact);
> > +
> > + return sigact.sa_handler != crash_handler;
> > +}
> > +
