[LTP] [PATCH 1/2] tst_test: discard the stderr output

[LTP] [PATCH 1/2] tst_test: discard the stderr output

[Li Wang]

Signed-off-by: Li Wang <liwang@redhat.com>
---

Notes:
    We can merge this before the new release.

 testcases/lib/tst_test.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 28c2052d6..3a5651c01 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -444,13 +444,13 @@ _tst_kill_test()
 	kill -INT -$pid
 	tst_sleep 100ms
 
-	while kill -0 $pid 2>&1 > /dev/null && [ $i -gt 0 ]; do
+	while kill -0 $pid >/dev/null 2>&1 && [ $i -gt 0 ]; do
 		tst_res TINFO "Test is still running, waiting ${i}s"
 		sleep 1
 		i=$((i-1))
 	done
 
-	if kill -0 $pid 2>&1 > /dev/null; then
+	if kill -0 $pid >/dev/null 2>&1; then
 		tst_res TBROK "Test still running, sending SIGKILL"
 		kill -KILL -$pid
 	fi
-- 
2.31.1

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Li Wang]

We'd better avoid using SIGINT for process terminating becuasue,
it has different behavoir on kind of shell.

From Joerg Vehlow's test:

 - bash does not seem to care about SIGINT delivery to background
   processes, but can be blocked using trap

 - zsh ignores SIGINT for background processes by default, but can be
   allowed using trap

 - dash and busybox sh ignore the signal to background processes, and
   this cannot be changed with trap

This patch cover the below situations:

 1. SIGINT (Ctrl^C) for terminating the main process and do cleanup
    correctly before a timeout

 2. Test finish normally and retrieves the _tst_timeout_process in the
    background via SIGTERM(sending by _tst_cleanup_timer)

 3. Test timed out occurs and _tst_kill_test sending SIGTERM to
    terminating all process, and the main process do cleanup work

 4. Test timed out occurs but still have process alive after _tst_kill_test
    sending SIGTERM, then sending SIGKILL to the whole group

 5. Test terminated by SIGTERM unexpectly (e.g. system shutdown or process
    manager) and do cleanup work as well

Co-authored-by: Joerg Vehlow <lkml@jv-coder.de>
Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/newlib_tests/shell/test_timeout.sh | 2 +-
 lib/newlib_tests/shell/timeout03.sh    | 1 +
 testcases/lib/tst_test.sh              | 9 +++++----
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
index b05680cb1..9f31afa32 100755
--- a/lib/newlib_tests/shell/test_timeout.sh
+++ b/lib/newlib_tests/shell/test_timeout.sh
@@ -28,7 +28,7 @@ timeout02.sh|  -10|0|  |2
 timeout02.sh| -0.1|0|  |0
 timeout02.sh| -1.1|0|  |2
 timeout02.sh|-10.1|0|  |2
-timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGINT
+timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGTERM
 timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
 timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
 "
diff --git a/lib/newlib_tests/shell/timeout03.sh b/lib/newlib_tests/shell/timeout03.sh
index cd548d9a2..124e96a84 100755
--- a/lib/newlib_tests/shell/timeout03.sh
+++ b/lib/newlib_tests/shell/timeout03.sh
@@ -30,6 +30,7 @@ TST_TIMEOUT=1
 
 do_test()
 {
+	trap "tst_res TINFO 'Sorry, timeout03 is still alive'" TERM
 	tst_res TINFO "testing killing test after TST_TIMEOUT"
 
 	sleep 2
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 3a5651c01..66ffde4eb 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -21,7 +21,8 @@ export TST_LIB_LOADED=1
 . tst_security.sh
 
 # default trap function
-trap "tst_brk TBROK 'test interrupted or timed out'" INT
+trap "tst_brk TBROK 'test interrupted'" INT
+trap "unset _tst_setup_timer_pid; tst_brk TBROK 'test terminated'" TERM
 
 _tst_do_exit()
 {
@@ -439,9 +440,9 @@ _tst_kill_test()
 {
 	local i=10
 
-	trap '' INT
-	tst_res TBROK "Test timeouted, sending SIGINT! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
-	kill -INT -$pid
+	trap '' TERM
+	tst_res TBROK "Test timed out, sending SIGTERM! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
+	kill -TERM -$pid
 	tst_sleep 100ms
 
 	while kill -0 $pid >/dev/null 2>&1 && [ $i -gt 0 ]; do
-- 
2.31.1

[LTP] [PATCH 1/2] tst_test: discard the stderr output

[Cyril Hrubis]

Hi!
>     We can merge this before the new release.

Looks safe enough for the release.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] tst_test: discard the stderr output

[Petr Vorel]

Hi Li,

thanks for the fix, merged!

Kind regards,
Petr

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Joerg Vehlow]

Hi Li,


On 5/18/2021 2:26 PM, Li Wang wrote:

> Co-authored-by: Joerg Vehlow <lkml@jv-coder.de>
Please use Joerg Vehlow <joerg.vehlow@aox-tech.de> here, thanks.
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>   lib/newlib_tests/shell/test_timeout.sh | 2 +-
>   lib/newlib_tests/shell/timeout03.sh    | 1 +
>   testcases/lib/tst_test.sh              | 9 +++++----
>   3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
> index b05680cb1..9f31afa32 100755
> --- a/lib/newlib_tests/shell/test_timeout.sh
> +++ b/lib/newlib_tests/shell/test_timeout.sh
> @@ -28,7 +28,7 @@ timeout02.sh|  -10|0|  |2
>   timeout02.sh| -0.1|0|  |0
>   timeout02.sh| -1.1|0|  |2
>   timeout02.sh|-10.1|0|  |2
> -timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGINT
> +timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGTERM
Whoa what did I write there, that's hard to read :)
While you're at it, can you please change this to "Test is killed, if it 
does not terminate after SIGTERM"
>   timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
>   timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
>   "
> diff --git a/lib/newlib_tests/shell/timeout03.sh b/lib/newlib_tests/shell/timeout03.sh
At the top of this file, there is a comment with the expected output, 
that should be corrected
> index cd548d9a2..124e96a84 100755
> --- a/lib/newlib_tests/shell/timeout03.sh
> +++ b/lib/newlib_tests/shell/timeout03.sh
> @@ -30,6 +30,7 @@ TST_TIMEOUT=1
>   
>   do_test()
>   {
> +	trap "tst_res TINFO 'Sorry, timeout03 is still alive'" TERM
We don't need this.
>   	tst_res TINFO "testing killing test after TST_TIMEOUT"
>   
>   	sleep 2
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 3a5651c01..66ffde4eb 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -21,7 +21,8 @@ export TST_LIB_LOADED=1
>   . tst_security.sh
>   
>   # default trap function
> -trap "tst_brk TBROK 'test interrupted or timed out'" INT
> +trap "tst_brk TBROK 'test interrupted'" INT
> +trap "unset _tst_setup_timer_pid; tst_brk TBROK 'test terminated'" TERM
>   
>   _tst_do_exit()
>   {
> @@ -439,9 +440,9 @@ _tst_kill_test()
>   {
>   	local i=10
>   
> -	trap '' INT
> -	tst_res TBROK "Test timeouted, sending SIGINT! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
> -	kill -INT -$pid
> +	trap '' TERM
> +	tst_res TBROK "Test timed out, sending SIGTERM! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1"
> +	kill -TERM -$pid
This works fine.
+Reviewed-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
after my remarks are resolved.

But one more strange thing here.
I wonder why this even works. $pid is used in _tst_kill_test and defined 
in _tst_setup_timer as a local variable.
It looks like it is inherited through the call chain and since it is 
copied to the background process, it cannot be manipulated by the tests.
Still I would vote for changing this at some point, because it is highly 
confusing.

J?rg

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Li Wang]

Hi Joerg,

> This works fine.
> +Reviewed-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
> after my remarks are resolved.

Thanks for the above comments, I will fix them all in patch V2.

> But one more strange thing here.
> I wonder why this even works. $pid is used in _tst_kill_test and defined
> in _tst_setup_timer as a local variable.
> It looks like it is inherited through the call chain and since it is
> copied to the background process, it cannot be manipulated by the tests.
> Still I would vote for changing this at some point, because it is highly
> confusing.

+1

Agree, I look at it a while and seems a local variable can only be
visible in the sub-function/call-chain of SHELL.

But I vote to fix the problem in a separate patch, it'd be appreciated
if you help make the patch.

-- 
Regards,
Li Wang

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Cyril Hrubis]

Hi!
> But one more strange thing here.
> I wonder why this even works. $pid is used in _tst_kill_test and defined 
> in _tst_setup_timer as a local variable.
> It looks like it is inherited through the call chain and since it is 
> copied to the background process, it cannot be manipulated by the tests.
> Still I would vote for changing this at some point, because it is highly 
> confusing.

That's actually a correct and well defined behavior, if you call a
function g from function f the function g has access to the variables
local to f.

And yes it's confusing, but the alternative is having another global
variable which I do not think is much better than this.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Cyril Hrubis]

Hi!
>  lib/newlib_tests/shell/test_timeout.sh | 2 +-
>  lib/newlib_tests/shell/timeout03.sh    | 1 +
>  testcases/lib/tst_test.sh              | 9 +++++----
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
> index b05680cb1..9f31afa32 100755
> --- a/lib/newlib_tests/shell/test_timeout.sh
> +++ b/lib/newlib_tests/shell/test_timeout.sh
> @@ -28,7 +28,7 @@ timeout02.sh|  -10|0|  |2
>  timeout02.sh| -0.1|0|  |0
>  timeout02.sh| -1.1|0|  |2
>  timeout02.sh|-10.1|0|  |2
> -timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGINT
> +timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGTERM
>  timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
>  timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
>  "
> diff --git a/lib/newlib_tests/shell/timeout03.sh b/lib/newlib_tests/shell/timeout03.sh
> index cd548d9a2..124e96a84 100755
> --- a/lib/newlib_tests/shell/timeout03.sh
> +++ b/lib/newlib_tests/shell/timeout03.sh
> @@ -30,6 +30,7 @@ TST_TIMEOUT=1
>  
>  do_test()
>  {
> +	trap "tst_res TINFO 'Sorry, timeout03 is still alive'" TERM
>  	tst_res TINFO "testing killing test after TST_TIMEOUT"
>  
>  	sleep 2
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 3a5651c01..66ffde4eb 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -21,7 +21,8 @@ export TST_LIB_LOADED=1
>  . tst_security.sh
>  
>  # default trap function
> -trap "tst_brk TBROK 'test interrupted or timed out'" INT
> +trap "tst_brk TBROK 'test interrupted'" INT
> +trap "unset _tst_setup_timer_pid; tst_brk TBROK 'test terminated'" TERM

I've been looking at this for a while and I think that we should unset
the _tst_setup_timer_pid at the end of the _tst_timeout_process()
instead, right?

Otherwise we will leave the timeout process sleeping if someone sends
SIGTERM to the test process from the outside, or do I miss something?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Cyril Hrubis]

Hi!
> >> But one more strange thing here.
> >> I wonder why this even works. $pid is used in _tst_kill_test and defined
> >> in _tst_setup_timer as a local variable.
> >> It looks like it is inherited through the call chain and since it is
> >> copied to the background process, it cannot be manipulated by the tests.
> >> Still I would vote for changing this at some point, because it is highly
> >> confusing.
> > That's actually a correct and well defined behavior, if you call a
> > function g from function f the function g has access to the variables
> > local to f.
> >
> > And yes it's confusing, but the alternative is having another global
> > variable which I do not think is much better than this.
> Hm shell code has more strange behavior than I would have ever expected...

Strongly agree here, to be honest if there was as better scripting
language that would be widely avaiable I would have switched long time
ago...

> But at least Li and myself did not know that and even while you know 
> about this "feature", you think it is strange.
> I would rather like to be explicit and use a global variable (what harm 
> is it really?) instead of confusing the next one looking at this piece 
> of code....

If you really think global variable would be better, then go ahead and
send a patch, I do not have a strong feelings about this particular
detail.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Cyril Hrubis]

Hi!
> >>   # default trap function
> >> -trap "tst_brk TBROK 'test interrupted or timed out'" INT
> >> +trap "tst_brk TBROK 'test interrupted'" INT
> >> +trap "unset _tst_setup_timer_pid; tst_brk TBROK 'test terminated'" TERM
> > I've been looking at this for a while and I think that we should unset
> > the _tst_setup_timer_pid at the end of the _tst_timeout_process()
> > instead, right?
> That won't work or am I missing something? ;) We are in a subshell in 
> _tst_timeout_process, it cannot manipulate the environment of the parent 
> or is my mental modal of how shell works wrong again?.

Nah, you are right this time, I missed that it runs on a background.

> > Otherwise we will leave the timeout process sleeping if someone sends
> > SIGTERM to the test process from the outside, or do I miss something?
> Yes, that is one of the reasons why I initially suggested a different 
> signal for timeout. That would cleanly separate the logic.
> But on the other hand when will SIGTERM be sent from the outside? This 
> is probably only happening on shutdown? In that case it is not really a 
> problem.
> If someone wants to terminate gracefully using SIGTERM, he could send it 
> to the process group. That would kill the timeout process as well.

Okay. Let's go with this version, it's simple enough and anything that
would behave better would only overcomplicate the code again.

For the patch:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Joerg Vehlow]

Hi Cyril,

On 5/31/2021 2:35 PM, Cyril Hrubis wrote:
> Hi!
>> But one more strange thing here.
>> I wonder why this even works. $pid is used in _tst_kill_test and defined
>> in _tst_setup_timer as a local variable.
>> It looks like it is inherited through the call chain and since it is
>> copied to the background process, it cannot be manipulated by the tests.
>> Still I would vote for changing this at some point, because it is highly
>> confusing.
> That's actually a correct and well defined behavior, if you call a
> function g from function f the function g has access to the variables
> local to f.
>
> And yes it's confusing, but the alternative is having another global
> variable which I do not think is much better than this.
Hm shell code has more strange behavior than I would have ever expected...
But at least Li and myself did not know that and even while you know 
about this "feature", you think it is strange.
I would rather like to be explicit and use a global variable (what harm 
is it really?) instead of confusing the next one looking at this piece 
of code....

J?rg

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Joerg Vehlow]

Hi,

On 5/31/2021 2:49 PM, Cyril Hrubis wrote:
> Hi!
>>   lib/newlib_tests/shell/test_timeout.sh | 2 +-
>>   lib/newlib_tests/shell/timeout03.sh    | 1 +
>>   testcases/lib/tst_test.sh              | 9 +++++----
>>   3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
>> index b05680cb1..9f31afa32 100755
>> --- a/lib/newlib_tests/shell/test_timeout.sh
>> +++ b/lib/newlib_tests/shell/test_timeout.sh
>> @@ -28,7 +28,7 @@ timeout02.sh|  -10|0|  |2
>>   timeout02.sh| -0.1|0|  |0
>>   timeout02.sh| -1.1|0|  |2
>>   timeout02.sh|-10.1|0|  |2
>> -timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGINT
>> +timeout03.sh|     |0|12|137| | | |Test kill if test does not terminate by SIGTERM
>>   timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
>>   timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
>>   "
>> diff --git a/lib/newlib_tests/shell/timeout03.sh b/lib/newlib_tests/shell/timeout03.sh
>> index cd548d9a2..124e96a84 100755
>> --- a/lib/newlib_tests/shell/timeout03.sh
>> +++ b/lib/newlib_tests/shell/timeout03.sh
>> @@ -30,6 +30,7 @@ TST_TIMEOUT=1
>>   
>>   do_test()
>>   {
>> +	trap "tst_res TINFO 'Sorry, timeout03 is still alive'" TERM
>>   	tst_res TINFO "testing killing test after TST_TIMEOUT"
>>   
>>   	sleep 2
>> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
>> index 3a5651c01..66ffde4eb 100644
>> --- a/testcases/lib/tst_test.sh
>> +++ b/testcases/lib/tst_test.sh
>> @@ -21,7 +21,8 @@ export TST_LIB_LOADED=1
>>   . tst_security.sh
>>   
>>   # default trap function
>> -trap "tst_brk TBROK 'test interrupted or timed out'" INT
>> +trap "tst_brk TBROK 'test interrupted'" INT
>> +trap "unset _tst_setup_timer_pid; tst_brk TBROK 'test terminated'" TERM
> I've been looking at this for a while and I think that we should unset
> the _tst_setup_timer_pid at the end of the _tst_timeout_process()
> instead, right?
That won't work or am I missing something? ;) We are in a subshell in 
_tst_timeout_process, it cannot manipulate the environment of the parent 
or is my mental modal of how shell works wrong again?.
> Otherwise we will leave the timeout process sleeping if someone sends
> SIGTERM to the test process from the outside, or do I miss something?
Yes, that is one of the reasons why I initially suggested a different 
signal for timeout. That would cleanly separate the logic.
But on the other hand when will SIGTERM be sent from the outside? This 
is probably only happening on shutdown? In that case it is not really a 
problem.
If someone wants to terminate gracefully using SIGTERM, he could send it 
to the process group. That would kill the timeout process as well.

J?rg

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Li Wang]

> Okay. Let's go with this version, it's simple enough and anything that
> would behave better would only overcomplicate the code again.
>
> For the patch:
>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Thanks for the review, I helped merge the patch V2.
(tiny updates in v2: 'add comments + remove trap' from timeout03)

-- 
Regards,
Li Wang

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Petr Vorel]

Hi Li, all,

> > Okay. Let's go with this version, it's simple enough and anything that
> > would behave better would only overcomplicate the code again.

> > For the patch:

> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> Thanks for the review, I helped merge the patch V2.
> (tiny updates in v2: 'add comments + remove trap' from timeout03)

Thanks a lot for finishing this, sorry for late reply.

Kind regards,
Petr

[LTP] [PATCH 2/2] tst_test: using SIGTERM to terminate process

[Joerg Vehlow]

Hi Cyril,

On 5/31/2021 2:54 PM, Cyril Hrubis wrote:
>>>> But one more strange thing here.
>>>> I wonder why this even works. $pid is used in _tst_kill_test and defined
>>>> in _tst_setup_timer as a local variable.
>>>> It looks like it is inherited through the call chain and since it is
>>>> copied to the background process, it cannot be manipulated by the tests.
>>>> Still I would vote for changing this at some point, because it is highly
>>>> confusing.
>>> That's actually a correct and well defined behavior, if you call a
>>> function g from function f the function g has access to the variables
>>> local to f.
>>>
>>> And yes it's confusing, but the alternative is having another global
>>> variable which I do not think is much better than this.
>> Hm shell code has more strange behavior than I would have ever expected...
> Strongly agree here, to be honest if there was as better scripting
> language that would be widely avaiable I would have switched long time
> ago...
>
>> But at least Li and myself did not know that and even while you know
>> about this "feature", you think it is strange.
>> I would rather like to be explicit and use a global variable (what harm
>> is it really?) instead of confusing the next one looking at this piece
>> of code....
> If you really think global variable would be better, then go ahead and
> send a patch, I do not have a strong feelings about this particular
> detail.
I think there is another solution, to make this at least more explicit 
without using a new global variable.
Passing the pid as a parameter to _tst_timeout_process and _tst_kill_test.
Although it is not required, it makes it a lot less implicit and a lot 
better simpler to read.

J?rg