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

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

[Alexey Kodanev]

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 <alexey.kodanev@oracle.com>
---
 testcases/lib/tst_test.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 16081fa..e84cf7f 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -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
 }
-- 
1.7.1

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

[Petr Vorel]

Hi Alexey,

> 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 <alexey.kodanev@oracle.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/lib/tst_test.sh |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 16081fa..e84cf7f 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -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
>  }

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:

diff --git testcases/lib/tst_net.sh testcases/lib/tst_net.sh
index d1206e285..385e3bf5c 100644
--- testcases/lib/tst_net.sh
+++ testcases/lib/tst_net.sh
@@ -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:

diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
index adfaea47e..beb1275ba 100644
--- testcases/lib/tst_test.sh
+++ testcases/lib/tst_test.sh
@@ -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

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

[Alexey Kodanev]

Hi Petr,
On 11/27/2018 12:04 PM, Petr Vorel wrote:
> Hi Alexey,
> 
>> 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 <alexey.kodanev@oracle.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> ---
>>  testcases/lib/tst_test.sh |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
>> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
>> index 16081fa..e84cf7f 100644
>> --- a/testcases/lib/tst_test.sh
>> +++ b/testcases/lib/tst_test.sh
>> @@ -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
>>  }
> 
> 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:
> 
> diff --git testcases/lib/tst_net.sh testcases/lib/tst_net.sh
> index d1206e285..385e3bf5c 100644
> --- testcases/lib/tst_net.sh
> +++ testcases/lib/tst_net.sh
> @@ -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?

>  	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.



>  	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 :)


>  	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:
> 
> diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
> index adfaea47e..beb1275ba 100644
> --- testcases/lib/tst_test.sh
> +++ testcases/lib/tst_test.sh
> @@ -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.
> 

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'."

> Kind regards,
> Petr
> 

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

[Petr Vorel]

Hi Alexey,

> > +	[ "$safe" -eq 1 ] && ttype=TWARN
> > +

> ttype=TBROK?
Correct, I'm sorry.

> >  	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.

...
> > -		[ "$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

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

[Petr Vorel]

Hi Alexey,

> 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

[LTP] WIKI updates via git

[Petr Vorel]

Hi Alexey,

> > 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

[LTP] WIKI updates via git

[Alexey Kodanev]

Hi Petr,
On 11/30/2018 09:18 PM, Petr Vorel wrote:
> Hi Alexey,
> 
>>> 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)
> 

Got it, thanks!