Skip to contentSkip to conversation

tst_test: Propagate SIGINT to test process

PATCH New Cyril Hrubis 3 participants 7 messages Aug 4, 2016 – Aug 16, 2016 Library code

lore
Share & tools
lore

Cyril Hrubis

author
#0 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

Since the test runs in separate process and separate process group pressing Ctrl+C only sends SIGINT to the library process.

This commit adds SIGINT handler for the library process that kills the whole test process group. The upside is that system resources allocated in the library are cleaned up even in this case.

We also reset signal handlers in the test process after the fork now.

Signed-off-by Cyril Hrubis <[email protected]>
---
 lib/tst_test.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
lib/tst_test.c +16 -1
index 1bd2619..cd5b049 100644
@@ -724,6 +724,16 @@ static void heartbeat_handler(int sig LTP_ATTRIBUTE_UNUSED)
 	alarm(results->timeout);
 }
 
+#define SIGINT_MSG "Sending SIGKILL to test process...\n"
+
+static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
+{
+	if (test_pid > 0) {
+		(void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
+		kill(-test_pid, SIGKILL);
+	}
+}
+
 void tst_set_timeout(unsigned int timeout)
 {
 	char *mul = getenv("LTP_TIMEOUT_MUL");

@@ -767,18 +777,23 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 	else
 		tst_set_timeout(300);
 
+	SAFE_SIGNAL(SIGINT, sigint_handler);
+
 	test_pid = fork();
 	if (test_pid < 0)
 		tst_brk(TBROK | TERRNO, "fork()");
 
 	if (!test_pid) {
+		SAFE_SIGNAL(SIGALRM, SIG_DFL);
+		SAFE_SIGNAL(SIGUSR1, SIG_DFL);
+		SAFE_SIGNAL(SIGINT, SIG_DFL);
 		SAFE_SETPGID(0, 0);
 		testrun();
 	}
 
 	SAFE_WAITPID(test_pid, &status, 0);
-
 	alarm(0);
+	SAFE_SIGNAL(SIGINT, SIG_DFL);
 
 	if (WIFEXITED(status) && WEXITSTATUS(status))
 		do_exit(WEXITSTATUS(status));
Signature · 6 lines
2.7.3


-- 
Cyril Hrubis
[email protected]
#1 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

----- Original Message -----

Quoted 25 lines

From: "Cyril Hrubis" <[email protected]>
To: [email protected]
Cc: "Jan Stancek" <[email protected]>
Sent: Thursday, 4 August, 2016 4:45:08 PM
Subject: [PATCH] tst_test: Propagate SIGINT to test process

Since the test runs in separate process and separate process group pressing Ctrl+C only sends SIGINT to the library process.

This commit adds SIGINT handler for the library process that kills the whole test process group. The upside is that system resources allocated in the library are cleaned up even in this case.

We also reset signal handlers in the test process after the fork now.

Signed-off-by Cyril Hrubis <[email protected]>
---
 lib/tst_test.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
lib/tst_test.c +0 -0
index 1bd2619..cd5b049 100644

lib/tst_test.c static void heartbeat_handler(int sig · line 733

Quoted 10 lines
lib/tst_test.c +6 -0
@@ -724,6 +724,16 @@ static void heartbeat_handler(int sig
 LTP_ATTRIBUTE_UNUSED)
 	alarm(results->timeout);
 }
 
+#define SIGINT_MSG "Sending SIGKILL to test process...\n"
+
+static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
+{
+	if (test_pid > 0) {
+		(void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);

Hi,

This still gives me a warning:
tst_test.c: In function ‘sigint_handler’:
tst_test.c:732:3: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]
(void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
^
We could use fprintf(stderr):
fprintf(stderr, "%s\n", SIGINT_MSG);

Patch looks OK to me. I was thinking about making test pid a foreground process group, but setup for that looked more complex than this approach (either temporarily block SIGTTOU or somehow synchronize lib and test pid to call tcsetpgrp() at right moment.

Regards,
Jan

Quoted 41 lines
lib/tst_test.c +4 -0
+		kill(-test_pid, SIGKILL);
+	}
+}
+
 void tst_set_timeout(unsigned int timeout)
 {
lib/tst_test.c +6 -1
 	char *mul = getenv("LTP_TIMEOUT_MUL");

@@ -767,18 +777,23 @@ void tst_run_tcases(int argc, char *argv[], struct
 tst_test *self)
 	else
 		tst_set_timeout(300);
 
+	SAFE_SIGNAL(SIGINT, sigint_handler);
+
 	test_pid = fork();
 	if (test_pid < 0)
 		tst_brk(TBROK | TERRNO, "fork()");
 
 	if (!test_pid) {
+		SAFE_SIGNAL(SIGALRM, SIG_DFL);
+		SAFE_SIGNAL(SIGUSR1, SIG_DFL);
+		SAFE_SIGNAL(SIGINT, SIG_DFL);
 		SAFE_SETPGID(0, 0);
 		testrun();
 	}
 
 	SAFE_WAITPID(test_pid, &status, 0);
-
 	alarm(0);
+	SAFE_SIGNAL(SIGINT, SIG_DFL);
 
 	if (WIFEXITED(status) && WEXITSTATUS(status))

do_exit(WEXITSTATUS(status));
--
2.7.3

--
Cyril Hrubis
[email protected]

#2 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

Quoted 20 lines · Jan Stancek

----- Original Message -----

Quoted 6 lines

From: "Cyril Hrubis" <[email protected]>

lib/tst_test.c +0 -0
index 1bd2619..cd5b049 100644

lib/tst_test.c static void heartbeat_handler(int sig · line 733

Quoted 10 lines
lib/tst_test.c +6 -0
@@ -724,6 +724,16 @@ static void heartbeat_handler(int sig
 LTP_ATTRIBUTE_UNUSED)
      alarm(results->timeout);
 }
 
+#define SIGINT_MSG "Sending SIGKILL to test process...\n"
+
+static void sigint_handler(int sig LTP_ATTRIBUTE_UNUSED)
+{
+     if (test_pid > 0) {
+             (void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);

Hi,

Quoted 7 lines

This still gives me a warning:
tst_test.c: In function ‘sigint_handler’:
tst_test.c:732:3: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result]

   (void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
   ^

We could use fprintf(stderr):
fprintf(stderr, "%s\n", SIGINT_MSG);

fprintf() isn't guaranteed to work inside a signal handler, unlike write() [see the list of "async-signal-safe" functions in the signal(7) manpage]. I think the officially recommended way to shut up this warning is something like

    if (write(fd, msg, len) < len) {
        /* It's OK to ignore the error because ... */
    }

which is a bit longwinded but works.

thanks
-- PMM

author
#3 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

Hi!

Quoted 13 lines
Quoted 7 lines

This still gives me a warning:
tst_test.c: In function ???sigint_handler???:
tst_test.c:732:3: warning: ignoring return value of ???write???, declared with attribute warn_unused_result [-Wunused-result]

   (void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
   ^

We could use fprintf(stderr):
fprintf(stderr, "%s\n", SIGINT_MSG);

fprintf() isn't guaranteed to work inside a signal handler, unlike write() [see the list of "async-signal-safe" functions in the signal(7) manpage]. I think the officially recommended way to shut up this warning is something like

Quoted 5 lines
    if (write(fd, msg, len) < len) {
        /* It's OK to ignore the error because ... */
    }

which is a bit longwinded but works.

More about this in:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

In short somebody thought that suppressing the warning with (void) is a bad idea for security. Which it indeed is for cases where somebody does it for setuid() and friends. But it's more or less fine for cases like this.

And at the bottom of the bug they decided that we need two warn_unused_result, one where it's OK to suppress the warning and one where it's not. But nobody implemented that so far.

I would be inclined to ingrore this warning in this case rather than working around compiler bugs.

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#4 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

In reply to Jan Stancek

Hi!

Quoted 7 lines

This still gives me a warning:
tst_test.c: In function ???sigint_handler???:
tst_test.c:732:3: warning: ignoring return value of ???write???, declared with attribute warn_unused_result [-Wunused-result]

   (void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
   ^

We could use fprintf(stderr):
fprintf(stderr, "%s\n", SIGINT_MSG);

Ping.

What do you think about this?

Should we ignore the warning for the time being or add the dummy if () to silence it?

Signature · 2 lines
Cyril Hrubis
[email protected]
#5 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

----- Original Message -----

Quoted 16 lines

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 16 August, 2016 1:34:51 PM
Subject: Re: [PATCH] tst_test: Propagate SIGINT to test process

Hi!

Quoted 8 lines

This still gives me a warning:
tst_test.c: In function ???sigint_handler???:
tst_test.c:732:3: warning: ignoring return value of ???write???, declared with attribute warn_unused_result [-Wunused-result]

   (void)write(2, SIGINT_MSG, sizeof(SIGINT_MSG) - 1);
   ^

We could use fprintf(stderr):
fprintf(stderr, "%s\n", SIGINT_MSG);

Quoted 6 lines

Ping.

What do you think about this?

Should we ignore the warning for the time being or add the dummy if () to silence it?

I'd go with dummy if (). I think we should try to keep warnings down, at least for lib. I always check compilation output, and one less warning to go through each time is IMO worth making this simple workaround.

Regards,
Jan

Quoted 5 lines

--
Cyril Hrubis
[email protected]

author
#6 lore raw

PATCH 1/1 tst_test: Propagate SIGINT to test process

Hi!

Quoted 3 lines

I'd go with dummy if (). I think we should try to keep warnings down, at least for lib. I always check compilation output, and one less warning to go through each time is IMO worth making this simple workaround.

Pushed with that change.

Signature · 2 lines
Cyril Hrubis
[email protected]