lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

PATCH New Alexey Kodanev 2 participants 7 messages Nov 20, 2018 – Dec 4, 2018 Library code

Share & tools

Alexey Kodanev

author

2018-11-20 16:03 UTC #0 lore raw

PATCH 1/1 lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

With this patch, "safe" functions such as "ROD" and "tst_rhost_run -s" can be used in a test cleanup function, i.e. in case of an error, "safe" command won't recursively call the cleanup function again but will only print the warning message about the failure.

This behavior is consistent with the LTP C library.

Signed-off-by Alexey Kodanev <[email protected]>

---
 testcases/lib/tst_test.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

testcases/lib/tst_test.sh +6 -0

index 16081fa..e84cf7f 100644
@@ -41,6 +41,7 @@ trap "tst_brk TBROK 'test interrupted'" INT
 _tst_do_exit()
 {
 	local ret=0
+	TST_DO_EXIT=1
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
 	     -z "$TST_NO_CLEANUP" ]; then

@@ -123,6 +124,11 @@ tst_brk()
 	local res=$1
 	shift
 
+	if [ "$TST_DO_EXIT" = 1 ]; then
+		tst_res TWARN "$@"
+		return
+	fi
+
 	tst_res "$res" "$@"
 	_tst_do_exit
 }

Signature · 1 line

1.7.1

Petr Vorel

2018-11-27 09:04 UTC #1 lore raw

PATCH 1/1 lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

Hi Alexey,

Quoted 8 lines

With this patch, "safe" functions such as "ROD" and "tst_rhost_run -s" can be used in a test cleanup function, i.e. in case of an error, "safe" command won't recursively call the cleanup function again but will only print the warning message about the failure.
This behavior is consistent with the LTP C library.
Signed-off-by Alexey Kodanev <[email protected]>

Reviewed-by Petr Vorel <[email protected]>

Quoted 18 lines

---
 testcases/lib/tst_test.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

testcases/lib/tst_test.sh +1 -0

index 16081fa..e84cf7f 100644
@@ -41,6 +41,7 @@ trap "tst_brk TBROK 'test interrupted'" INT
 _tst_do_exit()
 {
 	local ret=0
+	TST_DO_EXIT=1
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
 	     -z "$TST_NO_CLEANUP" ]; then

@@ -123,6 +124,11 @@ tst_brk()
 	local res=$1

testcases/lib/tst_test.sh tst_brk() · line 134

Quoted 10 lines

testcases/lib/tst_test.sh +5 -0

 	shift
 
+	if [ "$TST_DO_EXIT" = 1 ]; then
+		tst_res TWARN "$@"
+		return
+	fi
+
 	tst_res "$res" "$@"
 	_tst_do_exit
 }

Shouldn't we return explicitly after any tst_brk_ call in tst_rhost_run()? Without that we get warning twice, when called from cleanup. test 2 TWARN: tst_rhost_run: command not defined
test 2 TWARN: tst_rhost_run: command not defined

This should fix it:

lib/tst_net.sh +7 -4

index d1206e285..385e3bf5c 100644
@@ -140,8 +140,11 @@ tst_rhost_run()
 	local user="root"
 	local cmd=
 	local safe=0
+	local ttype=TWARN
 	local bg=
 
+	[ "$safe" -eq 1 ] && ttype=TWARN
+
 	OPTIND=0
 
 	while getopts :bBsc:u: opt; do

@@ -161,9 +164,7 @@ tst_rhost_run()
 	OPTIND=0
 
 	if [ -z "$cmd" ]; then
-		[ "$safe" -eq 1 ] && \
-			tst_brk_ TBROK "tst_rhost_run: command not defined"
-		tst_res_ TWARN "tst_rhost_run: command not defined"
+		tst_brk_ $ttype "tst_rhost_run: command not defined"
 		return 1
 	fi

@@ -182,8 +183,10 @@ tst_rhost_run()
 	echo "$output" | grep -q 'RTERR$' && ret=1
 	if [ $ret -eq 1 ]; then
 		output=$(echo "$output" | sed 's/RTERR//')
-		[ "$safe" -eq 1 ] && \
+		if [ "$safe" -eq 1 ]; then
 			tst_brk_ TBROK "'$cmd' failed on '$RHOST': '$output'"
+			return 1
+		fi
 	fi
 
 	[ -z "$out" -a -n "$output" ] && echo "$output"

Other option is really run $TST_CLEANUP function only once, but that's inconsistent with C library :-(. Code would be simple:

lib/tst_test.sh +1 -0

index adfaea47e..beb1275ba 100644
@@ -44,6 +44,7 @@ _tst_do_exit()
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
 	     -z "$TST_NO_CLEANUP" ]; then
+		TST_NO_CLEANUP=1
 		$TST_CLEANUP
 	fi

And we should update doc.

Kind regards,
Petr

Alexey Kodanev

author

2018-11-27 13:42 UTC #2 lore raw

PATCH 1/1 lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

Hi Petr,

Quoted 52 lines · Petr Vorel

Hi Alexey,
Quoted 8 lines
With this patch, "safe" functions such as "ROD" and "tst_rhost_run -s" can be used in a test cleanup function, i.e. in case of an error, "safe" command won't recursively call the cleanup function again but will only print the warning message about the failure.
This behavior is consistent with the LTP C library.
Signed-off-by Alexey Kodanev <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
Quoted 18 lines
---
 testcases/lib/tst_test.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
testcases/lib/tst_test.sh +1 -0
index 16081fa..e84cf7f 100644
@@ -41,6 +41,7 @@ trap "tst_brk TBROK 'test interrupted'" INT
 _tst_do_exit()
 {
 	local ret=0
+	TST_DO_EXIT=1
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
 	     -z "$TST_NO_CLEANUP" ]; then
@@ -123,6 +124,11 @@ tst_brk()
 	local res=$1
testcases/lib/tst_test.sh tst_brk() · line 134
Quoted 10 lines
testcases/lib/tst_test.sh +5 -0
 	shift
 
+	if [ "$TST_DO_EXIT" = 1 ]; then
+		tst_res TWARN "$@"
+		return
+	fi
+
 	tst_res "$res" "$@"
 	_tst_do_exit
 }
Shouldn't we return explicitly after any tst_brk_ call in tst_rhost_run()? Without that we get warning twice, when called from cleanup. test 2 TWARN: tst_rhost_run: command not defined
test 2 TWARN: tst_rhost_run: command not defined
This should fix it:
lib/tst_net.sh +0 -0
index d1206e285..385e3bf5c 100644

lib/tst_net.sh tst_rhost_run() · line 147

Quoted 8 lines

lib/tst_net.sh +3 -0

@@ -140,8 +140,11 @@ tst_rhost_run()
 	local user="root"
 	local cmd=
 	local safe=0
+	local ttype=TWARN
 	local bg=
 
+	[ "$safe" -eq 1 ] && ttype=TWARN
+

ttype=TBROK?

lib/tst_net.sh tst_rhost_run() · line 168

Quoted 12 lines

lib/tst_net.sh +1 -3

 	OPTIND=0
 
 	while getopts :bBsc:u: opt; do

@@ -161,9 +164,7 @@ tst_rhost_run()
 	OPTIND=0
 
 	if [ -z "$cmd" ]; then
-		[ "$safe" -eq 1 ] && \
-			tst_brk_ TBROK "tst_rhost_run: command not defined"
-		tst_res_ TWARN "tst_rhost_run: command not defined"
+		tst_brk_ $ttype "tst_rhost_run: command not defined"
 		return 1

I think we should only remove tst_res_ TWARN here. Otherwise it will exit the test for non-safe option too.

lib/tst_net.sh tst_rhost_run() · line 189

Quoted 11 lines

lib/tst_net.sh +3 -1

fi

@@ -182,8 +183,10 @@ tst_rhost_run()
 	echo "$output" | grep -q 'RTERR$' && ret=1
 	if [ $ret -eq 1 ]; then
 		output=$(echo "$output" | sed 's/RTERR//')
-		[ "$safe" -eq 1 ] && \
+		if [ "$safe" -eq 1 ]; then
 			tst_brk_ TBROK "'$cmd' failed on '$RHOST': '$output'"
+			return 1
+		fi

It looks as if someone forgot that tst_brk_ terminates the test :)

Quoted 22 lines

lib/tst_net.sh +0 -0
 	fi
 
 	[ -z "$out" -a -n "$output" ] && echo "$output"
Other option is really run $TST_CLEANUP function only once, but that's inconsistent with C library :-(. Code would be simple:
lib/tst_test.sh +1 -0
index adfaea47e..beb1275ba 100644
@@ -44,6 +44,7 @@ _tst_do_exit()
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
 	     -z "$TST_NO_CLEANUP" ]; then
+		TST_NO_CLEANUP=1
 		$TST_CLEANUP
 	fi
 

Quoted 2 lines

And we should update doc.

OK, we should add similar "WARNING" from 2.2 section to 2.3, right?

"WARNING: Calling tst_brk() in test 'cleanup()' does not exit the test as well and 'TBROK' is converted to 'TWARN'."

Quoted 3 lines

Kind regards,
Petr

Petr Vorel

2018-11-27 15:01 UTC #3 lore raw

PATCH 1/1 lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

Hi Alexey,

Quoted 4 lines

lib/tst_net.sh tst_rhost_run() · line 147
Quoted 2 lines
lib/tst_net.sh +2 -0
+	[ "$safe" -eq 1 ] && ttype=TWARN
+
ttype=TBROK?

Correct, I'm sorry.

Quoted 9 lines

lib/tst_net.sh tst_rhost_run() · line 168
Quoted 6 lines
lib/tst_net.sh +1 -3
 	if [ -z "$cmd" ]; then
-		[ "$safe" -eq 1 ] && \
-			tst_brk_ TBROK "tst_rhost_run: command not defined"
-		tst_res_ TWARN "tst_rhost_run: command not defined"
+		tst_brk_ $ttype "tst_rhost_run: command not defined"
 		return 1
I think we should only remove tst_res_ TWARN here. Otherwise it will exit the test for non-safe option too.

I see, I wrongly I use tst_brk_ on non safe as well.

...

Quoted 8 lines

lib/tst_net.sh tst_rhost_run() · line 189
Quoted 5 lines
lib/tst_net.sh +3 -1
-		[ "$safe" -eq 1 ] && \
+		if [ "$safe" -eq 1 ]; then
 			tst_brk_ TBROK "'$cmd' failed on '$RHOST': '$output'"
+			return 1
+		fi
It looks as if someone forgot that tst_brk_ terminates the test :)

And here I got confused by _tst_do_exit :)

I'd keep TWARN, so correct part to your commit could be something like patch bellow.

Kind regards,
Petr

@@ -161,9 +161,11 @@ tst_rhost_run()
OPTIND=0

if [ -z "$cmd" ]; then
- [ "$safe" -eq 1 ] && \
+ if [ "$safe" -eq 1 ]; then
tst_brk_ TBROK "tst_rhost_run: command not defined"
- tst_res_ TWARN "tst_rhost_run: command not defined"
+ else
+ tst_res_ TWARN "tst_rhost_run: command not defined"
+ fi
return 1
fi

Petr Vorel

2018-11-27 15:09 UTC #4 lore raw

PATCH 1/1 lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk()

In reply to Alexey Kodanev

Hi Alexey,

Quoted 4 lines

OK, we should add similar "WARNING" from 2.2 section to 2.3, right?
"WARNING: Calling tst_brk() in test 'cleanup()' does not exit the test as well and 'TBROK' is converted to 'TWARN'."

Yes, that'd be enough. Thanks for handling it.

Kind regards,
Petr

Petr Vorel

2018-11-30 18:18 UTC #5 lore raw

[LTP] WIKI updates via git

Hi Alexey,

Quoted 6 lines

Quoted 4 lines
OK, we should add similar "WARNING" from 2.2 section to 2.3, right?
"WARNING: Calling tst_brk() in test 'cleanup()' does not exit the test as well and 'TBROK' is converted to 'TWARN'."
Yes, that'd be enough. Thanks for handling it.

Noticed you pushed the changes, but didn't update wiki. I did it.

BTW: There is a git repository, which makes it easier to update it:
https://github.com/linux-test-project/ltp.wiki.git
(simple git commit is equivalent of change via web)

Kind regards,
Petr

Alexey Kodanev

author

2018-12-04 11:47 UTC #6 lore raw

[LTP] WIKI updates via git

Hi Petr,

Quoted 7 lines · Petr Vorel

Hi Alexey,
Quoted 4 lines
Quoted 4 lines
OK, we should add similar "WARNING" from 2.2 section to 2.3, right?
"WARNING: Calling tst_brk() in test 'cleanup()' does not exit the test as well and 'TBROK' is converted to 'TWARN'."

Quoted 8 lines

Quoted 1 line
Yes, that'd be enough. Thanks for handling it.
Noticed you pushed the changes, but didn't update wiki. I did it.
BTW: There is a git repository, which makes it easier to update it:
https://github.com/linux-test-project/ltp.wiki.git
(simple git commit is equivalent of change via web)

Got it, thanks!