[Qemu-devel] [PATCH] catch signals

[Qemu-devel] [PATCH] catch signals

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

  Hi,

This patch adds a signal handler to qemu, so fatal signals don't kill
off qemu.  Instead a shutdown request is issued, like it is done when
you close the SDL window.  qemu cleans up nicely then, cespecially it
calls all atexit handlers.

please apply,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

[-- Attachment #2: 0004-catch-signals-so-atexit-handlers-are-called-correct.patch --]
[-- Type: text/plain, Size: 2772 bytes --]

>From 886775f5229d83d73f3d162b919ea68b386b2a60 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 5 Aug 2008 17:53:36 +0200
Subject: [PATCH] catch signals, so atexit handlers are called correctly.


Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 curses.c |    2 --
 sdl.c    |    5 -----
 vl.c     |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/curses.c b/curses.c
index 87aa9b3..03580fb 100644
--- a/curses.c
+++ b/curses.c
@@ -346,8 +346,6 @@ void curses_display_init(DisplayState *ds, int full_screen)
     atexit(curses_atexit);
 
 #ifndef _WIN32
-    signal(SIGINT, SIG_DFL);
-    signal(SIGQUIT, SIG_DFL);
 #if defined(SIGWINCH) && defined(KEY_RESIZE)
     /* some curses implementations provide a handler, but we
      * want to be sure this is handled regardless of the library */
diff --git a/sdl.c b/sdl.c
index 0edc4a0..9ac5725 100644
--- a/sdl.c
+++ b/sdl.c
@@ -636,11 +636,6 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
         fprintf(stderr, "Could not initialize SDL - exiting\n");
         exit(1);
     }
-#ifndef _WIN32
-    /* NOTE: we still want Ctrl-C to work, so we undo the SDL redirections */
-    signal(SIGINT, SIG_DFL);
-    signal(SIGQUIT, SIG_DFL);
-#endif
 
     ds->dpy_update = sdl_update;
     ds->dpy_resize = sdl_resize;
diff --git a/vl.c b/vl.c
index e929370..c70b984 100644
--- a/vl.c
+++ b/vl.c
@@ -8141,6 +8141,40 @@ static BOOL WINAPI qemu_ctrl_handler(DWORD type)
 
 #define MAX_NET_CLIENTS 32
 
+#ifndef _WIN32
+static void termsig_handler(int signal)
+{
+    switch (signal) {
+    case SIGSEGV:
+    case SIGBUS:
+        /* returning from signal handler most likely isn't going to work */
+        fprintf(stderr, "qemu: got signal %d (%s), taking emergency exit\n",
+                signal, strsignal(signal));
+        exit(1);
+        break;
+    default:
+        qemu_system_shutdown_request();
+        vm_start();	/* In case we're paused */
+        break;
+    }
+}
+
+static void termsig_setup(void)
+{
+    struct sigaction act;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_flags   = SA_RESETHAND;
+    act.sa_handler = termsig_handler;
+
+    sigaction(SIGINT,  &act, NULL);
+    sigaction(SIGTERM, &act, NULL);
+    sigaction(SIGQUIT, &act, NULL);
+    sigaction(SIGSEGV, &act, NULL);
+    sigaction(SIGBUS,  &act, NULL);
+}
+#endif
+
 int main(int argc, char **argv)
 {
 #ifdef CONFIG_GDBSTUB
@@ -9031,6 +9065,11 @@ int main(int argc, char **argv)
 #endif
     }
 
+#ifndef _WIN32
+    /* must be after terminal init, SDL changes signal handlers */
+    termsig_setup();
+#endif
+
     /* Maintain compatibility with multiple stdio monitors */
 
     has_monitor = 0;
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH] catch signals

[Samuel Thibault]

Gerd Hoffmann, le Tue 05 Aug 2008 18:09:39 +0200, a écrit :
> +    sigaction(SIGINT,  &act, NULL);
> +    sigaction(SIGTERM, &act, NULL);
> +    sigaction(SIGQUIT, &act, NULL);
> +    sigaction(SIGSEGV, &act, NULL);
> +    sigaction(SIGBUS,  &act, NULL);

+ SIGHUP and SIGPIPE?

Samuel

Re: [Qemu-devel] [PATCH] catch signals

[Daniel P. Berrange]

On Tue, Aug 05, 2008 at 06:09:39PM +0200, Gerd Hoffmann wrote:
>  
> +#ifndef _WIN32
> +static void termsig_handler(int signal)
> +{
> +    switch (signal) {
> +    case SIGSEGV:
> +    case SIGBUS:
> +        /* returning from signal handler most likely isn't going to work */
> +        fprintf(stderr, "qemu: got signal %d (%s), taking emergency exit\n",
> +                signal, strsignal(signal));
> +        exit(1);

Neither of these functions are on the POSIX  async-signal-safe list,
so their use from signal handlers is not a good idea.

'man 7 signal' will detail the safe functions that can be used from
signal handlers. http://kerneltrap.org/man/linux/man7/signal.7

> +        break;
> +    default:
> +        qemu_system_shutdown_request();

This is ok it merely sets a flag

> +        vm_start();	/* In case we're paused */

I've not checked all the functions this calls to see if they
are safe, but I have a feeling this won't be safe in general
because the vm_state_notify() will invoke a number of callbacks
whose content we can't predict.

I think rather than trying todo anything in the signal handler,
it is safest to just set a flag and have its state checked
and acted upon in the main loop.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] catch signals

[Samuel Thibault]

Daniel P. Berrange, le Tue 05 Aug 2008 17:35:19 +0100, a écrit :
> I think rather than trying todo anything in the signal handler,
> it is safest to just set a flag and have its state checked
> and acted upon in the main loop.

We can not do that for SIGSEGV/SIGBUS, however.

Samuel

Re: [Qemu-devel] [PATCH] catch signals

[Daniel P. Berrange]

On Tue, Aug 05, 2008 at 05:53:21PM +0100, Samuel Thibault wrote:
> Daniel P. Berrange, le Tue 05 Aug 2008 17:35:19 +0100, a écrit :
> > I think rather than trying todo anything in the signal handler,
> > it is safest to just set a flag and have its state checked
> > and acted upon in the main loop.
> 
> We can not do that for SIGSEGV/SIGBUS, however.

We shouldn't be trying todo anything for SEGV/BUS. It is basically game
over at that point - you've no chance of orderly shutdown. Only QUIT, INT,
TERM, HUP should be trying todo graceful shutdown, because those don't
imply your process is corrupting its memory/ doing bad stuff.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] catch signals

[Jamie Lokier]

Daniel P. Berrange wrote:
> On Tue, Aug 05, 2008 at 05:53:21PM +0100, Samuel Thibault wrote:
> > Daniel P. Berrange, le Tue 05 Aug 2008 17:35:19 +0100, a écrit :
> > > I think rather than trying todo anything in the signal handler,
> > > it is safest to just set a flag and have its state checked
> > > and acted upon in the main loop.
> > 
> > We can not do that for SIGSEGV/SIGBUS, however.
> 
> We shouldn't be trying todo anything for SEGV/BUS. It is basically game
> over at that point - you've no chance of orderly shutdown. Only QUIT, INT,
> TERM, HUP should be trying todo graceful shutdown, because those don't
> imply your process is corrupting its memory/ doing bad stuff.

If you can detect _where_ is triggering SIGSEGV/SIGBUS, and it's in
generated code, then you've got a good chance of treating it as a
special kind of trap and aborting cleanly.

Also, while orderly shutdown is not possible, some subsystems could
register an "emergency shutdown" hook which is async-signal-safe.

Logging would have to be disabled during them, and they'd just do
things to tidy up, e.g. unlinking temporary files, writing memory/disk
state to an emergency snapshot if that's possible, complete pending
writes to disk image formats if that makes them safer.

I do something like that in a program of mine: register cleanup
handlers, and some of them say "can be called from an emergency signal".

Finally, another reason to have emergency cleanups is when you send
SIGTERM but it's wedged, and the main loop isn't responding.  After a
while, you want to kill the thing, and a bit of signal-safe cleanup
then is good.

-- Jamie

Re: [Qemu-devel] [PATCH] catch signals

[Gerd Hoffmann]

Samuel Thibault wrote:
> Gerd Hoffmann, le Tue 05 Aug 2008 18:09:39 +0200, a écrit :
>> +    sigaction(SIGINT,  &act, NULL);
>> +    sigaction(SIGTERM, &act, NULL);
>> +    sigaction(SIGQUIT, &act, NULL);
>> +    sigaction(SIGSEGV, &act, NULL);
>> +    sigaction(SIGBUS,  &act, NULL);
> 
> + SIGHUP and SIGPIPE?

SIGHUP yes.  SIGPIPE is handled elsewhere in the code.  Also guest
shutdown isn't the thing we want to do on SIGPIPE ...

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH] catch signals

[Gerd Hoffmann]

Daniel P. Berrange wrote:
> On Tue, Aug 05, 2008 at 06:09:39PM +0200, Gerd Hoffmann wrote:
>>  
>> +#ifndef _WIN32
>> +static void termsig_handler(int signal)
>> +{
>> +    switch (signal) {
>> +    case SIGSEGV:
>> +    case SIGBUS:
>> +        /* returning from signal handler most likely isn't going to work */
>> +        fprintf(stderr, "qemu: got signal %d (%s), taking emergency exit\n",
>> +                signal, strsignal(signal));
>> +        exit(1);
> 
> Neither of these functions are on the POSIX  async-signal-safe list,
> so their use from signal handlers is not a good idea.

We are in dead water already and also don't plan to ever return from the
 signal handler.  Is it really a problem then?

I want to be able to do cleanups (well, at least attempt) even in case
of a segfault.  If exit() + atexit handlers isn't going to fill the bill
we'll have to create some signal-save emergency cleanup handlers.

>> +        vm_start();	/* In case we're paused */
> 
> I think rather than trying todo anything in the signal handler,
> it is safest to just set a flag and have its state checked
> and acted upon in the main loop.

Isn't going to fly for SIGSEGV and SIGBUS.  Agree for the other signals.

I'll go check whenever we can handle stopped cpus some other way (guess
I just need an additional check in the main_loop() function).

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/

Re: [Qemu-devel] [PATCH] catch signals

[Daniel P. Berrange]

On Wed, Aug 06, 2008 at 11:20:39AM +0200, Gerd Hoffmann wrote:
> Daniel P. Berrange wrote:
> > On Tue, Aug 05, 2008 at 06:09:39PM +0200, Gerd Hoffmann wrote:
> >>  
> >> +#ifndef _WIN32
> >> +static void termsig_handler(int signal)
> >> +{
> >> +    switch (signal) {
> >> +    case SIGSEGV:
> >> +    case SIGBUS:
> >> +        /* returning from signal handler most likely isn't going to work */
> >> +        fprintf(stderr, "qemu: got signal %d (%s), taking emergency exit\n",
> >> +                signal, strsignal(signal));
> >> +        exit(1);
> > 
> > Neither of these functions are on the POSIX  async-signal-safe list,
> > so their use from signal handlers is not a good idea.
> 
> We are in dead water already and also don't plan to ever return from the
>  signal handler.  Is it really a problem then?

Yes, because if you further corrupt state in the signal handler it makes
debugging what went wrong even harder than it already is. In SEGV/BUS
case you really want to be able to get a decent core dump to analyse, so
the utmost care should be taken to avoid further messing up state. 

The STDIO libs are not re-entrant safe so if the original code was in a
STDIO function, and a SEGV comes in on another thread, the signal handler
will likely deadlock in a mutex. 'exit' will call into STDIO to flush
buffers, so will suffer the same problem.

> I want to be able to do cleanups (well, at least attempt) even in case
> of a segfault.  If exit() + atexit handlers isn't going to fill the bill
> we'll have to create some signal-save emergency cleanup handlers.

You could use 'sigaltstack' to setup an alternate pre-allocated stack
and carefully code the handler so it only uses pre-allocated memory,
or mem from the alt-stack & signal-safe functions. If you need something
really complex, you could 'fork' a cleanup program and communicate any
info to it using a pipe.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] catch signals

[Ian Jackson]

Gerd Hoffmann writes ("[Qemu-devel] [PATCH] catch signals"):
> This patch adds a signal handler to qemu, so fatal signals don't kill
> off qemu.  Instead a shutdown request is issued, like it is done when
> you close the SDL window.  qemu cleans up nicely then, cespecially it
> calls all atexit handlers.
...
> +    sigaction(SIGINT,  &act, NULL);
> +    sigaction(SIGTERM, &act, NULL);
> +    sigaction(SIGQUIT, &act, NULL);
> +    sigaction(SIGSEGV, &act, NULL);
> +    sigaction(SIGBUS,  &act, NULL);

This is wrong, I think.  It's fine to take this action for
INT and TERM.

It's very wrong to do so for SEGV and BUS.  QUIT is used for debugging
so should be left set to DFL too.

Ian.

Re: [Qemu-devel] [PATCH] catch signals

[Gerd Hoffmann]

Ian Jackson wrote:
> It's very wrong to do so for SEGV and BUS.

Why

Note there is a newer version which doesn't call signal-unsafe functions
 in the signal handler any more.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/