lib/tst_test.c: Run test in child process

From: "Cyril Hrubis" <[email protected]>
To: [email protected]
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.

lib/tst_test.c +5 -1

index b8ec246..eef54e4 100644
@@ -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)

lib/tst_test.c +0 -0

 	exit(TTYPE_RESULT(ttype));

}

lib/tst_test.c +26 -8

@@ -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)
+{

Quoted 21 lines

lib/tst_test.c +5 -1

index b8ec246..eef54e4 100644
@@ -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);
 

lib/tst_test.c void tst_vres_(const char *file, const int lineno, int ttype, · line 236

Quoted 7 lines

lib/tst_test.c +2 -4

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

Quoted 6 lines

lib/tst_test.c +6 -0

+void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+{
+	int status;
+	char *mul;
+
+	tst_test = self;

lib/tst_test.c void tst_run_tcases(int argc, char *argv[], struct tst_test *self) · line 682

Quoted 6 lines

lib/tst_test.c +6 -0

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

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Wednesday, 8 June, 2016 2:30:56 PM
Subject: Re: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process

Hi!

Quoted 22 lines

Quoted 22 lines

lib/tst_test.c +0 -0

index b8ec246..eef54e4 100644
@@ -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));

lib/tst_test.c +5 -1

-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);

Quoted 10 lines

lib/tst_test.c

Quoted 7 lines

lib/tst_test.c +2 -4

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

Quoted 6 lines

Quoted 6 lines

lib/tst_test.c +6 -0

+void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
+{
+	int status;
+	char *mul;
+
+	tst_test = self;

Quoted 8 lines

lib/tst_test.c

Quoted 6 lines

lib/tst_test.c +6 -0

+	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
[email protected]

Quoted 2 lines

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

Quoted 3 lines

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:

lib/tst_test.c +4 -1

index eef54e49bafb..1a0a9494af26 100644
@@ -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));
 }
 

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Wednesday, 8 June, 2016 3:15:15 PM
Subject: Re: [LTP] [PATCH v2] lib/tst_test.c: Run test in child process

Hi!

Quoted 14 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

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_().

...

Quoted 1 line

Looks good to me, acked.

Pushed.

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
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.

What about:

lib/tst_test.c +2 -2

index f7485bd..6e6d417 100644
@@ -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));

}

lib/tst_test.c +2 -4

@@ -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) {

lib/tst_test.c +1 -1

 		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
[email protected]

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