mbox series

[v3,0/9] mmapstress01: refactor to ltp framework

Message ID 20221004182040.1859774-1-edliaw@google.com
Headers show
Series mmapstress01: refactor to ltp framework | expand

Message

Edward Liaw Oct. 4, 2022, 6:20 p.m. UTC 
Attempt to refactor mmapstress01 to use the ltp framework.

v2->v3:
* apply make check lint suggestions
* refactor cleanup
* use SAFE_FORK
* update license

v1->v2:
* clean up formatting
* remove accidental change to header comment
* use SAFE_MMAP

Edward Liaw (9):
  mmapstress01: refactor to tst_test framework
  mmapstress01: apply make check suggestions
  mmapstress01: refactor options
  mmapstress01: use FILE_OFFSET_BITS=64
  mmapstress01: use safe macros
  mmapstress01: refactor cleanup and drop leavefile option
  mmapstress01: use SAFE_FORK
  mmapstress01: update license
  mmapstress01: reorder vars and functions

 testcases/kernel/mem/mmapstress/Makefile      |   2 +
 .../kernel/mem/mmapstress/mmapstress01.c      | 845 ++++++------------
 2 files changed, 289 insertions(+), 558 deletions(-)

Comments

Cyril Hrubis Oct. 5, 2022, 10:31 a.m. UTC | #1 
Hi!
The code is heading into the right direction but there are couple of
things to do (can be done in an incremental manner too):

* Most of the remaning tst_brk(TFAIL, "..") should actually be
  tst_brk(TBROK, "..") at least all the cases where we call
  a syscall and it fails. And we should include the TERRNO flag
  as well to get the actual error printed.

* The test should make use of runtime instead of the alarm()
  for test duration. That would mean getting rid of the -t option and
  using the -I option instead. Also the test should set up some
  .test_runtime for a default run duration in the tst_test structure.

* The top level comment has to be updated for the changes in the test
  since the options structure describes the command line parameters
  quite well I would just remove that part from the comment
Petr Vorel Oct. 5, 2022, 11:22 a.m. UTC | #2 
Hi all,

> Hi!
> The code is heading into the right direction but there are couple of
> things to do (can be done in an incremental manner too):

> * Most of the remaning tst_brk(TFAIL, "..") should actually be
>   tst_brk(TBROK, "..") at least all the cases where we call
>   a syscall and it fails. And we should include the TERRNO flag
>   as well to get the actual error printed.

> * The test should make use of runtime instead of the alarm()
>   for test duration. That would mean getting rid of the -t option and
>   using the -I option instead. Also the test should set up some
>   .test_runtime for a default run duration in the tst_test structure.

> * The top level comment has to be updated for the changes in the test
>   since the options structure describes the command line parameters
>   quite well I would just remove that part from the comment

I'd also squash at least same changes if not all (update licence, reorder vars,
make check fixes, ...  IMHO does not need to be in a separate commit).

While in it, it'd be worth the test had default parameters. i.e. -p X not having
to pass, and use the default 20. The same applies for -I (which should replaced -t).

Kind regards,
Petr