[v2] lib: Improve doc

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 lib/README.md | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Hi Avinesh,

> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  lib/README.md | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)

> diff --git a/lib/README.md b/lib/README.md
> index ccb1cf1da..dd79e70f4 100644
> --- a/lib/README.md
> +++ b/lib/README.md
> @@ -27,16 +27,16 @@

>  When a test is executed the very first thing to happen is that we check for
>  various test prerequisites. These are described in the tst\_test structure and
> -range from simple '.require\_root' to a more complicated kernel .config boolean
> +range from simple '.needs\_root' to a more complicated kernel .config boolean
Good catch!

>  expressions such as: "CONFIG\_X86\_INTEL\_UMIP=y | CONFIG\_X86\_UMIP=y".

> -If all checks are passed the process carries on with setting up the test
> +If all checks are passed, the process continues with setting up the test
>  environment as requested in the tst\_test structure. There are many different
>  setup steps that have been put into the test library again ranging from rather
>  simple creation of a unique test temporary directory to a bit more complicated
>  ones such as preparing, formatting, and mounting a block device.

> -The test library also intializes shrared memory used for IPC at this step.
> +The test library also initializes shared memory used for IPC at this step.

>  Once all the prerequisites are checked and test environment has been prepared
>  we can move on executing the testcase itself. The actual test is executed in a
> @@ -62,12 +62,11 @@ for test_variants:
>  		fork_testrun()
>  ```

> -Before we fork() the test process the test library sets up a timeout alarm and
> -also a heartbeat signal handlers and also sets up an alarm(2) accordingly to
> -the test timeout. When a test times out the test library gets SIGALRM and the
> -alarm handler mercilessly kills all forked children by sending SIGKILL to the
> -whole process group. The heartbeat handler is used by the test process to reset
> -this timer for example when the test functions run in a loop.
> +Before we fork() the test process, the test library sets up a timeout alarm and
nit: "Before we fork " or "Before 'fork() '.

> +also a heartbeat signal handler. When a test times out, the test library gets

I'm not sure about this, isn't the original description correct? (1) timeout
alarm() 2) a heartbeat signal handlers 3) second alarm() accordingly to the test
timeout?

I mean there are 2 alarm() calls in tst_test.c.

Otherwise LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> +SIGALRM and the alarm handler mercilessly kills all forked children by sending
> +SIGKILL to the whole process group. The heartbeat handler is used by the test
> +process to reset this timer for example when the test functions run in a loop.

>  With that done we finally fork() the test process. The test process firstly
>  resets signal handlers and sets its pid to be a process group leader so that we
> @@ -97,14 +96,14 @@ usually better option than exiting it in the middle.
>  The test cleanup() is also called by the tst\_brk() handler in order to cleanup
>  before exiting the test process, hence it must be able to cope even with
>  partial test setup. Usually it suffices to make sure to clean up only
> -resources that already have been set up and to do that in an inverse order that
> -we did in setup().
> +resources that already have been set up and to do that in the reverse order
> +that we did in setup().

>  Once the test process exits or leaves the run() or run\_all() function the test
>  library wakes up from the waitpid() call, and checks if the test process
>  exited normally.

> -Once the testrun is finished the test library does a cleanup() as well to clean
> +Once the testrun is finished, the test library does a cleanup() as well to clean
>  up resources set up in the test library setup(), reports test results and
>  finally exits the process.

> @@ -126,8 +125,8 @@ This especially means that:
>  - While the test results are, by the design, propagated to the test library
>    we may still miss a child that gets killed by a signal or exits unexpectedly.

> -The test writer should, because of this, take care for reaping these proceses
> -properly, in most cases this could be simply done by calling
> +The test writer should, because of this, take care of reaping these
> +processes properly, in most cases this could be simply done by calling
>  tst\_reap\_children() to collect and dissect deceased.

>  Also note that tst\_brk() does exit only the current process, so if a child
> @@ -136,9 +135,9 @@ exits.

>  ### Test library and exec()

> -The piece of mapped memory to store the results to is not preserved over
> +The piece of mapped memory to store the results is not preserved over
>  exec(2), hence to use the test library from a binary started by an exec() it
> -has to be remaped. In this case the process must to call tst\_reinit() before
> +has to be remapped. In this case, the process must call tst\_reinit() before
>  calling any other library functions. In order to make this happen the program
>  environment carries LTP\_IPC\_PATH variable with a path to the backing file on
>  tmpfs. This also allows us to use the test library from shell testcases.
> @@ -148,5 +147,5 @@ tmpfs. This also allows us to use the test library from shell testcases.
>  The piece of mapped memory is also used as a base for a futex-based
>  synchronization primitives called checkpoints. And as said previously the
>  memory can be mapped to any process by calling the tst\_reinit() function. As a
> -matter of a fact there is even a tst\_checkpoint binary that allows us to use
> +matter of a fact, there is even a tst\_checkpoint binary that allows us to use
>  the checkpoints from shell code as well.

Hi Petr,

Thank you for the review.

On Wednesday, January 3, 2024 1:55:16 PM CET Petr Vorel wrote:
> Hi Avinesh,
> 
> > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > ---
> > 
> >  lib/README.md | 33 ++++++++++++++++-----------------
> >  1 file changed, 16 insertions(+), 17 deletions(-)
> > 
> > diff --git a/lib/README.md b/lib/README.md
> > index ccb1cf1da..dd79e70f4 100644
> > --- a/lib/README.md
> > +++ b/lib/README.md
> > @@ -27,16 +27,16 @@
> > 
> >  When a test is executed the very first thing to happen is that we check
> >  for
> >  various test prerequisites. These are described in the tst\_test
> >  structure and> 
> > -range from simple '.require\_root' to a more complicated kernel .config
> > boolean +range from simple '.needs\_root' to a more complicated kernel
> > .config boolean
> Good catch!
> 
> >  expressions such as: "CONFIG\_X86\_INTEL\_UMIP=y | CONFIG\_X86\_UMIP=y".
> > 
> > -If all checks are passed the process carries on with setting up the test
> > +If all checks are passed, the process continues with setting up the test
> > 
> >  environment as requested in the tst\_test structure. There are many
> >  different setup steps that have been put into the test library again
> >  ranging from rather simple creation of a unique test temporary directory
> >  to a bit more complicated ones such as preparing, formatting, and
> >  mounting a block device.
> > 
> > -The test library also intializes shrared memory used for IPC at this
> > step.
> > +The test library also initializes shared memory used for IPC at this
> > step.
> > 
> >  Once all the prerequisites are checked and test environment has been
> >  prepared we can move on executing the testcase itself. The actual test
> >  is executed in a> 
> > @@ -62,12 +62,11 @@ for test_variants:
> >  		fork_testrun()
> >  
> >  ```
> > 
> > -Before we fork() the test process the test library sets up a timeout
> > alarm and -also a heartbeat signal handlers and also sets up an alarm(2)
> > accordingly to -the test timeout. When a test times out the test library
> > gets SIGALRM and the -alarm handler mercilessly kills all forked children
> > by sending SIGKILL to the -whole process group. The heartbeat handler is
> > used by the test process to reset -this timer for example when the test
> > functions run in a loop.
> > +Before we fork() the test process, the test library sets up a timeout
> > alarm and
> nit: "Before we fork " or "Before 'fork() '.
> 
> > +also a heartbeat signal handler. When a test times out, the test library
> > gets
> I'm not sure about this, isn't the original description correct? (1) timeout
> alarm() 2) a heartbeat signal handlers 3) second alarm() accordingly to the
> test timeout?
Sorry, I understood it incorrectly. I reversed it in the updated patch.

> 
> I mean there are 2 alarm() calls in tst_test.c.
> 
> Otherwise LGTM.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> Kind regards,
> Petr
> 
> > +SIGALRM and the alarm handler mercilessly kills all forked children by
> > sending +SIGKILL to the whole process group. The heartbeat handler is
> > used by the test +process to reset this timer for example when the test
> > functions run in a loop.> 
> >  With that done we finally fork() the test process. The test process
> >  firstly
> >  resets signal handlers and sets its pid to be a process group leader so
> >  that we> 
> > @@ -97,14 +96,14 @@ usually better option than exiting it in the middle.
> > 
> >  The test cleanup() is also called by the tst\_brk() handler in order to
> >  cleanup before exiting the test process, hence it must be able to cope
> >  even with partial test setup. Usually it suffices to make sure to clean
> >  up only> 
> > -resources that already have been set up and to do that in an inverse
> > order that -we did in setup().
> > +resources that already have been set up and to do that in the reverse
> > order +that we did in setup().
> > 
> >  Once the test process exits or leaves the run() or run\_all() function
> >  the test library wakes up from the waitpid() call, and checks if the
> >  test process exited normally.
> > 
> > -Once the testrun is finished the test library does a cleanup() as well to
> > clean +Once the testrun is finished, the test library does a cleanup() as
> > well to clean> 
> >  up resources set up in the test library setup(), reports test results and
> >  finally exits the process.
> > 
> > @@ -126,8 +125,8 @@ This especially means that:
> >  - While the test results are, by the design, propagated to the test
> >  library
> >  
> >    we may still miss a child that gets killed by a signal or exits
> >    unexpectedly.> 
> > -The test writer should, because of this, take care for reaping these
> > proceses -properly, in most cases this could be simply done by calling
> > +The test writer should, because of this, take care of reaping these
> > +processes properly, in most cases this could be simply done by calling
> > 
> >  tst\_reap\_children() to collect and dissect deceased.
> >  
> >  Also note that tst\_brk() does exit only the current process, so if a
> >  child
> > 
> > @@ -136,9 +135,9 @@ exits.
> > 
> >  ### Test library and exec()
> > 
> > -The piece of mapped memory to store the results to is not preserved over
> > +The piece of mapped memory to store the results is not preserved over
> > 
> >  exec(2), hence to use the test library from a binary started by an exec()
> >  it> 
> > -has to be remaped. In this case the process must to call tst\_reinit()
> > before +has to be remapped. In this case, the process must call
> > tst\_reinit() before> 
> >  calling any other library functions. In order to make this happen the
> >  program environment carries LTP\_IPC\_PATH variable with a path to the
> >  backing file on tmpfs. This also allows us to use the test library from
> >  shell testcases.> 
> > @@ -148,5 +147,5 @@ tmpfs. This also allows us to use the test library
> > from shell testcases.> 
> >  The piece of mapped memory is also used as a base for a futex-based
> >  synchronization primitives called checkpoints. And as said previously the
> >  memory can be mapped to any process by calling the tst\_reinit()
> >  function. As a> 
> > -matter of a fact there is even a tst\_checkpoint binary that allows us to
> > use +matter of a fact, there is even a tst\_checkpoint binary that allows
> > us to use> 
> >  the checkpoints from shell code as well.

--
Avinesh