[LTP] [PATCH v2] Correctly check setitimer params in setitimer01

[Andrea Cervesato]

Hi!

On 11/10/22 08:01, Li Wang wrote:
> Hi Andrea,
>
> On Mon, Nov 7, 2022 at 7:03 PM Andrea Cervesato via ltp 
> <ltp@lists.linux.it> wrote:
>
>     We use CLOCK_MONOTONIC_COARSE as our time resolution for checking
>     setitimer counter boundaries.
>
>     Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>     ---
>     Switching to CLOCK_MONOTONIC_COARSE for setitimer time resolution.
>
>      .../kernel/syscalls/setitimer/setitimer01.c   | 33
>     +++++++++++--------
>      1 file changed, 19 insertions(+), 14 deletions(-)
>
>     diff --git a/testcases/kernel/syscalls/setitimer/setitimer01.c
>     b/testcases/kernel/syscalls/setitimer/setitimer01.c
>     index eb62f02c6..5c880c6ef 100644
>     --- a/testcases/kernel/syscalls/setitimer/setitimer01.c
>     +++ b/testcases/kernel/syscalls/setitimer/setitimer01.c
>     @@ -8,7 +8,7 @@
>      /*\
>       * [Description]
>       *
>     - * Spawn a child and verify that setitimer() syscall passes, and
>     it ends up
>     + * Spawn a child, verify that setitimer() syscall passes and it
>     ends up
>       * counting inside expected boundaries. Then verify from the
>     parent that our
>       * syscall sent the correct signal to the child.
>       */
>     @@ -22,7 +22,8 @@
>      #include "tst_safe_clocks.h"
>
>      static struct itimerval *value, *ovalue;
>     -static unsigned long time_step;
>     +static long time_step;
>     +static long time_count;
>
>      static struct tcase {
>             int which;
>     @@ -56,7 +57,6 @@ static void verify_setitimer(unsigned int i)
>      {
>             pid_t pid;
>             int status;
>     -       int usec = 3 * time_step;
>             struct tcase *tc = &tcases[i];
>
>             pid = SAFE_FORK();
>     @@ -66,7 +66,7 @@ static void verify_setitimer(unsigned int i)
>
>                     tst_no_corefile(0);
>
>     -               set_setitimer_value(usec, 0);
>     +               set_setitimer_value(time_count, 0);
>                     TST_EXP_PASS(sys_setitimer(tc->which, value, NULL));
>
>                     set_setitimer_value(5 * time_step, 7 * time_step);
>
>
> Maybe we can use 'time_count' instead of 'time_step' as well.
>
This is needed if we want to check when setitimer syscall overrides 
value/ovalue on failure. We always expect time_count to be set in this case.

>     @@ -76,7 +76,7 @@ static void verify_setitimer(unsigned int i)
>                             ovalue->it_value.tv_sec,
>                             ovalue->it_value.tv_usec);
>
>     -               if (ovalue->it_value.tv_sec != 0 ||
>     ovalue->it_value.tv_usec > usec)
>     +               if (ovalue->it_value.tv_sec != 0 ||
>     ovalue->it_value.tv_usec > time_count + time_step)
>
>
> This is not correct for 'ITIMER_REAL', kernel does _not_
> add that one jiffy when using high resolution. I'd suggest
> taking reference to Martin's code in the last email thread.
>
> And, I also think we'd better insert code comments here to
> briefly declare why need to add the time_step for the result
> (of ITIMER_VIRTUAL/ITIMER_PROF) comparison, otherwise,
> people who are not familiar with this test will be confused.
Sure, I agree this this
>
>                             tst_res(TFAIL, "Ending counters are out of
>     range");
>
>                     for (;;)
>     @@ -93,24 +93,29 @@ static void verify_setitimer(unsigned int i)
>
>      static void setup(void)
>      {
>     -       struct timespec res;
>     +       struct timespec time_res;
>
>     -       SAFE_CLOCK_GETRES(CLOCK_MONOTONIC, &res);
>     +       SAFE_CLOCK_GETRES(CLOCK_MONOTONIC_COARSE, &time_res);
>
>
> And here require code comments on why we choose to
> use CLOCK_MONOTONIC_COARSE.
>
>
>     -       time_step = res.tv_nsec / 1000;
>     -       if (time_step < 10000)
>     -               time_step = 10000;
>     +       time_step = time_res.tv_nsec / 1000;
>     +       if (time_step <= 0)
>     +               time_step = 1000;
>
>     -       tst_res(TINFO, "clock resolution: %luns, time step: %luus",
>     -               res.tv_nsec,
>     -               time_step);
>     +       time_count = 3 * time_step;
>     +
>     +       tst_res(TINFO, "clock resolution: %luns, "
>     +               "time step: %luus, "
>     +               "time count: %luus",
>     +               time_res.tv_nsec,
>     +               time_step,
>     +               time_count);
>      }
>
>      static struct tst_test test = {
>             .tcnt = ARRAY_SIZE(tcases),
>             .forks_child = 1,
>     -       .test = verify_setitimer,
>             .setup = setup,
>     +       .test = verify_setitimer,
>             .bufs = (struct tst_buffers[]) {
>                     {&value,  .size = sizeof(struct itimerval)},
>                     {&ovalue, .size = sizeof(struct itimerval)},
>     -- 
>     2.35.3
>
>
>     -- 
>     Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
>
> -- 
> Regards,
> Li Wang

Andrea
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20221110/838bc78f/attachment-0001.htm>