[PATCH weston] Added general signal handling and added sig_abrt

[Pekka Paalanen]

Hi,

this looks mostly fine, few comments below, and we want a v2 after
those.

Oh, could you put your real name in the sender field, so we get it in
the git history properly?

On Wed, 27 Mar 2013 14:59:28 +0100
blackwolf12333 <blackwolf12333 at gmail.com> wrote:

> ---
>  src/compositor.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index a2860fd..92e9cbe 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -3220,7 +3220,7 @@ print_backtrace(void)
>  
>  #endif
>  
> -static void
> +/*static void // we should not need them anymore
>  on_segv_signal(int s, siginfo_t *siginfo, void *context)
>  {
>  	/* This SIGSEGV handler will do a best-effort backtrace, and
> @@ -3229,7 +3229,7 @@ on_segv_signal(int s, siginfo_t *siginfo, void *context)
>  	 * raise SIGTRAP.  If we run weston under gdb from X or a
>  	 * different vt, and tell gdb "handle SIGSEGV nostop", this
>  	 * will allow weston to switch back to gdb on crash and then
> -	 * gdb will catch the crash with SIGTRAP. */
> +	 * gdb will catch the crash with SIGTRAP. 
>  
>  	weston_log("caught segv\n");
>  
> @@ -3240,6 +3240,40 @@ on_segv_signal(int s, siginfo_t *siginfo, void *context)
>  	raise(SIGTRAP);
>  }
>  
> +static void
> +on_abrt_signal(int s, siginfo_t *siginfo, void *context)
> +{
> +	/* This SIGABRT handler will do a best-effort backtrace, and
> +	 * then call the backend restore function, which will switch
> +	 * back to the vt we launched from or ungrab X etc and then
> +	 * raise SIGTRAP.  If we run weston under gdb from X or a
> +	 * different vt, and tell gdb "handle SIGABRT nostop", this
> +	 * will allow weston to switch back to gdb on crash and then
> +	 * gdb will catch the crash with SIGTRAP. 
> +	
> +	weston_log("caught abrt\n");
> +	
> +	print_backtrace();
> +	
> +	segv_compositor->restore(segv_compositor);
> +
> +	raise(SIGTRAP);
> +}*/

Just delete all the dead code, there no point in keeping it.

> +
> +static void
> +on_caught_signal(int s, siginfo_t *siginfo, void *context)
> +{
> +	/* This handler should handle all signals set in catch_signals()
> +	 * Currently that would be: SIG_SEGV and SIG_ABRT

Spelled without the underscore in code.

> +	 */
> +	weston_log("caught signal: %d\n", s);
> +	
> +	print_backtrace();
> +	
> +	segv_compositor->restore(segv_compositor);
> +
> +	raise(SIGTRAP);
> +}
>  
>  static void *
>  load_module(const char *name, const char *entrypoint)
> @@ -3393,6 +3427,20 @@ usage(int error_code)
>  	exit(error_code);
>  }
>  
> +static void
> +catch_signals()

I'd expect a void in parens, doesn't this produce a warning?

> +{
> +	struct sigaction action;
> +	
> +	action.sa_flags = SA_SIGINFO | SA_RESETHAND;
> +	action.sa_sigaction = on_caught_signal;
> +	sigemptyset(&action.sa_mask);
> +	sigaction(SIGSEGV, &action, NULL);
> +	
> +	sigemptyset(&action.sa_mask);

No need to sigemptyset again, the argument to sigaction is const.

> +	sigaction(SIGABRT, &action, NULL);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int ret = EXIT_SUCCESS;
> @@ -3400,7 +3448,6 @@ int main(int argc, char *argv[])
>  	struct weston_compositor *ec;
>  	struct wl_event_source *signals[4];
>  	struct wl_event_loop *loop;
> -	struct sigaction segv_action;
>  	struct weston_compositor
>  		*(*backend_init)(struct wl_display *display,
>  				 int *argc, char *argv[], const char *config_file);
> @@ -3491,10 +3538,7 @@ int main(int argc, char *argv[])
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	segv_action.sa_flags = SA_SIGINFO | SA_RESETHAND;
> -	segv_action.sa_sigaction = on_segv_signal;
> -	sigemptyset(&segv_action.sa_mask);
> -	sigaction(SIGSEGV, &segv_action, NULL);
> +	catch_signals();
>  	segv_compositor = ec;
>  
>  	ec->option_idle_time = idle_time;

With the above fixed, reviewed-by me.


Thanks,
pq