[LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang]

On Tue, May 13, 2025 at 8:52 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi all,
>
> ...
> > > +++ b/lib/tst_test.c
> > > @@ -59,7 +59,6 @@ static const char *tid;
> > >  static int iterations = 1;
> > >  static float duration = -1;
> > >  static float timeout_mul = -1;
> > > -static pid_t lib_pid;
> > >  static int mntpoint_mounted;
> > >  static int ovl_mounted;
> > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> > >             tst_futexes = (char *)results + sizeof(struct results);
> > >             tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> > >     }
> > > -   results->lib_pid = lib_pid;
> > > +
> > > +   results->lib_pid = 0;
> > > +   results->main_pid = 0;
>
> nit: Is it really needed to int them to 0? Because they should be 0 due the
> default struct value, right?

Right!

I think we need to initialize them, because the test may fail/break
just before creating main_pid, then test cleanup work needs to
invoke tst_vbrk_ and tst_res_ somewhere that compares pid to
invoke the corresponding cleanup work.

>
> > >  }
>
> > >  static void cleanup_ipc(void)
> > > @@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> > >      * If tst_brk() is called from some of the C helpers even before the
> > >      * library was initialized, just exit.
> > >      */
> > > -   if (!results->lib_pid)
> > > +   if (!results || !results->lib_pid)
> > >             exit(TTYPE_RESULT(ttype));
>
> > >     update_results(TTYPE_RESULT(ttype));
> > > @@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
> > >             tst_test->forks_child = 1;
> > >     }
>
> > > +   setup_ipc();
> > > +
> > >     if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
> > >             tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");
>
> > > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> > >     if (tst_test->hugepages.number)
> > >             tst_reserve_hugepages(&tst_test->hugepages);
>
> > > -   setup_ipc();
> > > -
>
> > I suppose that this has to go before the tst_reserve_hugepages() so that
> > we have results->lib_pid defined and properly clean up after the
> > hugepages.
>
> Why? Is that due possible tst_brk() calls in tst_reserve_hugepages()?
> (Which uses SAFE_* macros.) That would trigger Because there are tst_brk() calls before.
>
> Also why not assign results->lib_pid = getpid() at the beginning of
> tst_run_tcases() ?

No, we can't.

Because we moved the lib_pid in the IPC region, which allocated memory
in the setup_ipc() function.

>
> Kind regards,
> Petr
>
> > However for that to work we have to set the
> > results->lib_pid directly in the setup_ipc().
>
> > >     if (tst_test->bufs)
> > >             tst_buffers_alloc(tst_test->bufs);
>
> > > @@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> > >     unsigned int test_variants = 1;
> > >     struct utsname uval;
>
> > > -   lib_pid = getpid();
> > >     tst_test = self;
>
> > >     do_setup(argc, argv);
> > > +
> > > +   results->lib_pid = getpid();
>
> > Setting it here is too late.
>
> > Other than that the patch looks good to me.
>


-- 
Regards,
Li Wang