* [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() @ 2018-11-20 16:03 Alexey Kodanev 2018-11-27 9:04 ` Petr Vorel 0 siblings, 1 reply; 7+ messages in thread From: Alexey Kodanev @ 2018-11-20 16:03 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() 2018-11-20 16:03 [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() Alexey Kodanev @ 2018-11-27 9:04 ` Petr Vorel 2018-11-27 13:42 ` Alexey Kodanev 0 siblings, 1 reply; 7+ messages in thread From: Petr Vorel @ 2018-11-27 9:04 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() 2018-11-27 9:04 ` Petr Vorel @ 2018-11-27 13:42 ` Alexey Kodanev 2018-11-27 15:01 ` Petr Vorel 2018-11-27 15:09 ` Petr Vorel 0 siblings, 2 replies; 7+ messages in thread From: Alexey Kodanev @ 2018-11-27 13:42 UTC (permalink / raw) To: ltp 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() 2018-11-27 13:42 ` Alexey Kodanev @ 2018-11-27 15:01 ` Petr Vorel 2018-11-27 15:09 ` Petr Vorel 1 sibling, 0 replies; 7+ messages in thread From: Petr Vorel @ 2018-11-27 15:01 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() 2018-11-27 13:42 ` Alexey Kodanev 2018-11-27 15:01 ` Petr Vorel @ 2018-11-27 15:09 ` Petr Vorel 2018-11-30 18:18 ` [LTP] WIKI updates via git Petr Vorel 1 sibling, 1 reply; 7+ messages in thread From: Petr Vorel @ 2018-11-27 15:09 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] WIKI updates via git 2018-11-27 15:09 ` Petr Vorel @ 2018-11-30 18:18 ` Petr Vorel 2018-12-04 11:47 ` Alexey Kodanev 0 siblings, 1 reply; 7+ messages in thread From: Petr Vorel @ 2018-11-30 18:18 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [LTP] WIKI updates via git 2018-11-30 18:18 ` [LTP] WIKI updates via git Petr Vorel @ 2018-12-04 11:47 ` Alexey Kodanev 0 siblings, 0 replies; 7+ messages in thread From: Alexey Kodanev @ 2018-12-04 11:47 UTC (permalink / raw) To: ltp 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-04 11:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-20 16:03 [LTP] [PATCH] lib/tst_test.sh: don't call _tst_do_exit() recursively with tst_brk() Alexey Kodanev 2018-11-27 9:04 ` Petr Vorel 2018-11-27 13:42 ` Alexey Kodanev 2018-11-27 15:01 ` Petr Vorel 2018-11-27 15:09 ` Petr Vorel 2018-11-30 18:18 ` [LTP] WIKI updates via git Petr Vorel 2018-12-04 11:47 ` Alexey Kodanev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox