Attachment 577796 Details for Bug 739836 – [PATCH] rework a bit signal handling

[patch] [PATCH] rework a bit signal handling

rework-signal-handling.mbox.patch (text/plain), 9.96 KB, created by Yann Droneaud on 2012-04-16 17:43:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Yann Droneaud

Created: 2012-04-16 17:43:43 UTC

Size: 9.96 KB

patch

obsolete

>From 8cb59dcacfaddb9737d2114c4b9873a26b75b9bf Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 19:37:29 +0200
>Subject: [PATCH 0/9] Rework a bit the way NetworkManager handle signals
>
>To be error prone, NetworkManager should let someone really kill it
>if it's stuck handling some other events.
>
>Yann Droneaud (9):
>  main: no need for a dedicated mask variable in setup_signals()
>  main: enable handler for SIGPIPE
>  main: handle SIGFPE like SIGILL and such
>  main: handle SIGQUIT like SIGTERM and such
>  main: removed in_fatal from nm_signal_handler()
>  main: abort if nm_signal_handler() can't write to the pipe
>  main: abort in case of unknown signals in nm_signal_handler()
>  main: restore default signal handler to catch multiple occurences
>  main: made SIGABRT, SIGSEGV, ... core dump as expected
>
> src/main.c |   47 +++++++++++++++++++++++++++++++----------------
> 1 files changed, 31 insertions(+), 16 deletions(-)
>
>-- 
>1.7.7.6
>
>From 33b9240c17e4761c977798c36c2ac8d1dcdbf593 Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:31:58 +0200
>Subject: [PATCH 1/9] main: no need for a dedicated mask variable in
> setup_signals()
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 4a21959..42301b9 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -135,7 +135,6 @@ static void
> setup_signals (void)
> {
> 	struct sigaction action;
>-	sigset_t mask;
> 	GIOChannel *quit_channel;
> 
> 	/* Set up our quit pipe */
>@@ -148,9 +147,8 @@ setup_signals (void)
> 	quit_channel = g_io_channel_unix_new (quit_pipe[0]);
> 	g_io_add_watch_full (quit_channel, G_PRIORITY_HIGH, G_IO_IN | G_IO_ERR, quit_watch, NULL, NULL);
> 
>-	sigemptyset (&mask);
> 	action.sa_handler = nm_signal_handler;
>-	action.sa_mask = mask;
>+	sigemptyset (&action.sa_mask);
> 	action.sa_flags = 0;
> 	sigaction (SIGTERM,  &action, NULL);
> 	sigaction (SIGINT,  &action, NULL);
>-- 
>1.7.7.6
>
>From ebff1b9c5b4ca07af4bebb16aec51b950ed6fc1f Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:57:56 +0200
>Subject: [PATCH 2/9] main: enable handler for SIGPIPE
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 42301b9..5ac96df 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -159,6 +159,7 @@ setup_signals (void)
> 	sigaction (SIGSEGV, &action, NULL);
> 	sigaction (SIGABRT, &action, NULL);
> 	sigaction (SIGUSR1,  &action, NULL);
>+	sigaction (SIGPIPE,  &action, NULL);
> }
> 
> static gboolean
>-- 
>1.7.7.6
>
>From f29b4578109d4a4e53b9ce0901558120adc3be82 Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:55:44 +0200
>Subject: [PATCH 3/9] main: handle SIGFPE like SIGILL and such
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 5ac96df..8e6ac8b 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -85,11 +85,11 @@ nm_signal_handler (int signo)
> 	case SIGBUS:
> 	case SIGILL:
> 	case SIGABRT:
>+	case SIGFPE:
> 		nm_log_warn (LOGD_CORE, "caught signal %d. Generating backtrace...", signo);
> 		nm_logging_backtrace ();
> 		exit (1);
> 		break;
>-	case SIGFPE:
> 	case SIGPIPE:
> 		/* let the fatal signals interrupt us */
> 		--in_fatal;
>-- 
>1.7.7.6
>
>From 5a505d3d6622720dba597a486bd8770dd3254393 Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:58:43 +0200
>Subject: [PATCH 4/9] main: handle SIGQUIT like SIGTERM and such
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 8e6ac8b..92f3bac 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -98,6 +98,7 @@ nm_signal_handler (int signo)
> 		x = write (quit_pipe[1], "X", 1);
> 		break;
> 	case SIGINT:
>+	case SIGQUIT:
> 	case SIGTERM:
> 		/* let the fatal signals interrupt us */
> 		--in_fatal;
>@@ -151,6 +152,7 @@ setup_signals (void)
> 	sigemptyset (&action.sa_mask);
> 	action.sa_flags = 0;
> 	sigaction (SIGTERM,  &action, NULL);
>+	sigaction (SIGQUIT,  &action, NULL);
> 	sigaction (SIGINT,  &action, NULL);
> 	sigaction (SIGILL,  &action, NULL);
> 	sigaction (SIGBUS,  &action, NULL);
>-- 
>1.7.7.6
>
>From df290fd32d7e3ab095b0b19606c46f134075a01c Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:20:12 +0200
>Subject: [PATCH 5/9] main: removed in_fatal from nm_signal_handler()
>
>- Removed in_fatal handling, this thing seems wrong and error prone,
>  It will be replaced in a later patch
>- Additionnaly, made 'x' a ssize_t variable
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |   11 +----------
> 1 files changed, 1 insertions(+), 10 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 92f3bac..d931c0b 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -73,12 +73,7 @@ static gboolean quit_early = FALSE;
> static void
> nm_signal_handler (int signo)
> {
>-	static int in_fatal = 0, x;
>-
>-	/* avoid loops */
>-	if (in_fatal > 0)
>-		return;
>-	++in_fatal;
>+	ssize_t x;
> 
> 	switch (signo) {
> 	case SIGSEGV:
>@@ -92,7 +87,6 @@ nm_signal_handler (int signo)
> 		break;
> 	case SIGPIPE:
> 		/* let the fatal signals interrupt us */
>-		--in_fatal;
> 		nm_log_warn (LOGD_CORE, "caught signal %d, shutting down abnormally. Generating backtrace...", signo);
> 		nm_logging_backtrace ();
> 		x = write (quit_pipe[1], "X", 1);
>@@ -101,17 +95,14 @@ nm_signal_handler (int signo)
> 	case SIGQUIT:
> 	case SIGTERM:
> 		/* let the fatal signals interrupt us */
>-		--in_fatal;
> 		nm_log_info (LOGD_CORE, "caught signal %d, shutting down normally.", signo);
> 		quit_early = TRUE;
> 		x = write (quit_pipe[1], "X", 1);
> 		break;
> 	case SIGHUP:
>-		--in_fatal;
> 		/* Reread config stuff like system config files, VPN service files, etc */
> 		break;
> 	case SIGUSR1:
>-		--in_fatal;
> 		/* Play with log levels or something */
> 		break;
> 	default:
>-- 
>1.7.7.6
>
>From 09e0653fde1bb2850fc040f950b04f543e02c4e9 Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:50:04 +0200
>Subject: [PATCH 6/9] main: abort if nm_signal_handler() can't write to the
> pipe
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index d931c0b..6f28112 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -90,6 +90,9 @@ nm_signal_handler (int signo)
> 		nm_log_warn (LOGD_CORE, "caught signal %d, shutting down abnormally. Generating backtrace...", signo);
> 		nm_logging_backtrace ();
> 		x = write (quit_pipe[1], "X", 1);
>+		if (x != 1) {
>+			abort();
>+		}
> 		break;
> 	case SIGINT:
> 	case SIGQUIT:
>@@ -98,6 +101,9 @@ nm_signal_handler (int signo)
> 		nm_log_info (LOGD_CORE, "caught signal %d, shutting down normally.", signo);
> 		quit_early = TRUE;
> 		x = write (quit_pipe[1], "X", 1);
>+		if (x != 1) {
>+			abort();
>+		}
> 		break;
> 	case SIGHUP:
> 		/* Reread config stuff like system config files, VPN service files, etc */
>-- 
>1.7.7.6
>
>From ff1e7e8709750391a59f719589ef79dfe3c8e977 Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 18:37:04 +0200
>Subject: [PATCH 7/9] main: abort in case of unknown signals in
> nm_signal_handler()
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 6f28112..9819c6e 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -112,7 +112,8 @@ nm_signal_handler (int signo)
> 		/* Play with log levels or something */
> 		break;
> 	default:
>-		signal (signo, nm_signal_handler);
>+		nm_log_info (LOGD_CORE, "caught invalid signal %d, shutting down abnormally.", signo);
>+		abort();
> 		break;
> 	}
> }
>-- 
>1.7.7.6
>
>From 503b6a66612ca327aebe9ac0e38977a1d07a3e4c Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 19:28:53 +0200
>Subject: [PATCH 8/9] main: restore default signal handler to catch multiple
> occurences
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |   12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index 9819c6e..cc6969c 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -73,6 +73,7 @@ static gboolean quit_early = FALSE;
> static void
> nm_signal_handler (int signo)
> {
>+	struct sigaction action;
> 	ssize_t x;
> 
> 	switch (signo) {
>@@ -93,6 +94,10 @@ nm_signal_handler (int signo)
> 		if (x != 1) {
> 			abort();
> 		}
>+		action.sa_handler = SIG_DFL;
>+		sigemptyset (&action.sa_mask);
>+		action.sa_flags = 0;
>+		sigaction (signo, &action, NULL);
> 		break;
> 	case SIGINT:
> 	case SIGQUIT:
>@@ -104,6 +109,13 @@ nm_signal_handler (int signo)
> 		if (x != 1) {
> 			abort();
> 		}
>+		/* set all quit signals to its default handler in case someone really want to kill us */
>+		action.sa_handler = SIG_DFL;
>+		sigemptyset (&action.sa_mask);
>+		action.sa_flags = 0;
>+		sigaction (SIGINT, &action, NULL);
>+		sigaction (SIGQUIT, &action, NULL);
>+		sigaction (SIGTERM, &action, NULL);
> 		break;
> 	case SIGHUP:
> 		/* Reread config stuff like system config files, VPN service files, etc */
>-- 
>1.7.7.6
>
>From 8cb59dcacfaddb9737d2114c4b9873a26b75b9bf Mon Sep 17 00:00:00 2001
>From: Yann Droneaud <yann@droneaud.fr>
>Date: Mon, 16 Apr 2012 19:29:52 +0200
>Subject: [PATCH 9/9] main: made SIGABRT, SIGSEGV, ... core dump as expected
>
>
>Signed-off-by: Yann Droneaud <yann@droneaud.fr>
>---
> src/main.c |    6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
>diff --git a/src/main.c b/src/main.c
>index cc6969c..ba1e51c 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -84,7 +84,11 @@ nm_signal_handler (int signo)
> 	case SIGFPE:
> 		nm_log_warn (LOGD_CORE, "caught signal %d. Generating backtrace...", signo);
> 		nm_logging_backtrace ();
>-		exit (1);
>+		action.sa_handler = SIG_DFL;
>+		sigemptyset (&action.sa_mask);
>+		action.sa_flags = 0;
>+		sigaction (signo, &action, NULL);
>+		kill (getpid (), signo);
> 		break;
> 	case SIGPIPE:
> 		/* let the fatal signals interrupt us */
>-- 
>1.7.7.6
>

Actions: View | Diff

Attachments on bug 739836: 523968 | 524361 | 533610 | 555210 | 555211 | 575228 | 575571 | 577696 | 577796