[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

This commit moves the actuall test to a child process. The parent process now
serves two purposes. It sets up an alarm timer that kills the test on a timeout
and also does the library setup and cleanup which means that the test temporary
directory is removed even if the test crashed in one of the functions exported
in tst_test structure.

The timeout is defined per test run, which is either one execution of
test_all() function or single loop over the test() function. The timeout is
reset after each test run by the test library by sending SIGUSR1 to the parent.

The default timeout is set to 300 seconds and can be overriden by setting
tst_test->timeout variable.

There is also LTP_TIMEOUT_MUL env variable that, if set to float > 1, is used
to multiply the timeout before the start of a test. This is especially intended
for slow machines where the default timeout may not be enough. In that
case you can double all timeouts just by exporting LTP_TIMEOUT_MUL=2
before starting the testrun.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_test.h          |  3 ++
 lib/newlib_tests/.gitignore |  3 ++
 lib/newlib_tests/test10.c   | 33 +++++++++++++++
 lib/newlib_tests/test11.c   | 34 ++++++++++++++++
 lib/newlib_tests/test12.c   | 32 +++++++++++++++
 lib/tst_test.c              | 98 +++++++++++++++++++++++++++++++++++++++------
 6 files changed, 190 insertions(+), 13 deletions(-)
 create mode 100644 lib/newlib_tests/test10.c
 create mode 100644 lib/newlib_tests/test11.c
 create mode 100644 lib/newlib_tests/test12.c

diff --git a/include/tst_test.h b/include/tst_test.h
index 88b46d6..ddd9060 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -84,6 +84,9 @@ struct tst_test {
 	int needs_device:1;
 	int needs_checkpoints:1;
 
+	/* override default timeout per test run */
+	unsigned int timeout;
+
 	void (*setup)(void);
 	void (*cleanup)(void);
 
diff --git a/lib/newlib_tests/.gitignore b/lib/newlib_tests/.gitignore
index e3ec6aa..dccf4c8 100644
--- a/lib/newlib_tests/.gitignore
+++ b/lib/newlib_tests/.gitignore
@@ -7,3 +7,6 @@ test06
 test07
 test08
 test09
+test10
+test11
+test12
diff --git a/lib/newlib_tests/test10.c b/lib/newlib_tests/test10.c
new file mode 100644
index 0000000..01d4807
--- /dev/null
+++ b/lib/newlib_tests/test10.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/*
+ * Test for watchdog timeout.
+ */
+
+#include "tst_test.h"
+
+
+static void do_test(void)
+{
+	sleep(2);
+	tst_res(TPASS, "Not reached");
+}
+
+static struct tst_test test = {
+	.tid = "test10",
+	.test_all = do_test,
+	.timeout = 1,
+};
diff --git a/lib/newlib_tests/test11.c b/lib/newlib_tests/test11.c
new file mode 100644
index 0000000..1354f52
--- /dev/null
+++ b/lib/newlib_tests/test11.c
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/*
+ * Test for segfault.
+ */
+
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	volatile char *ptr = NULL;
+
+	*ptr = 0;
+
+	tst_res(TPASS, "Not reached");
+}
+
+static struct tst_test test = {
+	.tid = "test11",
+	.test_all = do_test,
+};
diff --git a/lib/newlib_tests/test12.c b/lib/newlib_tests/test12.c
new file mode 100644
index 0000000..c7901f4
--- /dev/null
+++ b/lib/newlib_tests/test12.c
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2016 Linux Test Project
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/*
+ * Test for timeout override.
+ */
+
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	sleep(1);
+	tst_res(TPASS, "Passed!");
+}
+
+static struct tst_test test = {
+	.tid = "test12",
+	.timeout = 2,
+	.test_all = do_test,
+};
diff --git a/lib/tst_test.c b/lib/tst_test.c
index b8ec246..eef54e4 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -220,17 +220,19 @@ void tst_vres_(const char *file, const int lineno, int ttype,
 void tst_vbrk_(const char *file, const int lineno, int ttype,
                const char *fmt, va_list va) __attribute__((noreturn));
 
-static void do_cleanup(void);
+static void do_test_cleanup(void)
+{
+	if (tst_test->cleanup)
+		tst_test->cleanup();
+}
 
 void tst_vbrk_(const char *file, const int lineno, int ttype,
                const char *fmt, va_list va)
 {
 	print_result(file, lineno, ttype, fmt, va);
 
-	if (getpid() == main_pid) {
-		do_cleanup();
-		cleanup_ipc();
-	}
+	if (getpid() == main_pid)
+		do_test_cleanup();
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -550,7 +552,10 @@ static void do_setup(int argc, char *argv[])
 
 	if (tst_test->resource_files)
 		copy_resources();
+}
 
+static void do_test_setup(void)
+{
 	main_pid = getpid();
 
 	if (tst_test->setup)
@@ -562,9 +567,6 @@ static void do_setup(int argc, char *argv[])
 
 static void do_cleanup(void)
 {
-	if (tst_test->cleanup)
-		tst_test->cleanup();
-
 	if (tst_test->needs_device && tdev.dev)
 		tst_release_device(tdev.dev);
 
@@ -619,16 +621,13 @@ static unsigned long long get_time_ms(void)
 	return tv.tv_sec * 1000 + tv.tv_usec / 1000;
 }
 
-void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+static void testrun(void)
 {
 	unsigned int i = 0;
 	unsigned long long stop_time = 0;
 	int cont = 1;
 
-	tst_test = self;
-	TCID = tst_test->tid;
-
-	do_setup(argc, argv);
+	do_test_setup();
 
 	if (duration > 0)
 		stop_time = get_time_ms() + (unsigned long long)(duration * 1000);
@@ -648,8 +647,81 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 			break;
 
 		run_tests();
+
+		kill(getppid(), SIGUSR1);
 	}
 
+	do_test_cleanup();
+	exit(0);
+}
+
+static pid_t test_pid;
+static unsigned int timeout = 300;
+
+static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
+{
+	kill(test_pid, SIGKILL);
+}
+
+static void heartbeat_handler(int sig LTP_ATTRIBUTE_UNUSED)
+{
+	alarm(timeout);
+}
+
+void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+{
+	int status;
+	char *mul;
+
+	tst_test = self;
+	TCID = tst_test->tid;
+
+	do_setup(argc, argv);
+
+	if (tst_test->timeout)
+		timeout = tst_test->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;
+	}
+
+	tst_res(TINFO, "Timeout per run is %us", timeout);
+
+	SAFE_SIGNAL(SIGALRM, alarm_handler);
+	SAFE_SIGNAL(SIGUSR1, heartbeat_handler);
+
+	alarm(timeout);
+
+	test_pid = fork();
+	if (test_pid < 0)
+		tst_brk(TBROK | TERRNO, "fork()");
+
+	if (!test_pid)
+		testrun();
+
+	SAFE_WAITPID(test_pid, &status, 0);
+
+	alarm(0);
+
 	do_cleanup();
+
+	if (WIFEXITED(status) && WEXITSTATUS(status))
+		exit(WEXITSTATUS(status));
+
+	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
+		tst_res(TINFO, "If you are running on slow machine, "
+			       "try exporting LTP_TIMEOUT_MUL > 1");
+		tst_brk(TBROK, "Test killed! (timeout?)");
+	}
+
+	if (WIFSIGNALED(status))
+		tst_brk(TBROK, "Test killed by %s!", tst_strsig(WTERMSIG(status)));
+
 	do_exit();
 }
-- 
2.7.3


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: ltp@lists.linux.it
> Sent: Tuesday, 7 June, 2016 1:58:07 PM
> Subject: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process
> 
> This commit moves the actuall test to a child process. The parent process now
> serves two purposes. It sets up an alarm timer that kills the test on a
> timeout
> and also does the library setup and cleanup which means that the test
> temporary
> directory is removed even if the test crashed in one of the functions
> exported
> in tst_test structure.
> 
> The timeout is defined per test run, which is either one execution of
> test_all() function or single loop over the test() function. The timeout is
> reset after each test run by the test library by sending SIGUSR1 to the
> parent.
> 
> The default timeout is set to 300 seconds and can be overriden by setting
> tst_test->timeout variable.
> 
> There is also LTP_TIMEOUT_MUL env variable that, if set to float > 1, is used
> to multiply the timeout before the start of a test. This is especially
> intended
> for slow machines where the default timeout may not be enough. In that
> case you can double all timeouts just by exporting LTP_TIMEOUT_MUL=2
> before starting the testrun.

Looks good to me, ACK. Couple notes below.

<snip>

> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index b8ec246..eef54e4 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -220,17 +220,19 @@ void tst_vres_(const char *file, const int lineno, int
> ttype,
>  void tst_vbrk_(const char *file, const int lineno, int ttype,
>                 const char *fmt, va_list va) __attribute__((noreturn));
>  
> -static void do_cleanup(void);
> +static void do_test_cleanup(void)
> +{
> +	if (tst_test->cleanup)
> +		tst_test->cleanup();
> +}
>  
>  void tst_vbrk_(const char *file, const int lineno, int ttype,
>                 const char *fmt, va_list va)
>  {
>  	print_result(file, lineno, ttype, fmt, va);
>  
> -	if (getpid() == main_pid) {
> -		do_cleanup();
> -		cleanup_ipc();
> -	}
> +	if (getpid() == main_pid)
> +		do_test_cleanup();
>  

Not directly related to this patch, but I noticed that we don't
seem to cleanup_ipc if we hit TBROK outside of main test pid.

>  	exit(TTYPE_RESULT(ttype));
>  }
> @@ -550,7 +552,10 @@ static void do_setup(int argc, char *argv[])
>  
>  	if (tst_test->resource_files)
>  		copy_resources();
> +}
>  
> +static void do_test_setup(void)
> +{
>  	main_pid = getpid();
>  
>  	if (tst_test->setup)
> @@ -562,9 +567,6 @@ static void do_setup(int argc, char *argv[])
>  
>  static void do_cleanup(void)
>  {
> -	if (tst_test->cleanup)
> -		tst_test->cleanup();
> -
>  	if (tst_test->needs_device && tdev.dev)
>  		tst_release_device(tdev.dev);
>  
> @@ -619,16 +621,13 @@ static unsigned long long get_time_ms(void)
>  	return tv.tv_sec * 1000 + tv.tv_usec / 1000;
>  }
>  
> -void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> +static void testrun(void)
>  {
>  	unsigned int i = 0;
>  	unsigned long long stop_time = 0;
>  	int cont = 1;
>  
> -	tst_test = self;
> -	TCID = tst_test->tid;
> -
> -	do_setup(argc, argv);
> +	do_test_setup();
>  
>  	if (duration > 0)
>  		stop_time = get_time_ms() + (unsigned long long)(duration * 1000);
> @@ -648,8 +647,81 @@ void tst_run_tcases(int argc, char *argv[], struct
> tst_test *self)
>  			break;
>  
>  		run_tests();
> +
> +		kill(getppid(), SIGUSR1);
>  	}
>  
> +	do_test_cleanup();
> +	exit(0);
> +}
> +
> +static pid_t test_pid;
> +static unsigned int timeout = 300;
> +
> +static void alarm_handler(int sig LTP_ATTRIBUTE_UNUSED)
> +{
> +	kill(test_pid, SIGKILL);
> +}
> +
> +static void heartbeat_handler(int sig LTP_ATTRIBUTE_UNUSED)
> +{
> +	alarm(timeout);
> +}
> +
> +void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> +{
> +	int status;
> +	char *mul;
> +
> +	tst_test = self;
> +	TCID = tst_test->tid;
> +
> +	do_setup(argc, argv);
> +
> +	if (tst_test->timeout)
> +		timeout = tst_test->timeout;

Can you think of a testcase where we would want to disable timeout?

Regards,
Jan

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

Hi!
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index b8ec246..eef54e4 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -220,17 +220,19 @@ void tst_vres_(const char *file, const int lineno, int
> > ttype,
> >  void tst_vbrk_(const char *file, const int lineno, int ttype,
> >                 const char *fmt, va_list va) __attribute__((noreturn));
> >  
> > -static void do_cleanup(void);
> > +static void do_test_cleanup(void)
> > +{
> > +	if (tst_test->cleanup)
> > +		tst_test->cleanup();
> > +}
> >  
> >  void tst_vbrk_(const char *file, const int lineno, int ttype,
> >                 const char *fmt, va_list va)
> >  {
> >  	print_result(file, lineno, ttype, fmt, va);
> >  
> > -	if (getpid() == main_pid) {
> > -		do_cleanup();
> > -		cleanup_ipc();
> > -	}
> > +	if (getpid() == main_pid)
> > +		do_test_cleanup();
> >  
> 
> Not directly related to this patch, but I noticed that we don't
> seem to cleanup_ipc if we hit TBROK outside of main test pid.

The cleanup_ipc() is intended to be executed just before the main pid
exits since it unlinks the shm file. If child TBROKs it's catched in the
check_child_status() in the parent, tst_brk() is called which will call
the cleanup_ipc().

I guess that we may close and unmap the shm even in the children but
wouldn't that happen anyway as the process exits?

> > +void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> > +{
> > +	int status;
> > +	char *mul;
> > +
> > +	tst_test = self;
> > +	TCID = tst_test->tid;
> > +
> > +	do_setup(argc, argv);
> > +
> > +	if (tst_test->timeout)
> > +		timeout = tst_test->timeout;
> 
> Can you think of a testcase where we would want to disable timeout?

At the moment I do not remember a test that would need timeout to be
turned off. And if we find that it's necessary we can always disable in
case that tst_test->timeout < 0.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 8 June, 2016 2:30:56 PM
> Subject: Re: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process
> 
> Hi!
> > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > index b8ec246..eef54e4 100644
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > @@ -220,17 +220,19 @@ void tst_vres_(const char *file, const int lineno,
> > > int
> > > ttype,
> > >  void tst_vbrk_(const char *file, const int lineno, int ttype,
> > >                 const char *fmt, va_list va) __attribute__((noreturn));
> > >  
> > > -static void do_cleanup(void);
> > > +static void do_test_cleanup(void)
> > > +{
> > > +	if (tst_test->cleanup)
> > > +		tst_test->cleanup();
> > > +}
> > >  
> > >  void tst_vbrk_(const char *file, const int lineno, int ttype,
> > >                 const char *fmt, va_list va)
> > >  {
> > >  	print_result(file, lineno, ttype, fmt, va);
> > >  
> > > -	if (getpid() == main_pid) {
> > > -		do_cleanup();
> > > -		cleanup_ipc();
> > > -	}
> > > +	if (getpid() == main_pid)
> > > +		do_test_cleanup();
> > >  
> > 
> > Not directly related to this patch, but I noticed that we don't
> > seem to cleanup_ipc if we hit TBROK outside of main test pid.
> 
> The cleanup_ipc() is intended to be executed just before the main pid
> exits since it unlinks the shm file.

So, if we fail a SAFE macro somewhere in lib/tst_test.c,
I don't see we ever call cleanup_ipc().

tst_run_tcases
 SAFE_WAITPID(test_pid, &status, 0);
  tst_brkm
   tst_brkm_
    tst_brk_
     tst_vbrk_
      exit


> If child TBROKs it's catched in the
> check_child_status() in the parent, tst_brk() is called which will call
> the cleanup_ipc().

Not anymore. After this patch the only place that now calls
cleanup_ipc() is do_exit().

What I'm thinking is something like this on top of your patch:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index eef54e49bafb..1a0a9494af26 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -40,7 +40,7 @@ struct tst_test *tst_test;
 static char tmpdir_created;
 static int iterations = 1;
 static float duration = -1;
-static pid_t main_pid;
+static pid_t main_pid, lib_pid;
 
 struct results {
        int passed;
@@ -234,6 +234,9 @@ void tst_vbrk_(const char *file, const int lineno, int ttype,
        if (getpid() == main_pid)
                do_test_cleanup();
 
+       if (getpid() == lib_pid)
+               do_exit();
+
        exit(TTYPE_RESULT(ttype));
 }
 
@@ -673,6 +676,7 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
        int status;
        char *mul;
 
+       lib_pid = getpid();
        tst_test = self;
        TCID = tst_test->tid;
 


> 
> I guess that we may close and unmap the shm even in the children but
> wouldn't that happen anyway as the process exits?
> 
> > > +void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> > > +{
> > > +	int status;
> > > +	char *mul;
> > > +
> > > +	tst_test = self;
> > > +	TCID = tst_test->tid;
> > > +
> > > +	do_setup(argc, argv);
> > > +
> > > +	if (tst_test->timeout)
> > > +		timeout = tst_test->timeout;
> > 
> > Can you think of a testcase where we would want to disable timeout?
> 
> At the moment I do not remember a test that would need timeout to be
> turned off. And if we find that it's necessary we can always disable in
> case that tst_test->timeout < 0.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

Hi!
> > The cleanup_ipc() is intended to be executed just before the main pid
> > exits since it unlinks the shm file.
> 
> So, if we fail a SAFE macro somewhere in lib/tst_test.c,
> I don't see we ever call cleanup_ipc().
> 
> tst_run_tcases
>  SAFE_WAITPID(test_pid, &status, 0);
>   tst_brkm
>    tst_brkm_
>     tst_brk_
>      tst_vbrk_
>       exit
> 
> > If child TBROKs it's catched in the
> > check_child_status() in the parent, tst_brk() is called which will call
> > the cleanup_ipc().
> 
> Not anymore. After this patch the only place that now calls
> cleanup_ipc() is do_exit().

Ah, right, that is since what was previously main_pid has been split
into the library process and main test process.

Indeed the cleanup_ipc() should be called from the library in the
tst_vbrk_().

> What I'm thinking is something like this on top of your patch:
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index eef54e49bafb..1a0a9494af26 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -40,7 +40,7 @@ struct tst_test *tst_test;
>  static char tmpdir_created;
>  static int iterations = 1;
>  static float duration = -1;
> -static pid_t main_pid;
> +static pid_t main_pid, lib_pid;
>  
>  struct results {
>         int passed;
> @@ -234,6 +234,9 @@ void tst_vbrk_(const char *file, const int lineno, int ttype,
>         if (getpid() == main_pid)
>                 do_test_cleanup();
>  
> +       if (getpid() == lib_pid)
> +               do_exit();
> +
>         exit(TTYPE_RESULT(ttype));
>  }
>  
> @@ -673,6 +676,7 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
>         int status;
>         char *mul;
>  
> +       lib_pid = getpid();
>         tst_test = self;
>         TCID = tst_test->tid;

Looks good to me, acked.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 8 June, 2016 3:15:15 PM
> Subject: Re: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process
> 
> Hi!
> > > The cleanup_ipc() is intended to be executed just before the main pid
> > > exits since it unlinks the shm file.
> > 
> > So, if we fail a SAFE macro somewhere in lib/tst_test.c,
> > I don't see we ever call cleanup_ipc().
> > 
> > tst_run_tcases
> >  SAFE_WAITPID(test_pid, &status, 0);
> >   tst_brkm
> >    tst_brkm_
> >     tst_brk_
> >      tst_vbrk_
> >       exit
> > 
> > > If child TBROKs it's catched in the
> > > check_child_status() in the parent, tst_brk() is called which will call
> > > the cleanup_ipc().
> > 
> > Not anymore. After this patch the only place that now calls
> > cleanup_ipc() is do_exit().
> 
> Ah, right, that is since what was previously main_pid has been split
> into the library process and main test process.
> 
> Indeed the cleanup_ipc() should be called from the library in the
> tst_vbrk_().
...
> Looks good to me, acked.

Pushed.

Regards,
Jan

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

Hi!
> > Ah, right, that is since what was previously main_pid has been split
> > into the library process and main test process.
> > 
> > Indeed the cleanup_ipc() should be called from the library in the
> > tst_vbrk_().
> ...
> > Looks good to me, acked.
> 
> Pushed.

And looking at the code closely, this is not enough, since we have:

        if (WIFEXITED(status) && WEXITSTATUS(status))
		exit(WEXITSTATUS(status));

in the tst_run_tcases(). So we have to call cleanup_ipc() before the
exit() here as well.


Also we do not count the TBROK in results structure, since the TBROK is
propagated only by exit value so do_exit() will exit with 0 in case that
we TBROK in the test library.

This is getting more complicated than I would like.

Minimal patch to fix this would be:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index f7485bd..67d778e 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -237,7 +237,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype,
 		do_test_cleanup();
 
 	if (getpid() == lib_pid)
-		do_exit();
+		cleanup_ipc();
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -718,8 +718,10 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 
 	do_cleanup();
 
-	if (WIFEXITED(status) && WEXITSTATUS(status))
+	if (WIFEXITED(status) && WEXITSTATUS(status)) {\
+		cleanup_ipc();
 		exit(WEXITSTATUS(status));
+	}
 
 	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
 		tst_res(TINFO, "If you are running on slow machine, "



Or we may change do_exit() to take parameter so that we can actually
pass the TBROK to it, in a case that you want to print the statistics in
this case as well...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

Hi!
And I guess that the best solution is to pass the exit value to
do_exit(), that way we have only one function that calls the exit() in
the code called by the test library process.

What about:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index f7485bd..6e6d417 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -64,7 +64,7 @@ char *const tst_ipc_envp[] = {ipc_path, NULL};
 
 static char shm_path[1024];
 
-static void do_exit(void) __attribute__ ((noreturn));
+static void do_exit(int ret) __attribute__ ((noreturn));
 
 static void setup_ipc(void)
 {
@@ -237,7 +237,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype,
 		do_test_cleanup();
 
 	if (getpid() == lib_pid)
-		do_exit();
+		do_exit(TTYPE_RESULT(ttype));
 
 	exit(TTYPE_RESULT(ttype));
 }
@@ -443,10 +443,8 @@ static void parse_opts(int argc, char *argv[])
 }
 
 
-static void do_exit(void)
+static void do_exit(int ret)
 {
-	int ret = 0;
-
 	printf("\nSummary:\n");
 	printf("passed   %d\n", results->passed);
 	printf("failed   %d\n", results->failed);
@@ -719,7 +717,7 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 	do_cleanup();
 
 	if (WIFEXITED(status) && WEXITSTATUS(status))
-		exit(WEXITSTATUS(status));
+		do_exit(WEXITSTATUS(status));
 
 	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
 		tst_res(TINFO, "If you are running on slow machine, "
@@ -730,5 +728,5 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 	if (WIFSIGNALED(status))
 		tst_brk(TBROK, "Test killed by %s!", tst_strsig(WTERMSIG(status)));
 
-	do_exit();
+	do_exit(0);
 }


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 8 June, 2016 4:06:35 PM
> Subject: Re: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process
> 
> Hi!
> And I guess that the best solution is to pass the exit value to
> do_exit(), that way we have only one function that calls the exit() in
> the code called by the test library process.

I was thinking the same as your patch below, but plus these changes:
- remove call to do_cleanup in tst_run_tcases
- add call to cleanup_ipc in do_cleanup
- remove call to cleanup_ipc in do_exit
- add call to do_cleanup in do_exit

> 
> What about:
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index f7485bd..6e6d417 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -64,7 +64,7 @@ char *const tst_ipc_envp[] = {ipc_path, NULL};
>  
>  static char shm_path[1024];
>  
> -static void do_exit(void) __attribute__ ((noreturn));
> +static void do_exit(int ret) __attribute__ ((noreturn));
>  
>  static void setup_ipc(void)
>  {
> @@ -237,7 +237,7 @@ void tst_vbrk_(const char *file, const int lineno, int
> ttype,
>  		do_test_cleanup();
>  
>  	if (getpid() == lib_pid)
> -		do_exit();
> +		do_exit(TTYPE_RESULT(ttype));
>  
>  	exit(TTYPE_RESULT(ttype));
>  }
> @@ -443,10 +443,8 @@ static void parse_opts(int argc, char *argv[])
>  }
>  
>  
> -static void do_exit(void)
> +static void do_exit(int ret)
>  {
> -	int ret = 0;
> -
>  	printf("\nSummary:\n");
>  	printf("passed   %d\n", results->passed);
>  	printf("failed   %d\n", results->failed);
> @@ -719,7 +717,7 @@ void tst_run_tcases(int argc, char *argv[], struct
> tst_test *self)
>  	do_cleanup();
>  
>  	if (WIFEXITED(status) && WEXITSTATUS(status))
> -		exit(WEXITSTATUS(status));
> +		do_exit(WEXITSTATUS(status));
>  
>  	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) {
>  		tst_res(TINFO, "If you are running on slow machine, "
> @@ -730,5 +728,5 @@ void tst_run_tcases(int argc, char *argv[], struct
> tst_test *self)
>  	if (WIFSIGNALED(status))
>  		tst_brk(TBROK, "Test killed by %s!", tst_strsig(WTERMSIG(status)));
>  
> -	do_exit();
> +	do_exit(0);
>  }
> 
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH v2] lib/tst_test.c: Run test in child process

[Cyril Hrubis]

Hi!
> I was thinking the same as your patch below, but plus these changes:
> - remove call to do_cleanup in tst_run_tcases
> - add call to cleanup_ipc in do_cleanup
> - remove call to cleanup_ipc in do_exit
> - add call to do_cleanup in do_exit

So, the test library could exit only via do_exit() call and the whole
cleanup would be done in do_exit() just before the call to exit(),
right? Sounds good to me. Will you prepare a patch?

-- 
Cyril Hrubis
chrubis@suse.cz