| Message ID | 20190911165225.2122-1-cfamullaconrad@suse.de |
|---|---|
| State | Superseded |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v2,1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() | expand |
On Thu, Sep 12, 2019 at 12:52 AM Clemens Famulla-Conrad < cfamullaconrad@suse.de> wrote: > Because of timeout problems when using TST_RETRY_FN() we do now use > LTP_TIMEOUT_MUL to adopt the timeout value. > > Introduced tst_adjut_timeout function to have a generic place to > adopt timeout values. > What about using tst_multipy_timeout as the function name? Since it only raises the timeout value with a multiplier. > > Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de> > --- > testcases/lib/tst_test.sh | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > index e0b24c6b9..03692e503 100644 > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -164,7 +164,7 @@ TST_RETRY_FN_EXP_BACKOFF() > { > local tst_fun="$1" > local tst_exp=$2 > - local tst_sec=$(expr $3 \* 1000000) > + local tst_sec=$(tst_adjust_timeout $(expr $3 \* 1000000)) > local tst_delay=1 > > if [ $# -ne 3 ]; then > @@ -371,12 +371,16 @@ _tst_rescmp() > fi > } > > - > -_tst_setup_timer() > +tst_adjust_timeout() > { > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > + local timeout=$1 > + echo $(( timeout * LTP_TIMEOUT_MUL)) > Shouldn't we check the LTP_TIMEOUT_MUL > 1 before using it?
On Thu, 2019-09-12 at 13:42 +0800, Li Wang wrote: > On Thu, Sep 12, 2019 at 12:52 AM Clemens Famulla-Conrad < > cfamullaconrad@suse.de> wrote: > > > Because of timeout problems when using TST_RETRY_FN() we do now use > > LTP_TIMEOUT_MUL to adopt the timeout value. > > > > Introduced tst_adjut_timeout function to have a generic place to > > adopt timeout values. > > > > What about using tst_multipy_timeout as the function name? Since it > only > raises the timeout value with a multiplier. I had a this patchset [1] in my mind. Maybe we will also apply a minimum. But we would still just multiply :) so Sure we can name it tst_multiply_timeout(). [1]https://patchwork.ozlabs.org/patch/1155460 > > > > > Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de> > > --- > > testcases/lib/tst_test.sh | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > > index e0b24c6b9..03692e503 100644 > > --- a/testcases/lib/tst_test.sh > > +++ b/testcases/lib/tst_test.sh > > @@ -164,7 +164,7 @@ TST_RETRY_FN_EXP_BACKOFF() > > { > > local tst_fun="$1" > > local tst_exp=$2 > > - local tst_sec=$(expr $3 \* 1000000) > > + local tst_sec=$(tst_adjust_timeout $(expr $3 \* 1000000)) > > local tst_delay=1 > > > > if [ $# -ne 3 ]; then > > @@ -371,12 +371,16 @@ _tst_rescmp() > > fi > > } > > > > - > > -_tst_setup_timer() > > +tst_adjust_timeout() > > { > > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > > + local timeout=$1 > > + echo $(( timeout * LTP_TIMEOUT_MUL)) > > > > Shouldn't we check the LTP_TIMEOUT_MUL > 1 before using it? Yes, thx for the hint.
Hi Clemens, > On Thu, 2019-09-12 at 13:42 +0800, Li Wang wrote: > > On Thu, Sep 12, 2019 at 12:52 AM Clemens Famulla-Conrad < > > cfamullaconrad@suse.de> wrote: > > > Because of timeout problems when using TST_RETRY_FN() we do now use > > > LTP_TIMEOUT_MUL to adopt the timeout value. > > > Introduced tst_adjut_timeout function to have a generic place to > > > adopt timeout values. > > What about using tst_multipy_timeout as the function name? Since it > > only > > raises the timeout value with a multiplier. +1. > I had a this patchset [1] in my mind. > Maybe we will also apply a minimum. But we would still just multiply :) > so Sure we can name it tst_multiply_timeout(). > [1]https://patchwork.ozlabs.org/patch/1155460 > > > Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de> > > > --- > > > testcases/lib/tst_test.sh | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > > > index e0b24c6b9..03692e503 100644 > > > --- a/testcases/lib/tst_test.sh > > > +++ b/testcases/lib/tst_test.sh > > > @@ -164,7 +164,7 @@ TST_RETRY_FN_EXP_BACKOFF() > > > { > > > local tst_fun="$1" > > > local tst_exp=$2 > > > - local tst_sec=$(expr $3 \* 1000000) > > > + local tst_sec=$(tst_adjust_timeout $(expr $3 \* 1000000)) > > > local tst_delay=1 > > > if [ $# -ne 3 ]; then > > > @@ -371,12 +371,16 @@ _tst_rescmp() > > > fi > > > } > > > - > > > -_tst_setup_timer() > > > +tst_adjust_timeout() > > > { > > > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > > > + local timeout=$1 > > > + echo $(( timeout * LTP_TIMEOUT_MUL)) > > Shouldn't we check the LTP_TIMEOUT_MUL > 1 before using it? Also timeout should be checked. I'd tst_brk TBROK if either of them is < 0. > Yes, thx for the hint. BTW: I haven't finish patchset doing the same [1]. Feel free to include docs from [2]. I'll also sent tonight LTP_TIMEOUT_MUL_MIN (as part of v2 for [3]). But it should be trivial to rebase changes of the later commit (whoever gets merged first). Kind regards, Petr [1] https://patchwork.ozlabs.org/project/ltp/list/?series=120151&state=* [2] https://patchwork.ozlabs.org/patch/1133627/ [3] https://patchwork.ozlabs.org/patch/1155460/
On Thu, 2019-09-12 at 16:55 +0200, Petr Vorel wrote: > Hi Clemens, > > > On Thu, 2019-09-12 at 13:42 +0800, Li Wang wrote: > > > On Thu, Sep 12, 2019 at 12:52 AM Clemens Famulla-Conrad < > > > cfamullaconrad@suse.de> wrote: > > > > Because of timeout problems when using TST_RETRY_FN() we do now > > > > use > > > > LTP_TIMEOUT_MUL to adopt the timeout value. > > > > Introduced tst_adjut_timeout function to have a generic place > > > > to > > > > adopt timeout values. > > > > > What about using tst_multipy_timeout as the function name? Since > > > it > > > only > > > raises the timeout value with a multiplier. > > +1. > > > I had a this patchset [1] in my mind. > > Maybe we will also apply a minimum. But we would still just > > multiply :) > > so Sure we can name it tst_multiply_timeout(). > > [1]https://patchwork.ozlabs.org/patch/1155460 > > > > > > > Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de> > > > > --- > > > > testcases/lib/tst_test.sh | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/testcases/lib/tst_test.sh > > > > b/testcases/lib/tst_test.sh > > > > index e0b24c6b9..03692e503 100644 > > > > --- a/testcases/lib/tst_test.sh > > > > +++ b/testcases/lib/tst_test.sh > > > > @@ -164,7 +164,7 @@ TST_RETRY_FN_EXP_BACKOFF() > > > > { > > > > local tst_fun="$1" > > > > local tst_exp=$2 > > > > - local tst_sec=$(expr $3 \* 1000000) > > > > + local tst_sec=$(tst_adjust_timeout $(expr $3 \* > > > > 1000000)) > > > > local tst_delay=1 > > > > if [ $# -ne 3 ]; then > > > > @@ -371,12 +371,16 @@ _tst_rescmp() > > > > fi > > > > } > > > > - > > > > -_tst_setup_timer() > > > > +tst_adjust_timeout() > > > > { > > > > LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} > > > > + local timeout=$1 > > > > + echo $(( timeout * LTP_TIMEOUT_MUL)) > > > Shouldn't we check the LTP_TIMEOUT_MUL > 1 before using it? > > Also timeout should be checked. > I'd tst_brk TBROK if either of them is < 0. > > > Yes, thx for the hint. > > BTW: I haven't finish patchset doing the same [1]. Feel free to > include docs > from [2]. Thx for that hint, was aware of that patchset [1]. > I'll also sent tonight LTP_TIMEOUT_MUL_MIN (as part of v2 for [3]). > But it should be trivial to rebase changes of the later commit > (whoever gets merged first). > > > Kind regards, > Petr > > [1] https://patchwork.ozlabs.org/project/ltp/list/?series=120151&stat > e=* > [2] https://patchwork.ozlabs.org/patch/1133627/ > [3] https://patchwork.ozlabs.org/patch/1155460/
On Thu, 2019-09-12 at 18:49 +0200, Clemens Famulla-Conrad wrote: > On Thu, 2019-09-12 at 16:55 +0200, Petr Vorel wrote: > > > > BTW: I haven't finish patchset doing the same [1]. Feel free to > > include docs > > from [2]. > > Thx for that hint, was aware of that patchset [1]. ^ not (important detail :)
Hi Clemens, ... > - local tst_sec=$(expr $3 \* 1000000) > + local tst_sec=$(tst_adjust_timeout $(expr $3 \* 1000000)) Just a style comment: instead of using $(expr ), I'd switch to $(( )). Kind regards, Petr
Hi Clemens, > Because of timeout problems when using TST_RETRY_FN() we do now use > LTP_TIMEOUT_MUL to adopt the timeout value. > Introduced tst_adjut_timeout function to have a generic place to > adopt timeout values. @Li, Cyril, Jan acking this? Acked-by: Petr Vorel <pvorel@suse.cz> for whole patchset, with suggestion to use $(( )) instead of $(expr ) [1]. (was used originaly, but when you're touching the code, could you please change it?) I'm for merging this patchset, but I'd rather merge my "v5 shell: Introduce TST_TIMEOUT variable" [2] [3] patchset first. I can rebase this patchset before merge. Kind regards, Petr [1] https://patchwork.ozlabs.org/patch/1161157/#2258600 [2] https://patchwork.ozlabs.org/patch/1175082/ [3] https://patchwork.ozlabs.org/project/ltp/list/?series=135548&state=*
Hi! > I'm for merging this patchset, but I'd rather merge my "v5 shell: Introduce > TST_TIMEOUT variable" [2] [3] patchset first. > I can rebase this patchset before merge. Well, we still need to ceil the LTP_TIMEOUT_MUL in the shell version of the TST_RETRY_FN_EXP_BACKOFF. So I would say that your patch has to go first, then we need to export the part that ceils the timeout multiplier as a function and finally use it in the first patch of this patchset.
On Fri, Oct 11, 2019 at 8:53 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > I'm for merging this patchset, but I'd rather merge my "v5 shell: > Introduce > > TST_TIMEOUT variable" [2] [3] patchset first. > > I can rebase this patchset before merge. > > Well, we still need to ceil the LTP_TIMEOUT_MUL in the shell version of > the TST_RETRY_FN_EXP_BACKOFF. So I would say that your patch has to go > first, then we need to export the part that ceils the timeout multiplier > as a function and finally use it in the first patch of this patchset. > Agreed. @Petr, I have no more comments, feel free to merge this with the changing as we discussed.
On Sat, 2019-10-12 at 10:46 +0800, Li Wang wrote: <snip> > Agreed. > > @Petr, I have no more comments, feel free to merge this with the > changing > as we discussed. > @Petr, I have v3 almost ready. Will send it soon.
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index e0b24c6b9..03692e503 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -164,7 +164,7 @@ TST_RETRY_FN_EXP_BACKOFF() { local tst_fun="$1" local tst_exp=$2 - local tst_sec=$(expr $3 \* 1000000) + local tst_sec=$(tst_adjust_timeout $(expr $3 \* 1000000)) local tst_delay=1 if [ $# -ne 3 ]; then @@ -371,12 +371,16 @@ _tst_rescmp() fi } - -_tst_setup_timer() +tst_adjust_timeout() { LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1} + local timeout=$1 + echo $(( timeout * LTP_TIMEOUT_MUL)) +} - local sec=$((300 * LTP_TIMEOUT_MUL)) +_tst_setup_timer() +{ + local sec=$(tst_adjust_timeout 300) 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_adjut_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 | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)