* [LTP] [PATCH v2 2/3] tst_test.c: Add tst_adjust_timeout()
2019-09-11 16:52 ` [LTP] [PATCH v2 1/3] tst_test.sh: " Clemens Famulla-Conrad
@ 2019-09-11 16:52 ` Clemens Famulla-Conrad
2019-09-11 16:52 ` [LTP] [PATCH v2 3/3] tst_common.h: Use tst_adjust_timeout in TST_RETRY_FN() Clemens Famulla-Conrad
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-09-11 16:52 UTC (permalink / raw)
To: ltp
This function is used to adjust timeout values with environment
variables like LTP_TIMEOUT_MUL.
Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de>
---
include/tst_test.h | 1 +
lib/tst_test.c | 32 ++++++++++++++++++++------------
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/include/tst_test.h b/include/tst_test.h
index cdeaf6ad0..908015846 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -261,6 +261,7 @@ const char *tst_strsig(int sig);
const char *tst_strstatus(int status);
unsigned int tst_timeout_remaining(void);
+unsigned int tst_adjust_timeout(unsigned int timeout);
void tst_set_timeout(int timeout);
diff --git a/lib/tst_test.c b/lib/tst_test.c
index d2cf92608..1283dba1a 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -36,6 +36,7 @@ struct tst_test *tst_test;
static const char *tid;
static int iterations = 1;
static float duration = -1;
+static float timeout_mul = -1;
static pid_t main_pid, lib_pid;
static int mntpoint_mounted;
static int ovl_mounted;
@@ -1087,25 +1088,32 @@ unsigned int tst_timeout_remaining(void)
return 0;
}
-void tst_set_timeout(int timeout)
+unsigned int tst_adjust_timeout(unsigned int timeout)
{
- char *mul = getenv("LTP_TIMEOUT_MUL");
+ char *mul;
+ if (timeout_mul == -1){
+ mul = getenv("LTP_TIMEOUT_MUL");
+ if (mul) {
+ timeout_mul = atof(mul);
+ if (timeout_mul < 1)
+ tst_brk(TBROK,
+ "Invalid timeout multiplier '%s'",
+ mul);
+ } else {
+ timeout_mul = 1;
+ }
+ }
+ return timeout * timeout_mul + 0.5;
+}
+void tst_set_timeout(int timeout)
+{
if (timeout == -1) {
tst_res(TINFO, "Timeout per run is disabled");
return;
}
- results->timeout = timeout;
-
- if (mul) {
- float m = atof(mul);
-
- if (m < 1)
- tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
-
- results->timeout = results->timeout * m + 0.5;
- }
+ results->timeout = tst_adjust_timeout(timeout);
tst_res(TINFO, "Timeout per run is %uh %02um %02us",
results->timeout/3600, (results->timeout%3600)/60,
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 3/3] tst_common.h: Use tst_adjust_timeout in TST_RETRY_FN()
2019-09-11 16:52 ` [LTP] [PATCH v2 1/3] tst_test.sh: " Clemens Famulla-Conrad
2019-09-11 16:52 ` [LTP] [PATCH v2 2/3] tst_test.c: Add tst_adjust_timeout() Clemens Famulla-Conrad
@ 2019-09-11 16:52 ` Clemens Famulla-Conrad
2019-09-12 5:42 ` [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL " Li Wang
` (2 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-09-11 16:52 UTC (permalink / raw)
To: ltp
Because of timeout problems when using TST_RETRY_FN() we want a LTP_TIMEOUT_MUL
adopted timeout value here as well.
Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de>
---
include/tst_common.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/tst_common.h b/include/tst_common.h
index c21505450..ca8f06d01 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -50,12 +50,14 @@
TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1)
#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY) \
-({ int tst_delay_ = 1; \
+({ unsigned int tst_delay_, tst_max_delay_; \
+ tst_delay_ = 1; \
+ tst_max_delay_ = tst_adjust_timeout(MAX_DELAY * 1000000); \
for (;;) { \
typeof(FUNC) tst_ret_ = FUNC; \
if (tst_ret_ == ERET) \
break; \
- if (tst_delay_ < MAX_DELAY * 1000000) { \
+ if (tst_delay_ < tst_max_delay_) { \
usleep(tst_delay_); \
tst_delay_ *= 2; \
} else { \
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-11 16:52 ` [LTP] [PATCH v2 1/3] tst_test.sh: " Clemens Famulla-Conrad
2019-09-11 16:52 ` [LTP] [PATCH v2 2/3] tst_test.c: Add tst_adjust_timeout() Clemens Famulla-Conrad
2019-09-11 16:52 ` [LTP] [PATCH v2 3/3] tst_common.h: Use tst_adjust_timeout in TST_RETRY_FN() Clemens Famulla-Conrad
@ 2019-09-12 5:42 ` Li Wang
2019-09-12 8:52 ` Clemens Famulla-Conrad
2019-09-12 21:51 ` Petr Vorel
2019-10-11 12:02 ` Petr Vorel
4 siblings, 1 reply; 27+ messages in thread
From: Li Wang @ 2019-09-12 5:42 UTC (permalink / raw)
To: ltp
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?
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190912/d28e6372/attachment.htm>
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-12 5:42 ` [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL " Li Wang
@ 2019-09-12 8:52 ` Clemens Famulla-Conrad
2019-09-12 14:55 ` Petr Vorel
0 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-09-12 8:52 UTC (permalink / raw)
To: ltp
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.
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-12 8:52 ` Clemens Famulla-Conrad
@ 2019-09-12 14:55 ` Petr Vorel
2019-09-12 16:49 ` Clemens Famulla-Conrad
0 siblings, 1 reply; 27+ messages in thread
From: Petr Vorel @ 2019-09-12 14:55 UTC (permalink / raw)
To: ltp
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/
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-12 14:55 ` Petr Vorel
@ 2019-09-12 16:49 ` Clemens Famulla-Conrad
2019-09-12 19:46 ` Clemens Famulla-Conrad
0 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-09-12 16:49 UTC (permalink / raw)
To: ltp
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/
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-11 16:52 ` [LTP] [PATCH v2 1/3] tst_test.sh: " Clemens Famulla-Conrad
` (2 preceding siblings ...)
2019-09-12 5:42 ` [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL " Li Wang
@ 2019-09-12 21:51 ` Petr Vorel
2019-10-11 12:02 ` Petr Vorel
4 siblings, 0 replies; 27+ messages in thread
From: Petr Vorel @ 2019-09-12 21:51 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-09-11 16:52 ` [LTP] [PATCH v2 1/3] tst_test.sh: " Clemens Famulla-Conrad
` (3 preceding siblings ...)
2019-09-12 21:51 ` Petr Vorel
@ 2019-10-11 12:02 ` Petr Vorel
2019-10-11 12:53 ` Cyril Hrubis
4 siblings, 1 reply; 27+ messages in thread
From: Petr Vorel @ 2019-10-11 12:02 UTC (permalink / raw)
To: ltp
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=*
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-11 12:02 ` Petr Vorel
@ 2019-10-11 12:53 ` Cyril Hrubis
2019-10-12 2:46 ` Li Wang
0 siblings, 1 reply; 27+ messages in thread
From: Cyril Hrubis @ 2019-10-11 12:53 UTC (permalink / raw)
To: ltp
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.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-11 12:53 ` Cyril Hrubis
@ 2019-10-12 2:46 ` Li Wang
2019-10-16 11:25 ` Clemens Famulla-Conrad
0 siblings, 1 reply; 27+ messages in thread
From: Li Wang @ 2019-10-12 2:46 UTC (permalink / raw)
To: ltp
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.
--
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191012/13570177/attachment.htm>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v2 1/3] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-12 2:46 ` Li Wang
@ 2019-10-16 11:25 ` Clemens Famulla-Conrad
2019-10-16 16:15 ` [LTP] [PATCH v3 1/4] " Clemens Famulla-Conrad
0 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-16 11:25 UTC (permalink / raw)
To: ltp
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-16 11:25 ` Clemens Famulla-Conrad
@ 2019-10-16 16:15 ` Clemens Famulla-Conrad
2019-10-16 16:15 ` [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout() Clemens Famulla-Conrad
` (3 more replies)
0 siblings, 4 replies; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-16 16:15 UTC (permalink / raw)
To: ltp
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(-)
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))
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout()
2019-10-16 16:15 ` [LTP] [PATCH v3 1/4] " Clemens Famulla-Conrad
@ 2019-10-16 16:15 ` Clemens Famulla-Conrad
2019-10-17 8:47 ` Petr Vorel
2019-10-16 16:15 ` [LTP] [PATCH v3 3/4] tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN() Clemens Famulla-Conrad
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-16 16:15 UTC (permalink / raw)
To: ltp
This function is used to adjust timeout values with environment
variables like LTP_TIMEOUT_MUL.
Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de>
---
include/tst_test.h | 1 +
lib/tst_test.c | 41 ++++++++++++++++++++++++++++++-----------
2 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/include/tst_test.h b/include/tst_test.h
index 84acf2c59..aaea128ba 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -267,6 +267,7 @@ const char *tst_strsig(int sig);
const char *tst_strstatus(int status);
unsigned int tst_timeout_remaining(void);
+unsigned int tst_multiply_timeout(unsigned int timeout);
void tst_set_timeout(int timeout);
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 6239acf89..6d92d8775 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -36,6 +36,7 @@ struct tst_test *tst_test;
static const char *tid;
static int iterations = 1;
static float duration = -1;
+static int timeout_mul = -1;
static pid_t main_pid, lib_pid;
static int mntpoint_mounted;
static int ovl_mounted;
@@ -1093,25 +1094,43 @@ unsigned int tst_timeout_remaining(void)
return 0;
}
-void tst_set_timeout(int timeout)
+unsigned int tst_multiply_timeout(unsigned int timeout)
{
- char *mul = getenv("LTP_TIMEOUT_MUL");
+ char *mul;
+ float mul_float;
+
+ if (timeout_mul == -1){
+ mul = getenv("LTP_TIMEOUT_MUL");
+ if (mul) {
+ timeout_mul = mul_float = atof(mul);
+ if (timeout_mul != mul_float){
+ timeout_mul++;
+ tst_res(TINFO, "ceiling LTP_TIMEOUT_MUL to %d", timeout_mul);
+ }
+ } else {
+ timeout_mul = 1;
+ }
+ }
+ if (timeout_mul < 1)
+ tst_brk(TBROK, "LTP_TIMEOUT_MUL need to be int >= 1! (%d)", timeout_mul);
+
+ if (timeout < 1)
+ tst_brk(TBROK, "timeout need to be >= 1! (%d)", timeout);
+
+ return timeout * timeout_mul;
+}
+void tst_set_timeout(int timeout)
+{
if (timeout == -1) {
tst_res(TINFO, "Timeout per run is disabled");
return;
}
- results->timeout = timeout;
+ if (timeout < 1)
+ tst_brk(TBROK, "timeout need to be >= 1! (%d)", timeout);
- if (mul) {
- float m = atof(mul);
-
- if (m < 1)
- tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
-
- results->timeout = results->timeout * m + 0.5;
- }
+ results->timeout = tst_multiply_timeout(timeout);
tst_res(TINFO, "Timeout per run is %uh %02um %02us",
results->timeout/3600, (results->timeout%3600)/60,
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout()
2019-10-16 16:15 ` [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout() Clemens Famulla-Conrad
@ 2019-10-17 8:47 ` Petr Vorel
0 siblings, 0 replies; 27+ messages in thread
From: Petr Vorel @ 2019-10-17 8:47 UTC (permalink / raw)
To: ltp
Hi Clemens,
> This function is used to adjust timeout values with environment
> variables like LTP_TIMEOUT_MUL.
> Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
LGTM, minor formatting/wording bellow.
...
> -void tst_set_timeout(int timeout)
> +unsigned int tst_multiply_timeout(unsigned int timeout)
> {
> - char *mul = getenv("LTP_TIMEOUT_MUL");
> + char *mul;
> + float mul_float;
> +
> + if (timeout_mul == -1){
Please add space before { (on more places + unnecessary { }).
linux/scripts/checkpatch.pl --terse --no-tree -f lib/tst_test.c
would tell you :)
([1] but we don't follow it fully - common sense is applied, but we try).
> + mul = getenv("LTP_TIMEOUT_MUL");
> + if (mul) {
> + timeout_mul = mul_float = atof(mul);
> + if (timeout_mul != mul_float){
> + timeout_mul++;
> + tst_res(TINFO, "ceiling LTP_TIMEOUT_MUL to %d", timeout_mul);
> + }
> + } else {
> + timeout_mul = 1;
> + }
> + }
> + if (timeout_mul < 1)
> + tst_brk(TBROK, "LTP_TIMEOUT_MUL need to be int >= 1! (%d)", timeout_mul);
> +
> + if (timeout < 1)
> + tst_brk(TBROK, "timeout need to be >= 1! (%d)", timeout);
nit: need => maybe must/needs ?
Kind regards,
Petr
[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v3 3/4] tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN()
2019-10-16 16:15 ` [LTP] [PATCH v3 1/4] " Clemens Famulla-Conrad
2019-10-16 16:15 ` [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout() Clemens Famulla-Conrad
@ 2019-10-16 16:15 ` Clemens Famulla-Conrad
2019-10-17 8:48 ` Petr Vorel
2019-10-16 16:15 ` [LTP] [PATCH v3 4/4] Add newlib shell test for tst_multiply_timeout() Clemens Famulla-Conrad
2019-10-17 8:39 ` [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() Petr Vorel
3 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-16 16:15 UTC (permalink / raw)
To: ltp
Because of timeout problems when using TST_RETRY_FN() we want a LTP_TIMEOUT_MUL
adopted timeout value here as well.
Signed-off-by: Clemens Famulla-Conrad <cfamullaconrad@suse.de>
---
include/tst_common.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/tst_common.h b/include/tst_common.h
index c21505450..b901273b0 100644
--- a/include/tst_common.h
+++ b/include/tst_common.h
@@ -50,12 +50,14 @@
TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, 1)
#define TST_RETRY_FN_EXP_BACKOFF(FUNC, ERET, MAX_DELAY) \
-({ int tst_delay_ = 1; \
+({ unsigned int tst_delay_, tst_max_delay_; \
+ tst_delay_ = 1; \
+ tst_max_delay_ = tst_multiply_timeout(MAX_DELAY * 1000000); \
for (;;) { \
typeof(FUNC) tst_ret_ = FUNC; \
if (tst_ret_ == ERET) \
break; \
- if (tst_delay_ < MAX_DELAY * 1000000) { \
+ if (tst_delay_ < tst_max_delay_) { \
usleep(tst_delay_); \
tst_delay_ *= 2; \
} else { \
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 4/4] Add newlib shell test for tst_multiply_timeout()
2019-10-16 16:15 ` [LTP] [PATCH v3 1/4] " Clemens Famulla-Conrad
2019-10-16 16:15 ` [LTP] [PATCH v3 2/4] tst_test.c: Add tst_multiply_timeout() Clemens Famulla-Conrad
2019-10-16 16:15 ` [LTP] [PATCH v3 3/4] tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN() Clemens Famulla-Conrad
@ 2019-10-16 16:15 ` Clemens Famulla-Conrad
2019-10-17 9:00 ` Petr Vorel
2019-10-17 8:39 ` [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() Petr Vorel
3 siblings, 1 reply; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-16 16:15 UTC (permalink / raw)
To: ltp
Simple test for different kinds of calls from tst_multiply_timeout()
---
lib/newlib_tests/shell/test_timeout_mul.sh | 54 ++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100755 lib/newlib_tests/shell/test_timeout_mul.sh
diff --git a/lib/newlib_tests/shell/test_timeout_mul.sh b/lib/newlib_tests/shell/test_timeout_mul.sh
new file mode 100755
index 000000000..8397df25c
--- /dev/null
+++ b/lib/newlib_tests/shell/test_timeout_mul.sh
@@ -0,0 +1,54 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (c) 2019 Clemens Famulla-Conrad <cfamullaconrad@suse.de>
+
+TST_TESTFUNC=do_test
+. tst_test.sh
+
+
+call_it()
+{
+ local SAVE_MUL=${LTP_TIMEOUT_MUL}
+ $1
+ if [ "${!2}" -ne "$3" ]; then
+ tst_brk TBROK "LTP_TIMEOUT_MUL=$SAVE_MUL $1 (${!2} != $3)"
+ else
+ tst_res TPASS "LTP_TIMEOUT_MUL=$SAVE_MUL $1 (${!2} == $3)"
+ fi
+}
+
+do_test()
+{
+ LTP_TIMEOUT_MUL=2
+ local sec=1
+
+ call_it 'tst_multiply_timeout sec' 'sec' 2
+ call_it 'tst_multiply_timeout foo 1000' 'foo' 2000
+
+ sec=1
+ call_it 'tst_multiply_timeout sec 500' 'sec' 1000
+
+ sec=1
+ LTP_TIMEOUT_MUL="1.5"
+ call_it 'tst_multiply_timeout sec' 'sec' 2
+
+ sec=1
+ LTP_TIMEOUT_MUL=0.5
+ call_it 'tst_multiply_timeout sec' 'sec' 1
+
+ sec=1
+ LTP_TIMEOUT_MUL=1.5
+ call_it 'tst_multiply_timeout sec 5' 'sec' 10
+
+ sec=1
+ LTP_TIMEOUT_MUL=0.5
+ call_it 'tst_multiply_timeout sec 5' 'sec' 5
+
+ sec=1
+ LTP_TIMEOUT_MUL=2
+ call_it 'tst_multiply_timeout sec' 'sec' 2
+ call_it 'tst_multiply_timeout sec' 'sec' 4
+ call_it 'tst_multiply_timeout sec' 'sec' 8
+}
+
+tst_run;
--
2.16.4
^ permalink raw reply related [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 4/4] Add newlib shell test for tst_multiply_timeout()
2019-10-16 16:15 ` [LTP] [PATCH v3 4/4] Add newlib shell test for tst_multiply_timeout() Clemens Famulla-Conrad
@ 2019-10-17 9:00 ` Petr Vorel
0 siblings, 0 replies; 27+ messages in thread
From: Petr Vorel @ 2019-10-17 9:00 UTC (permalink / raw)
To: ltp
Hi Clemens,
> Simple test for different kinds of calls from tst_multiply_timeout()
> ---
> lib/newlib_tests/shell/test_timeout_mul.sh | 54 ++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100755 lib/newlib_tests/shell/test_timeout_mul.sh
> diff --git a/lib/newlib_tests/shell/test_timeout_mul.sh b/lib/newlib_tests/shell/test_timeout_mul.sh
> new file mode 100755
> index 000000000..8397df25c
> --- /dev/null
> +++ b/lib/newlib_tests/shell/test_timeout_mul.sh
> @@ -0,0 +1,54 @@
> +#!/bin/bash
This is library test, which will run on developer box, where bash is for sure.
But I'd still prefer to have /bin/sh with posix shell syntax.
Mainly because if you have /bin/bash in sources people will write tests with
bashisms (even if you document that bash is only allowed for these library
tests).
But we should have a discussion on that and document it ([1]).
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (c) 2019 Clemens Famulla-Conrad <cfamullaconrad@suse.de>
> +
> +TST_TESTFUNC=do_test
> +. tst_test.sh
This expect PATH to be set correctly. Which is probably better approach, than
mine (I put PATH for tst_test.sh into
lib/newlib_tests/shell/test_timeout_mul.sh; best would be to have 'make check',
which would set it; my patch [2] does not set it).
> +
> +
> +call_it()
> +{
> + local SAVE_MUL=${LTP_TIMEOUT_MUL}
> + $1
> + if [ "${!2}" -ne "$3" ]; then
> + tst_brk TBROK "LTP_TIMEOUT_MUL=$SAVE_MUL $1 (${!2} != $3)"
> + else
> + tst_res TPASS "LTP_TIMEOUT_MUL=$SAVE_MUL $1 (${!2} == $3)"
> + fi
> +}
> +
> +do_test()
> +{
> + LTP_TIMEOUT_MUL=2
> + local sec=1
> +
> + call_it 'tst_multiply_timeout sec' 'sec' 2
> + call_it 'tst_multiply_timeout foo 1000' 'foo' 2000
> +
> + sec=1
You mix tabs and spaces.
> + call_it 'tst_multiply_timeout sec 500' 'sec' 1000
> +
> + sec=1
> + LTP_TIMEOUT_MUL="1.5"
> + call_it 'tst_multiply_timeout sec' 'sec' 2
> +
> + sec=1
> + LTP_TIMEOUT_MUL=0.5
> + call_it 'tst_multiply_timeout sec' 'sec' 1
> +
> + sec=1
> + LTP_TIMEOUT_MUL=1.5
> + call_it 'tst_multiply_timeout sec 5' 'sec' 10
> +
> + sec=1
> + LTP_TIMEOUT_MUL=0.5
> + call_it 'tst_multiply_timeout sec 5' 'sec' 5
> +
> + sec=1
> + LTP_TIMEOUT_MUL=2
> + call_it 'tst_multiply_timeout sec' 'sec' 2
> + call_it 'tst_multiply_timeout sec' 'sec' 4
> + call_it 'tst_multiply_timeout sec' 'sec' 8
> +}
> +
> +tst_run;
Unnecessary ;
Kind regards,
Petr
[1] https://patchwork.ozlabs.org/patch/1166786/
[2] https://patchwork.ozlabs.org/patch/1166785/
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-16 16:15 ` [LTP] [PATCH v3 1/4] " Clemens Famulla-Conrad
` (2 preceding siblings ...)
2019-10-16 16:15 ` [LTP] [PATCH v3 4/4] Add newlib shell test for tst_multiply_timeout() Clemens Famulla-Conrad
@ 2019-10-17 8:39 ` Petr Vorel
2019-10-17 12:23 ` Clemens Famulla-Conrad
3 siblings, 1 reply; 27+ messages in thread
From: Petr Vorel @ 2019-10-17 8:39 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-17 8:39 ` [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() Petr Vorel
@ 2019-10-17 12:23 ` Clemens Famulla-Conrad
2019-10-18 7:46 ` Petr Vorel
2019-10-18 8:29 ` Petr Vorel
0 siblings, 2 replies; 27+ messages in thread
From: Clemens Famulla-Conrad @ 2019-10-17 12:23 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 27+ messages in thread* [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-17 12:23 ` Clemens Famulla-Conrad
@ 2019-10-18 7:46 ` Petr Vorel
2019-10-18 8:29 ` Petr Vorel
1 sibling, 0 replies; 27+ messages in thread
From: Petr Vorel @ 2019-10-18 7:46 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [LTP] [PATCH v3 1/4] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
2019-10-17 12:23 ` Clemens Famulla-Conrad
2019-10-18 7:46 ` Petr Vorel
@ 2019-10-18 8:29 ` Petr Vorel
1 sibling, 0 replies; 27+ messages in thread
From: Petr Vorel @ 2019-10-18 8:29 UTC (permalink / raw)
To: ltp
Hi Clemens,
I wonder if we want to note in docs that LTP_TIMEOUT_MUL in used in
TST_RETRY_FN().
Kind regards,
Petr
^ permalink raw reply [flat|nested] 27+ messages in thread