[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Cyril Hrubis]

There are testcases that take runtime as a parameter. These needs to be
able to set the timeout dynamically in the test setup().

This commit places the timeout value into the struct results stored in
shared memory so that it can be changed from the test setup() that runs
in the child process.

We also reset the alarm() right after we finish setup() so that the new
value is actually used.

+ Added a few lines about test timeouts into the documentation.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 doc/test-writing-guidelines.txt | 11 ++++++++++
 include/tst_test.h              |  2 ++
 lib/tst_test.c                  | 45 +++++++++++++++++++++++++----------------
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index db12d18..4990903 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -319,6 +319,10 @@ in range of [0, '.tcnt' - 1].
 
 IMPORTANT: Only one of '.test' and '.test_all' can be set at a time.
 
+Each test has a default timeout set to 300s. The default timeout can be
+overriden by setting '.timeout' in the test structure or by calling
+'tst_set_timeout()' in the test 'setup()'.
+
 A word about the cleanup() callback
 +++++++++++++++++++++++++++++++++++
 
@@ -438,6 +442,13 @@ Return the given errno number's corresponding string. Using this function to
 translate 'errno' values to strings is preferred. You should not use the
 'strerror()' function in the testcases.
 
+[source,c]
+-------------------------------------------------------------------------------
+void tst_set_timeout(unsigned int timeout);
+-------------------------------------------------------------------------------
+
+Allows for setting timeout per test iteration dymanically in the test setup(),
+the timeout is specified in seconds.
 
 2.2.3 Test temporary directory
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/include/tst_test.h b/include/tst_test.h
index c89e51f..c839178 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -133,6 +133,8 @@ const char *tst_strsig(int sig);
 
 static struct tst_test test;
 
+void tst_set_timeout(unsigned int timeout);
+
 int main(int argc, char *argv[])
 {
 	tst_run_tcases(argc, argv, &test);
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 26fff97..72d4696 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -46,6 +46,7 @@ struct results {
 	int skipped;
 	int failed;
 	int warnings;
+	unsigned int timeout;
 };
 
 static struct results *results;
@@ -635,6 +636,8 @@ static void testrun(void)
 
 	do_test_setup();
 
+	kill(getppid(), SIGUSR1);
+
 	if (duration > 0)
 		stop_time = get_time_ms() + (unsigned long long)(duration * 1000);
 
@@ -662,7 +665,6 @@ static void testrun(void)
 }
 
 static pid_t test_pid;
-static unsigned int timeout = 300;
 
 static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
 {
@@ -671,39 +673,48 @@ static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
 
 static void heartbeat_handler(int sig LTP_ATTRIBUTE_UNUSED)
 {
-	alarm(timeout);
+	alarm(results->timeout);
 }
 
-void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+void tst_set_timeout(unsigned int timeout)
 {
-	int status;
-	char *mul;
-
-	lib_pid = getpid();
-	tst_test = self;
-	TCID = tst_test->tid;
-
-	do_setup(argc, argv);
+	char *mul = getenv("LTP_TIMEOUT_MUL");
 
-	if (tst_test->timeout)
-		timeout = tst_test->timeout;
+	results->timeout = timeout;
 
-	mul = getenv("LTP_TIMEOUT_MUL");
 	if (mul) {
 		float m = atof(mul);
 
 		if (m < 1)
 			tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
 
-		timeout = timeout * m + 0.5;
+		results->timeout = results->timeout * m + 0.5;
 	}
 
-	tst_res(TINFO, "Timeout per run is %us", timeout);
+	tst_res(TINFO, "Timeout per run is %uh %02um %02us",
+		results->timeout/3600, (results->timeout%3600)/60,
+		results->timeout % 60);
+}
+
+void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+{
+	int status;
+
+	lib_pid = getpid();
+	tst_test = self;
+	TCID = tst_test->tid;
+
+	do_setup(argc, argv);
+
+	if (tst_test->timeout)
+		tst_set_timeout(tst_test->timeout);
+	else
+		tst_set_timeout(300);
 
 	SAFE_SIGNAL(SIGALRM, alarm_handler);
 	SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
 
-	alarm(timeout);
+	alarm(results->timeout);
 
 	test_pid = fork();
 	if (test_pid < 0)
-- 
2.7.3


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: ltp@lists.linux.it
> Cc: "Jan Stancek" <jstancek@redhat.com>
> Sent: Wednesday, 3 August, 2016 3:53:55 PM
> Subject: [PATCH] tst_test: Allow to set timeout from test setup()
> 
> There are testcases that take runtime as a parameter. These needs to be
> able to set the timeout dynamically in the test setup().
> 
> This commit places the timeout value into the struct results stored in
> shared memory so that it can be changed from the test setup() that runs
> in the child process.
> 
> We also reset the alarm() right after we finish setup() so that the new
> value is actually used.
> 
> + Added a few lines about test timeouts into the documentation.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  doc/test-writing-guidelines.txt | 11 ++++++++++
>  include/tst_test.h              |  2 ++
>  lib/tst_test.c                  | 45
>  +++++++++++++++++++++++++----------------
>  3 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/doc/test-writing-guidelines.txt
> b/doc/test-writing-guidelines.txt
> index db12d18..4990903 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -319,6 +319,10 @@ in range of [0, '.tcnt' - 1].
>  
>  IMPORTANT: Only one of '.test' and '.test_all' can be set at a time.
>  
> +Each test has a default timeout set to 300s. The default timeout can be
> +overriden by setting '.timeout' in the test structure or by calling
> +'tst_set_timeout()' in the test 'setup()'.
> +
>  A word about the cleanup() callback
>  +++++++++++++++++++++++++++++++++++
>  
> @@ -438,6 +442,13 @@ Return the given errno number's corresponding string.
> Using this function to
>  translate 'errno' values to strings is preferred. You should not use the
>  'strerror()' function in the testcases.
>  
> +[source,c]
> +-------------------------------------------------------------------------------
> +void tst_set_timeout(unsigned int timeout);
> +-------------------------------------------------------------------------------
> +
> +Allows for setting timeout per test iteration dymanically in the test
> setup(),
> +the timeout is specified in seconds.
>  
>  2.2.3 Test temporary directory
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/tst_test.h b/include/tst_test.h
> index c89e51f..c839178 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -133,6 +133,8 @@ const char *tst_strsig(int sig);
>  
>  static struct tst_test test;
>  
> +void tst_set_timeout(unsigned int timeout);
> +
>  int main(int argc, char *argv[])
>  {
>  	tst_run_tcases(argc, argv, &test);
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 26fff97..72d4696 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -46,6 +46,7 @@ struct results {
>  	int skipped;
>  	int failed;
>  	int warnings;
> +	unsigned int timeout;
>  };
>  
>  static struct results *results;
> @@ -635,6 +636,8 @@ static void testrun(void)
>  
>  	do_test_setup();
>  
> +	kill(getppid(), SIGUSR1);
> +
>  	if (duration > 0)
>  		stop_time = get_time_ms() + (unsigned long long)(duration * 1000);
>  
> @@ -662,7 +665,6 @@ static void testrun(void)
>  }
>  
>  static pid_t test_pid;
> -static unsigned int timeout = 300;
>  
>  static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
>  {
> @@ -671,39 +673,48 @@ static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
>  
>  static void heartbeat_handler(int sig LTP_ATTRIBUTE_UNUSED)
>  {
> -	alarm(timeout);
> +	alarm(results->timeout);
>  }
>  
> -void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> +void tst_set_timeout(unsigned int timeout)
>  {
> -	int status;
> -	char *mul;
> -
> -	lib_pid = getpid();
> -	tst_test = self;
> -	TCID = tst_test->tid;
> -
> -	do_setup(argc, argv);
> +	char *mul = getenv("LTP_TIMEOUT_MUL");
>  
> -	if (tst_test->timeout)
> -		timeout = tst_test->timeout;
> +	results->timeout = timeout;
>  
> -	mul = getenv("LTP_TIMEOUT_MUL");
>  	if (mul) {
>  		float m = atof(mul);
>  
>  		if (m < 1)
>  			tst_brk(TBROK, "Invalid timeout multiplier '%s'", mul);
>  
> -		timeout = timeout * m + 0.5;
> +		results->timeout = results->timeout * m + 0.5;
>  	}
>  
> -	tst_res(TINFO, "Timeout per run is %us", timeout);
> +	tst_res(TINFO, "Timeout per run is %uh %02um %02us",
> +		results->timeout/3600, (results->timeout%3600)/60,
> +		results->timeout % 60);
> +}
> +
> +void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> +{
> +	int status;
> +
> +	lib_pid = getpid();
> +	tst_test = self;
> +	TCID = tst_test->tid;
> +
> +	do_setup(argc, argv);
> +
> +	if (tst_test->timeout)
> +		tst_set_timeout(tst_test->timeout);
> +	else
> +		tst_set_timeout(300);
>  
>  	SAFE_SIGNAL(SIGALRM, alarm_handler);
>  	SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
>  
> -	alarm(timeout);
> +	alarm(results->timeout);
>  
>  	test_pid = fork();
>  	if (test_pid < 0)
> --
> 2.7.3
> 
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

Looks OK to me, but I'd make tst_set_timeout() do all the work
necessary, if we later change mind and allow to set timeout
in other places. So I'm thinking:
- allow tst_set_timeout() only from test pid
- tst_set_timeout will propagate updates via heartbeat
- timeout is set only once during startup

These are changes that would go on top of your patch (untested):

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 72d46969a51d..a5fcc087a6a5 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -636,7 +636,12 @@ static void testrun(void)
 
        do_test_setup();
 
-       kill(getppid(), SIGUSR1);
+       if (results->timeout)
+               ; /* setup called tst_set_timeout */
+       else if (tst_test->timeout)
+               tst_set_timeout(tst_test->timeout);
+       else
+               tst_set_timeout(300);
 
        if (duration > 0)
                stop_time = get_time_ms() + (unsigned long long)(duration * 1000);
@@ -680,6 +685,9 @@ void tst_set_timeout(unsigned int timeout)
 {
        char *mul = getenv("LTP_TIMEOUT_MUL");
 
+       if (getpid() == lib_pid)
+               tst_brk(TBROK, "tst_set_timeout called from lib pid");
+
        results->timeout = timeout;
 
        if (mul) {
@@ -694,6 +702,7 @@ void tst_set_timeout(unsigned int timeout)
        tst_res(TINFO, "Timeout per run is %uh %02um %02us",
                results->timeout/3600, (results->timeout%3600)/60,
                results->timeout % 60);
+       kill(getppid(), SIGUSR1);
 }
 
 void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
@@ -706,16 +715,9 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 
        do_setup(argc, argv);
 
-       if (tst_test->timeout)
-               tst_set_timeout(tst_test->timeout);
-       else
-               tst_set_timeout(300);
-
        SAFE_SIGNAL(SIGALRM, alarm_handler);
        SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
 
-       alarm(results->timeout);
-
        test_pid = fork();
        if (test_pid < 0)
                tst_brk(TBROK | TERRNO, "fork()");

Regards,
Jan

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Cyril Hrubis]

Hi!
>  void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> @@ -706,16 +715,9 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
>  
>         do_setup(argc, argv);
>  
> -       if (tst_test->timeout)
> -               tst_set_timeout(tst_test->timeout);
> -       else
> -               tst_set_timeout(300);
> -
>         SAFE_SIGNAL(SIGALRM, alarm_handler);
>         SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
>  
> -       alarm(results->timeout);
> -

After this change the tst_test->setup() runs without a timeout, doesn't it?

I think that it will be safer to have it run with either with a default
timeout or with a test->timeout. And that is the whole point of setting
the timeout twice in case that we call tst_set_timeout() in the test
setup().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 3 August, 2016 5:06:55 PM
> Subject: Re: [PATCH] tst_test: Allow to set timeout from test setup()
> 
> Hi!
> >  void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> > @@ -706,16 +715,9 @@ void tst_run_tcases(int argc, char *argv[], struct
> > tst_test *self)
> >  
> >         do_setup(argc, argv);
> >  
> > -       if (tst_test->timeout)
> > -               tst_set_timeout(tst_test->timeout);
> > -       else
> > -               tst_set_timeout(300);
> > -
> >         SAFE_SIGNAL(SIGALRM, alarm_handler);
> >         SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
> >  
> > -       alarm(results->timeout);
> > -
> 
> After this change the tst_test->setup() runs without a timeout, doesn't it?
> 
> I think that it will be safer to have it run with either with a default
> timeout or with a test->timeout. And that is the whole point of setting
> the timeout twice in case that we call tst_set_timeout() in the test
> setup().

Good point, so setting timeout twice seems unavoidable.
But I'm still thinking about actions needed to set timeout
that are now not part of tst_set_timeout(). Would it make sense
to bring those in?


diff --git a/lib/tst_test.c b/lib/tst_test.c
index 72d46969a51d..9eb1393415fa 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -636,8 +636,6 @@ static void testrun(void)

        do_test_setup();

-       kill(getppid(), SIGUSR1);
-
        if (duration > 0)
                stop_time = get_time_ms() + (unsigned long long)(duration * 1000);

@@ -694,6 +692,11 @@ void tst_set_timeout(unsigned int timeout)
        tst_res(TINFO, "Timeout per run is %uh %02um %02us",
                results->timeout/3600, (results->timeout%3600)/60,
                results->timeout % 60);
+
+       if (getpid() == lib_pid)
+               alarm(results->timeout);
+       else
+               kill(getppid(), SIGUSR1);
 }

 void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
@@ -706,16 +709,14 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)

        do_setup(argc, argv);

+       SAFE_SIGNAL(SIGALRM, alarm_handler);
+       SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
+
        if (tst_test->timeout)
                tst_set_timeout(tst_test->timeout);
        else
                tst_set_timeout(300);

-       SAFE_SIGNAL(SIGALRM, alarm_handler);
-       SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
-
-       alarm(results->timeout);
-
        test_pid = fork();
        if (test_pid < 0)
                tst_brk(TBROK | TERRNO, "fork()");

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Cyril Hrubis]

Hi!
> > I think that it will be safer to have it run with either with a default
> > timeout or with a test->timeout. And that is the whole point of setting
> > the timeout twice in case that we call tst_set_timeout() in the test
> > setup().
> 
> Good point, so setting timeout twice seems unavoidable.
> But I'm still thinking about actions needed to set timeout
> that are now not part of tst_set_timeout(). Would it make sense
> to bring those in?

Looks good to me. I will merge your changes into my patch, add your
Signed-off-by and push it, OK?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_test: Allow to set timeout from test setup()

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 3 August, 2016 5:34:19 PM
> Subject: Re: [PATCH] tst_test: Allow to set timeout from test setup()
> 
> Hi!
> > > I think that it will be safer to have it run with either with a default
> > > timeout or with a test->timeout. And that is the whole point of setting
> > > the timeout twice in case that we call tst_set_timeout() in the test
> > > setup().
> > 
> > Good point, so setting timeout twice seems unavoidable.
> > But I'm still thinking about actions needed to set timeout
> > that are now not part of tst_set_timeout(). Would it make sense
> > to bring those in?
> 
> Looks good to me. I will merge your changes into my patch, add your
> Signed-off-by and push it, OK?

Fine by me.

Regards,
Jan

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>