| Message ID | 20191016161519.11256-1-cfamullaconrad@suse.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v3,1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() | expand |
Hi Clemens, ... > -_tst_setup_timer() > +tst_multiply_timeout() Private function, it should have underscore prefix. > { > - TST_TIMEOUT=${TST_TIMEOUT:-300} > - LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > + # first parameter is used as return value > + local timeout="${!1}" Bashism, this will not work on dash, busybox shell ('busybox sh'), etc. checkbashisms.pl is your friend :). https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style Variable variable is possible to do portable way with eval. eval timeout=\$$1 > + [ $# -gt 1 ] && timeout="$2" > - if [ "$TST_TIMEOUT" = -1 ]; then > - tst_res TINFO "Timeout per run is disabled" > - return > - fi > + LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > local err="LTP_TIMEOUT_MUL must be number >= 1!" > @@ -396,13 +395,29 @@ _tst_setup_timer() > LTP_TIMEOUT_MUL=$((LTP_TIMEOUT_MUL+1)) > tst_res TINFO "ceiling LTP_TIMEOUT_MUL to $LTP_TIMEOUT_MUL" > fi > + > [ "$LTP_TIMEOUT_MUL" -ge 1 ] || tst_brk TBROK "$err ($LTP_TIMEOUT_MUL)" > + [ "$timeout" -ge 1 ] || tst_brk TBROK "timeout need to be >= 1 ($timeout)" > + > + eval "$1='$(( timeout * LTP_TIMEOUT_MUL))'" Eval on input, eval on output :). > + return 0 You don't use return value anywhere. + There is no return 1. > +} Passing timeout variable name and optionally timeout value works and allows TBROK messages not to be mangled/hidden (which would be if function echo the result, which is then read the usual way: timeout=$(tst_multiply_timeout 100) ), but I'm not sure if all this is worth of just error handling. Having 2x eval, $2 optionally used (but only in tests) makes code a bit complex. How about just simply save the result into global variable $TST_TIMEOUT? Kind regards, Petr
On Thu, 2019-10-17 at 10:39 +0200, Petr Vorel wrote: > Hi Clemens, > > ... > > -_tst_setup_timer() > > +tst_multiply_timeout() > > Private function, it should have underscore prefix. > > { > > - TST_TIMEOUT=${TST_TIMEOUT:-300} > > - LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > > + # first parameter is used as return value > > + local timeout="${!1}" > > Bashism, this will not work on dash, busybox shell ('busybox sh'), > etc. > > checkbashisms.pl is your friend :). > https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkba > shisms.pl > https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guideline > s#132-shell-coding-style > > Variable variable is possible to do portable way with eval. > eval timeout=\$$1 Ok, I will take bashisms into acount. Thx for pointing me to that script. > > > + [ $# -gt 1 ] && timeout="$2" > > - if [ "$TST_TIMEOUT" = -1 ]; then > > - tst_res TINFO "Timeout per run is disabled" > > - return > > - fi > > + LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > > local err="LTP_TIMEOUT_MUL must be number >= 1!" > > @@ -396,13 +395,29 @@ _tst_setup_timer() > > LTP_TIMEOUT_MUL=$((LTP_TIMEOUT_MUL+1)) > > tst_res TINFO "ceiling LTP_TIMEOUT_MUL to > > $LTP_TIMEOUT_MUL" > > fi > > + > > [ "$LTP_TIMEOUT_MUL" -ge 1 ] || tst_brk TBROK "$err > > ($LTP_TIMEOUT_MUL)" > > + [ "$timeout" -ge 1 ] || tst_brk TBROK "timeout need to be > > >= 1 ($timeout)" > > + > > + eval "$1='$(( timeout * LTP_TIMEOUT_MUL))'" > > Eval on input, eval on output :). > > > + return 0 > > You don't use return value anywhere. + There is no return 1. > > +} > > Passing timeout variable name and optionally timeout value works and > allows > TBROK messages not to be mangled/hidden (which would be if function > echo the > result, which is then read the usual way: > timeout=$(tst_multiply_timeout 100) ), > but I'm not sure if all this is worth of just error handling. > Having 2x eval, $2 optionally used (but only in tests) makes code a > bit complex. In the end, I never called the function with the optional second parameter. So we could remove it and with it, the first eval. Would you be ok with just one eval ? > How about just simply save the result into global variable > $TST_TIMEOUT? Will not work, as this function is also used in TST_RETRY_FN_EXP_BACKOFF() where we do not use TST_TIMEOUT. thanks Clemens
Hi Clemens, > > Variable variable is possible to do portable way with eval. > > eval timeout=\$$1 > Ok, I will take bashisms into acount. Thx for pointing me to that > script. Thanks! > > Passing timeout variable name and optionally timeout value works and > > allows > > TBROK messages not to be mangled/hidden (which would be if function > > echo the > > result, which is then read the usual way: > > timeout=$(tst_multiply_timeout 100) ), > > but I'm not sure if all this is worth of just error handling. > > Having 2x eval, $2 optionally used (but only in tests) makes code a > > bit complex. > In the end, I never called the function with the optional second > parameter. So we could remove it and with it, the first eval. > Would you be ok with just one eval ? > > How about just simply save the result into global variable > > $TST_TIMEOUT? > Will not work, as this function is also used in > TST_RETRY_FN_EXP_BACKOFF() where we do not use TST_TIMEOUT. OK. Previously I was thinking to echo result or error message and handle error outside, depending on return value: timeout_or_error=$(tst_multiply_timeout 100) || tst_brk TBROK "$timeout_or_error" but that's not nice solution either. Your solution (with removed second parameter) is ok, it just looks unusual for me (passing variable name to be changed, something like "shell way of passing a pointer" is not really common), but as I don't see any other solution, I'm ok with that. But I'd like to get somebody else opinion, maybe we just don't see other obvious solution. Kind regards, Petr
Hi Clemens, I wonder if we want to note in docs that LTP_TIMEOUT_MUL in used in TST_RETRY_FN(). Kind regards, Petr
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index d8071cb10..ee220ac19 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -164,9 +164,11 @@ TST_RETRY_FN_EXP_BACKOFF() { local tst_fun="$1" local tst_exp=$2 - local tst_sec=$(expr $3 \* 1000000) + local tst_sec=$(( $3 * 1000000 )) local tst_delay=1 + tst_multiply_timeout tst_sec + if [ $# -ne 3 ]; then tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF expects 3 parameters" fi @@ -376,16 +378,13 @@ _tst_rescmp() fi } - -_tst_setup_timer() +tst_multiply_timeout() { - TST_TIMEOUT=${TST_TIMEOUT:-300} - LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} + # first parameter is used as return value + local timeout="${!1}" + [ $# -gt 1 ] && timeout="$2" - if [ "$TST_TIMEOUT" = -1 ]; then - tst_res TINFO "Timeout per run is disabled" - return - fi + LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} local err="LTP_TIMEOUT_MUL must be number >= 1!" @@ -396,13 +395,29 @@ _tst_setup_timer() LTP_TIMEOUT_MUL=$((LTP_TIMEOUT_MUL+1)) tst_res TINFO "ceiling LTP_TIMEOUT_MUL to $LTP_TIMEOUT_MUL" fi + [ "$LTP_TIMEOUT_MUL" -ge 1 ] || tst_brk TBROK "$err ($LTP_TIMEOUT_MUL)" + [ "$timeout" -ge 1 ] || tst_brk TBROK "timeout need to be >= 1 ($timeout)" + + eval "$1='$(( timeout * LTP_TIMEOUT_MUL))'" + return 0 +} + +_tst_setup_timer() +{ + TST_TIMEOUT=${TST_TIMEOUT:-300} + + if [ "$TST_TIMEOUT" = -1 ]; then + tst_res TINFO "Timeout per run is disabled" + return + fi if ! tst_is_int "$TST_TIMEOUT" || [ "$TST_TIMEOUT" -lt 1 ]; then tst_brk TBROK "TST_TIMEOUT must be int >= 1! ($TST_TIMEOUT)" fi - local sec=$((TST_TIMEOUT * LTP_TIMEOUT_MUL)) + local sec=$TST_TIMEOUT + tst_multiply_timeout sec local h=$((sec / 3600)) local m=$((sec / 60 % 60)) local s=$((sec % 60))
Because of timeout problems when using TST_RETRY_FN() we do now use LTP_TIMEOUT_MUL to adopt the timeout value. Introduced tst_multiply_timeout function to have a generic place to adopt timeout values. Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de> --- testcases/lib/tst_test.sh | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)