[LTP] [PATCH v1] Fix: dirtyc0w_shmem child process exit with error due to uninitialized lib_pid

[Cyril Hrubis]

Hi!
> I think we'd better move them into a new structure dedicated to storing
> library infrastructure.

Sounds good.

> > > As LTP library already provides a tst_reinit() function for child
> > processes
> > > spawned via exec()/execlp() to restore their test context.
> > >
> > > We could consider two approaches:
> > >
> > > 1. Create a separate IPC region to store infrastructure state(e.g.,
> > > main_pid, lib_pid)
> > > in a new struct tst_meta_info. The child can then access this data via
> > > tst_reinit()
> > > without modifying the struct results.
> >
> > I would like to keep a single IPC region because we have to pass a
> > path to it to all children too.
> >
> > Also given that the minimal amount of memory we can allocate for IPC is
> > a page we can as well have two structures, one for data and one for
> > library infrastructure bits and place these structures in there
> > manually. All we need to do is to make sure that we place them at
> > aligned offsets.
> >
> 
> Yes, that was my thought as well. Something maybe like:
> 
> #define LTP_MAGIC 0x4C54504D

I suppose that we will check this in the tst_reinit() right? That is
nice improvement.

> struct context {
>         pid_t main_pid;
>         pid_t lib_pid;
>         struct timespec tst_start_time;
>         /*
>          * This is set by a call to tst_brk() with TBROK parameter and means
>          * that the test should exit immediately.
>          */
>         unsigned int runtime;
>         unsigned int overall_time;
>         int abort_flag;
>         int mntpoint_mounted;
>         int ovl_mounted;
>         int tdebug;
> };
> 
> struct results {
>         int passed;
>         int skipped;
>         int failed;
>         int warnings;
>         int broken;
> };
> 
> static struct ipc_block {
>         unsigned int magic;
>         struct context context;
>         struct results results;
>         futex_t futexes[];
> };

Maybe it would make sense to switch to the uint32_t and int32_t here
when we are doing the cleanup. Currently we we call tst_reinit() from a
32bit process and the parent was 64bit it wouldn't work at all. We do
not have tests like that at the moment but we may possibly end up in
this situation later on.

> static struct ipc_block *ipc = NULL;
> static struct context *context = NULL;
> static struct results *results = NULL;
> 
> #define TST_CONTEXT(ipc) ((struct context *)&(ipc)->context)
> #define TST_RESULTS(ipc) ((struct results *)&(ipc)->results)
> #define TST_FUTEXES(ipc) ((futex_t *)(ipc)->futexes)

Is there a reason for these macros? The types should be correct, so
there should be no need for the casts.

-- 
Cyril Hrubis
chrubis@suse.cz