diff mbox series

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

Message ID 20191018124502.25599-3-cfamullaconrad@suse.de
State Accepted
Delegated to: Petr Vorel
Headers show
Series [v4,1/5] tst_test.sh: Use LTP_TIMEOUT_MUL in TST_RETRY_FN() | expand

Commit Message

Clemens Famulla-Conrad Oct. 18, 2019, 12:44 p.m. UTC 
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(-)

Comments

Petr Vorel Oct. 21, 2019, 12:50 p.m. UTC | #1 
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
Clemens Famulla-Conrad Oct. 21, 2019, 2:17 p.m. UTC | #2 
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
Cyril Hrubis Oct. 21, 2019, 2:37 p.m. UTC | #3 
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;
	}
Petr Vorel Oct. 21, 2019, 2:42 p.m. UTC | #4 
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))
Clemens Famulla-Conrad Oct. 21, 2019, 3:31 p.m. UTC | #5 
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
Petr Vorel Oct. 21, 2019, 6:08 p.m. UTC | #6 
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
Petr Vorel Oct. 22, 2019, 1:14 p.m. UTC | #7 
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
diff mbox series

Patch

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,