[LTP] [PATCH v4 0/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()

[LTP] [PATCH v4 0/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()

[Clemens Famulla-Conrad]

* tst_test.sh:
  Force one parameter and fix bashisms.
  But I still need two time eval.
* test_timeout_mul.sh:
  Formatting, but don't add PATH somewhere, as we wait for a runner.
* Mention LTP_TIMEOUT_MUL usage in TST_RETRY_FN() in doc


Clemens Famulla-Conrad (5):
  tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()
  tst_test.c: Add tst_multiply_timeout()
  tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN()
  Add newlib shell test for tst_multiply_timeout()
  Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

 doc/test-writing-guidelines.txt            |  3 ++-
 include/tst_common.h                       |  6 +++--
 include/tst_test.h                         |  1 +
 lib/newlib_tests/shell/test_timeout_mul.sh | 43 ++++++++++++++++++++++++++++++
 lib/tst_test.c                             | 43 ++++++++++++++++++++++--------
 testcases/lib/tst_test.sh                  | 34 ++++++++++++++++-------
 6 files changed, 106 insertions(+), 24 deletions(-)
 create mode 100755 lib/newlib_tests/shell/test_timeout_mul.sh

--
2.16.4

[LTP] [PATCH v4 1/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()

[Clemens Famulla-Conrad]

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 | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index d8071cb10..1ef6712b6 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,12 @@ _tst_rescmp()
 	fi
 }
 
-
-_tst_setup_timer()
+tst_multiply_timeout()
 {
-	TST_TIMEOUT=${TST_TIMEOUT:-300}
-	LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
+	[ $# -ne 1 ] && tst_brk TBROK "tst_multiply_timeout expect 1 parameter"
+	eval "local timeout=\$$1"
 
-	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 +394,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

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Clemens Famulla-Conrad]

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>
---
 include/tst_test.h |  1 +
 lib/tst_test.c     | 43 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 33 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..5f43b1e0b 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,45 @@ 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 must to be int >= 1! (%d)",
+				timeout_mul);
+
+	if (timeout < 1)
+		tst_brk(TBROK, "timeout must 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

[LTP] [PATCH v4 3/5] tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN()

[Clemens Famulla-Conrad]

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>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 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

[LTP] [PATCH v4 4/5] Add newlib shell test for tst_multiply_timeout()

[Clemens Famulla-Conrad]

Simple test for different kinds of calls from tst_multiply_timeout()
---
 lib/newlib_tests/shell/test_timeout_mul.sh | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 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..6682e5d66
--- /dev/null
+++ b/lib/newlib_tests/shell/test_timeout_mul.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+# 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
+	eval "exp_value=\$$2"
+	if [ "$exp_value" -ne "$3" ]; then
+		tst_brk TBROK "LTP_TIMEOUT_MUL=$SAVE_MUL $1 ($exp_value != $3)"
+	else
+		tst_res TPASS "LTP_TIMEOUT_MUL=$SAVE_MUL $1 ($exp_value == $3)"
+	fi
+}
+
+do_test()
+{
+	LTP_TIMEOUT_MUL=2
+	local sec=1
+
+	call_it 'tst_multiply_timeout sec' 'sec' 2
+
+	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=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

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Clemens Famulla-Conrad]

Mention that time limit is mulitplied with LTP_TIMEOUT_MUL.
---
 doc/test-writing-guidelines.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index efff4d40c..905a4baa5 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2313,7 +2313,8 @@ Retry a function in limited time
 Sometimes LTP test needs retrying a function for many times to get success.
 This achievement makes that possible via keeping it retrying if the return
 value of the function is NOT as we expected. After exceeding a limited time,
-test will break from the retries immediately.
+test will break from the retries immediately. The time limit is multiplied
+with LTP_TIMEOUT_MUL.
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.16.4

[LTP] [PATCH v4 4/5] Add newlib shell test for tst_multiply_timeout()

[Li Wang]

On Fri, Oct 18, 2019 at 8:45 PM Clemens Famulla-Conrad <
cfamullaconrad@suse.de> wrote:

> Simple test for different kinds of calls from tst_multiply_timeout()
>

Patch 4/5 and 5/5 needs "Signed-off-by: ..." tag, otherwise LGTM.
    Reviewed-by: Li Wang <liwang@redhat.com>

---
>  lib/newlib_tests/shell/test_timeout_mul.sh | 43
> ++++++++++++++++++++++++++++++
>  1 file changed, 43 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..6682e5d66
> --- /dev/null
> +++ b/lib/newlib_tests/shell/test_timeout_mul.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +# 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
> +       eval "exp_value=\$$2"
> +       if [ "$exp_value" -ne "$3" ]; then
> +               tst_brk TBROK "LTP_TIMEOUT_MUL=$SAVE_MUL $1 ($exp_value !=
> $3)"
> +       else
> +               tst_res TPASS "LTP_TIMEOUT_MUL=$SAVE_MUL $1 ($exp_value ==
> $3)"
> +       fi
> +}
> +
> +do_test()
> +{
> +       LTP_TIMEOUT_MUL=2
> +       local sec=1
> +
> +       call_it 'tst_multiply_timeout sec' 'sec' 2
> +
> +       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=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
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191021/7de3ad38/attachment.htm>

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Richard Palethorpe]

Hello,

Clemens Famulla-Conrad <cfamullaconrad@suse.de> writes:

> Mention that time limit is mulitplied with LTP_TIMEOUT_MUL.
> ---
>  doc/test-writing-guidelines.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index efff4d40c..905a4baa5 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -2313,7 +2313,8 @@ Retry a function in limited time
>  Sometimes LTP test needs retrying a function for many times to get success.
>  This achievement makes that possible via keeping it retrying if the return
>  value of the function is NOT as we expected. After exceeding a limited time,
> -test will break from the retries immediately.
> +test will break from the retries immediately. The time limit is multiplied
> +with LTP_TIMEOUT_MUL.

I think the function tst_multiply_timeout also needs documenting.

>  
>  [source,c]
>  -------------------------------------------------------------------------------
> -- 
> 2.16.4


-- 
Thank you,
Richard.

[LTP] [PATCH v4 1/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()

[Petr Vorel]

Hi Clemens,

...
> +	tst_multiply_timeout tst_sec
> +
>  	if [ $# -ne 3 ]; then
>  		tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF expects 3 parameters"
>  	fi
> @@ -376,16 +378,12 @@ _tst_rescmp()
>  	fi
>  }

> -
> -_tst_setup_timer()
> +tst_multiply_timeout()
This is a private function, it should be called '_tst_multiply_timeout'.
This can be changed by person who merges the patchset.

Kind regards,
Petr

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Petr Vorel]

Hi Clements,

> +	if (timeout_mul < 1)
> +		tst_brk(TBROK, "LTP_TIMEOUT_MUL must to be int >= 1! (%d)",
> +				timeout_mul);
> +
> +	if (timeout < 1)
> +		tst_brk(TBROK, "timeout must 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);
need => needs, but better to use must (to be consistent with the previous one:
		tst_brk(TBROK, "timeout must to be >= 1! (%d)", timeout);

I also wonder, if this check is needed, next step is
results->timeout = tst_multiply_timeout(timeout);
which does the same check.

Can be changed with the committer (unless you plan to do v5 for some reason).

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

Kind regards,
Petr

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Petr Vorel]

Hi,

> Clemens Famulla-Conrad <cfamullaconrad@suse.de> writes:

> > Mention that time limit is mulitplied with LTP_TIMEOUT_MUL.
> > ---
> >  doc/test-writing-guidelines.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)

> > diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> > index efff4d40c..905a4baa5 100644
> > --- a/doc/test-writing-guidelines.txt
> > +++ b/doc/test-writing-guidelines.txt
> > @@ -2313,7 +2313,8 @@ Retry a function in limited time
> >  Sometimes LTP test needs retrying a function for many times to get success.
> >  This achievement makes that possible via keeping it retrying if the return
> >  value of the function is NOT as we expected. After exceeding a limited time,
> > -test will break from the retries immediately.
> > +test will break from the retries immediately. The time limit is multiplied
> > +with LTP_TIMEOUT_MUL.

> I think the function tst_multiply_timeout also needs documenting.
Isn't it meant to be used just in library (in lib/tst_test.c)?
Thus I wouldn't document it here. And in fact remove it from include/tst_test.h
(second commit).

Kind regards,
Petr

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Clemens Famulla-Conrad]

On Mon, 2019-10-21 at 15:15 +0200, Petr Vorel wrote:
<snip>
> > I think the function tst_multiply_timeout also needs documenting.
> 
> Isn't it meant to be used just in library (in lib/tst_test.c)?
> Thus I wouldn't document it here. And in fact remove it from
> include/tst_test.h
> (second commit).

I'm ok with private function. But you need to make it available to
tst_common.h. We could use extern. But not sure if this is what we
want.

On the other hand, we could make it as lib function, so others, who
need to set timeout in arbitrary way, have a utility to adjust there
timeout in a generic way. WDYT? But we also could leave this for later,
and simply change it if really needed.

Thanks
Clemens

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Petr Vorel]

Hi Clemens,

> > Isn't it meant to be used just in library (in lib/tst_test.c)?
> > Thus I wouldn't document it here. And in fact remove it from
> > include/tst_test.h
> > (second commit).

> I'm ok with private function. But you need to make it available to
> tst_common.h. We could use extern. But not sure if this is what we
> want.

> On the other hand, we could make it as lib function, so others, who
> need to set timeout in arbitrary way, have a utility to adjust there
> timeout in a generic way. WDYT? But we also could leave this for later,
> and simply change it if really needed.
Well, it's not that important to complicate things.
My point was, that function is used only internally, unlike tst_set_timeout()
and tst_timeout_remaining() which are also used in some tests.

> Thanks
> Clemens

Kind regards,
Petr

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Clemens Famulla-Conrad]

On Mon, 2019-10-21 at 14:50 +0200, Petr Vorel wrote:

> +	if (timeout < 1)
> > +		tst_brk(TBROK, "timeout need to be >= 1! (%d)",
> > timeout);
> 
> need => needs, but better to use must (to be consistent with the
> previous one:
> 		tst_brk(TBROK, "timeout must to be >= 1! (%d)",
> timeout);

agree

> I also wonder, if this check is needed, next step is
> results->timeout = tst_multiply_timeout(timeout);
> which does the same check.

In shell we have the same check. And there it is more clear, as we
refer to TST_TIMEOUT variable. Here both messages just say "timeout"
but the linenumber would be more close to the actual call.

kind regards
Clemens

[LTP] [PATCH v4 1/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN()

[Clemens Famulla-Conrad]

On Mon, 2019-10-21 at 14:42 +0200, Petr Vorel wrote:
> This is a private function, it should be called
> '_tst_multiply_timeout'.
> This can be changed by person who merges the patchset.

ok

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Cyril Hrubis]

Hi!
> +	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);
> +			}

Huh, why are we ceiling the timeout multiplier?

We do that for shell because it simplifies the code and we do not care
that much about being precise for timeouts, but it does not make much
sense here.

Why can't we just convert the env variable to float and multiply?

Something as:

	if (mul) {
		if (ret = tst_parse_float(mul, &timeout_mul, 1, 10000)) {
			tst_brk(TBROK, "Failed to parse LTP_TIMEOUT_MUL: %s",
			        tst_strerrno(ret));
		}
	} else {
		timeout_mul = 1;
	}


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 3/5] tst_common.h: Use tst_multiply_timeout in TST_RETRY_FN()

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Petr Vorel]

Hi,

...
> > I also wonder, if this check is needed, next step is
> > results->timeout = tst_multiply_timeout(timeout);
> > which does the same check.

> In shell we have the same check. And there it is more clear, as we
> refer to TST_TIMEOUT variable. Here both messages just say "timeout"
> but the linenumber would be more close to the actual call.
True, make sense to keep it.

Unless anyone has some objections, I'll merge it with changes bellow.

Kind regards,
Petr

diff --git lib/newlib_tests/shell/test_timeout_mul.sh lib/newlib_tests/shell/test_timeout_mul.sh
index 6682e5d66..a3abda043 100755
--- lib/newlib_tests/shell/test_timeout_mul.sh
+++ lib/newlib_tests/shell/test_timeout_mul.sh
@@ -5,7 +5,6 @@
 TST_TESTFUNC=do_test
 . tst_test.sh
 
-
 call_it()
 {
 	local SAVE_MUL=${LTP_TIMEOUT_MUL}
diff --git lib/tst_test.c lib/tst_test.c
index 5f43b1e0b..7cdf3c35a 100644
--- lib/tst_test.c
+++ lib/tst_test.c
@@ -1130,7 +1130,7 @@ void tst_set_timeout(int timeout)
 	}
 
 	if (timeout < 1)
-		tst_brk(TBROK, "timeout need to be >= 1! (%d)", timeout);
+		tst_brk(TBROK, "timeout must to be >= 1! (%d)", timeout);
 
 	results->timeout = tst_multiply_timeout(timeout);
 
diff --git testcases/lib/tst_test.sh testcases/lib/tst_test.sh
index 1cc5b42b8..7d0bf347e 100644
--- testcases/lib/tst_test.sh
+++ testcases/lib/tst_test.sh
@@ -190,10 +190,10 @@ TST_RETRY_FN_EXP_BACKOFF()
 {
 	local tst_fun="$1"
 	local tst_exp=$2
-	local tst_sec=$(( $3 * 1000000 ))
+	local tst_sec=$(($3 * 1000000))
 	local tst_delay=1
 
-	tst_multiply_timeout tst_sec
+	_tst_multiply_timeout tst_sec
 
 	if [ $# -ne 3 ]; then
 		tst_brk TBROK "TST_RETRY_FN_EXP_BACKOFF expects 3 parameters"
@@ -404,9 +404,9 @@ _tst_rescmp()
 	fi
 }
 
-tst_multiply_timeout()
+_tst_multiply_timeout()
 {
-	[ $# -ne 1 ] && tst_brk TBROK "tst_multiply_timeout expect 1 parameter"
+	[ $# -ne 1 ] && tst_brk TBROK "_tst_multiply_timeout expect 1 parameter"
 	eval "local timeout=\$$1"
 
 	LTP_TIMEOUT_MUL=${LTP_TIMEOUT_MUL:-1}
@@ -424,7 +424,7 @@ tst_multiply_timeout()
 	[ "$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 "$1='$((timeout * LTP_TIMEOUT_MUL))'"
 	return 0
 }
 
@@ -442,7 +442,7 @@ _tst_setup_timer()
 	fi
 
 	local sec=$TST_TIMEOUT
-	tst_multiply_timeout sec
+	_tst_multiply_timeout sec
 	local h=$((sec / 3600))
 	local m=$((sec / 60 % 60))
 	local s=$((sec % 60))

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Cyril Hrubis]

Hi!
> I think the function tst_multiply_timeout also needs documenting.

That depends on if we want to export it or not.

We may as well keep it in API and create a helper under testcases/lib/
tehn we could use the C code that can multiply precisely in the shell as
well. But I do not care that much either way...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Clemens Famulla-Conrad]

On Mon, 2019-10-21 at 16:37 +0200, Cyril Hrubis wrote:
> Hi!
> > +	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);
> > +			}
> 
> Huh, why are we ceiling the timeout multiplier?

Hm, I just understood the discussion about TST_TIMEOUT/LTP_TIMEOUT_MUL
in that way, that we tried to keep both implementations more or less
the same.

So we keep float for LTP_TIMEOUT_MUL in c implementation?
Maybe a v5 is then needed, pvorel?

thanks
Clemens

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Petr Vorel]

Hi,

> On Mon, 2019-10-21 at 16:37 +0200, Cyril Hrubis wrote:
> > Hi!
> > > +	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);
> > > +			}

> > Huh, why are we ceiling the timeout multiplier?
I didn't notice Cyril's comment.
I'm sorry to overlook this. I agree with Cyril to keep float for C.
> Hm, I just understood the discussion about TST_TIMEOUT/LTP_TIMEOUT_MUL
> in that way, that we tried to keep both implementations more or less
> the same.

> So we keep float for LTP_TIMEOUT_MUL in c implementation?
> Maybe a v5 is then needed, pvorel?
Yes please. Can you please document float vs int in LTP_TIMEOUT_MUL
in the last commit.

> thanks
> Clemens


Kind regards,
Petr

[LTP] [PATCH v4 5/5] Adopt doc for TST_RETRY_FUNC for LTP_TIMEOUT_MUL

[Petr Vorel]

> Hi!
> > I think the function tst_multiply_timeout also needs documenting.

> That depends on if we want to export it or not.

> We may as well keep it in API and create a helper under testcases/lib/
> tehn we could use the C code that can multiply precisely in the shell as
> well. But I do not care that much either way...
+1. I wouldn't export it unless it's needed.

Kind regards,
Petr

[LTP] [PATCH v4 4/5] Add newlib shell test for tst_multiply_timeout()

[Petr Vorel]

Hi,

> +do_test()
> +{
> +	LTP_TIMEOUT_MUL=2
> +	local sec=1
> +
> +	call_it 'tst_multiply_timeout sec' 'sec' 2
still some issue with this patchset. Making the function private
(_tst_multiply_timeout), we obviously get warning:
test_timeout_mul 1 TWARN: Private variable or function _tst_multiply_timeout used!

Unless we want to to add some special variable for library tests
(i.e. TST_IGNORE_WARN), the function cannot be used directly

I suggest to push the patchset change to use float (not to be postponed even
more), but without this patchset. Tests should be later added.

Kind regards,
Petr

> +	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=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

[LTP] [PATCH v4 4/5] Add newlib shell test for tst_multiply_timeout()

[Clemens Famulla-Conrad]

On Tue, 2019-10-22 at 10:08 +0200, Petr Vorel wrote:
> Hi,
> 
> > +do_test()
> > +{
> > +	LTP_TIMEOUT_MUL=2
> > +	local sec=1
> > +
> > +	call_it 'tst_multiply_timeout sec' 'sec' 2
> 
> still some issue with this patchset. Making the function private
> (_tst_multiply_timeout), we obviously get warning:
> test_timeout_mul 1 TWARN: Private variable or function
> _tst_multiply_timeout used!
> 
> Unless we want to to add some special variable for library tests
> (i.e. TST_IGNORE_WARN), the function cannot be used directly
> 
> I suggest to push the patchset change to use float (not to be
> postponed even
> more), but without this patchset. Tests should be later added.

+1

[LTP] [PATCH v4 2/5] tst_test.c: Add tst_multiply_timeout()

[Petr Vorel]

Hi,

merged with previously announced changes (keep float for C, add underscore to
tst_multiply_timeout function, fix some formatting, drop 4th commit).
Thanks all for patience.

Kind regards,
Petr