[v2,1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
diff mbox series

Message ID 20190911165225.2122-1-cfamullaconrad@suse.de
State New
Headers show
Series
  • [v2,1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
Related show

Commit Message

Clemens Famulla-Conrad Sept. 11, 2019, 4:52 p.m. UTC
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(-)

Comments

Li Wang Sept. 12, 2019, 5:42 a.m. UTC | #1
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?
Clemens Famulla-Conrad Sept. 12, 2019, 8:52 a.m. UTC | #2
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.
Petr Vorel Sept. 12, 2019, 2:55 p.m. UTC | #3
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/
Clemens Famulla-Conrad Sept. 12, 2019, 4:49 p.m. UTC | #4
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/
Clemens Famulla-Conrad Sept. 12, 2019, 7:46 p.m. UTC | #5
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 :)
Petr Vorel Sept. 12, 2019, 9:51 p.m. UTC | #6
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
Petr Vorel Oct. 11, 2019, 12:02 p.m. UTC | #7
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=*
Cyril Hrubis Oct. 11, 2019, 12:53 p.m. UTC | #8
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.
Li Wang Oct. 12, 2019, 2:46 a.m. UTC | #9
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.

Patch
diff mbox series

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