[Qemu-devel] [PATCH] Handle terminating signals.

[Qemu-devel] [PATCH] Handle terminating signals.

[Gerd Hoffmann]

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

  Hi,

This patch makes qemu handle signals better.  For normal signals
(SIGINT, SIGTERM & friends) it just sets the request_shutdown flag,
making the main_loop exit and qemu taking the usual exit route, with
atexit handlers being called and so on, instead of qemu just being
killed by the signal.

For fatal signals such as SIGSEGV, where the exit handler can't return,
it allows to register clean up handlers which will be called instead of
the atexit handlers.  Those handlers must be save to run in signal context.

please apply,
  Gerd

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

[-- Attachment #2: 0004-Handle-terminating-signals.patch --]
[-- Type: text/plain, Size: 6896 bytes --]

>From 11d6a40bee3cdb3ee0d807cf2ab6537ba26afc25 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 5 Aug 2008 17:53:36 +0200
Subject: [PATCH] Handle terminating signals.

This patch makes qemu handle signals better.  For normal signals
(SIGINT, SIGTERM & friends) it just sets the request_shutdown flag,
making the main_loop exit and qemu taking the usual exit route, with
atexit handlers being called and so on, instead of qemu just being
killed by the signal.

For fatal signals such as SIGSEGV, where the exit handler can't return,
it allows to register clean up handlers which will be called instead of
the atexit handlers.  Those handlers must be save to run in signal context.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.target |    2 +-
 curses.c        |    2 -
 sdl.c           |    9 +----
 termsig.c       |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 termsig.h       |   15 ++++++++
 vl.c            |    6 +++
 6 files changed, 127 insertions(+), 11 deletions(-)
 create mode 100644 termsig.c
 create mode 100644 termsig.h

diff --git a/Makefile.target b/Makefile.target
index 42162c3..ae05af0 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -472,7 +472,7 @@ endif #CONFIG_DARWIN_USER
 # System emulator target
 ifndef CONFIG_USER_ONLY
 
-OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o net-checksum.o
+OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o net-checksum.o termsig.o
 ifdef CONFIG_WIN32
 OBJS+=block-raw-win32.o
 else
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..84a9d6d 100644
--- a/sdl.c
+++ b/sdl.c
@@ -476,10 +476,8 @@ static void sdl_refresh(DisplayState *ds)
                 sdl_process_key(&ev->key);
             break;
         case SDL_QUIT:
-            if (!no_quit) {
+            if (!no_quit)
                 qemu_system_shutdown_request();
-                vm_start();	/* In case we're paused */
-            }
             break;
         case SDL_MOUSEMOTION:
             if (gui_grab || kbd_mouse_is_absolute() ||
@@ -636,11 +634,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/termsig.c b/termsig.c
new file mode 100644
index 0000000..d0136a8
--- /dev/null
+++ b/termsig.c
@@ -0,0 +1,104 @@
+/*
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; under version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <signal.h>
+
+#include "qemu-common.h"
+#include "sysemu.h"
+#include "sys-queue.h"
+#include "termsig.h"
+
+#ifdef _WIN32
+
+void termsig_register_handler(void (*func)(void))
+{
+}
+
+void termsig_setup(void)
+{
+}
+
+#else
+
+struct handlers {
+    void (*func)(void);
+    int called;
+    TAILQ_ENTRY(handlers) list;
+};
+static TAILQ_HEAD(dummy, handlers) handlers = TAILQ_HEAD_INITIALIZER(handlers);
+static int termsig;
+
+void fatalsig_register_handler(void (*func)(void))
+{
+    struct handlers *h;
+
+    h = qemu_mallocz(sizeof(*h));
+    if (!h)
+        return;
+    h->func = func;
+    TAILQ_INSERT_TAIL(&handlers, h, list);
+}
+
+static void fatalsig_call_handlers(void)
+{
+    struct handlers *h;
+
+    TAILQ_FOREACH(h, &handlers, list) {
+        if (h->called)
+            continue;
+        h->called++;
+	h->func();
+    }
+}
+
+static void termsig_handler(int signal)
+{
+    switch (signal) {
+    case SIGSEGV:
+    case SIGBUS:
+        fatalsig_call_handlers();
+        abort();
+        break;
+    default:
+        if (termsig) {
+            /* Hmm, we got a exit signal before.  Still running.
+             * Main loop is probably stuck somewhere ... */
+            fatalsig_call_handlers();
+            _exit(1);
+        }
+        qemu_system_shutdown_request();
+        termsig = signal;
+        break;
+    }
+}
+
+void termsig_setup(void)
+{
+    struct sigaction act;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_handler = termsig_handler;
+
+    sigaction(SIGINT,  &act, NULL);
+    sigaction(SIGHUP,  &act, NULL);
+    sigaction(SIGTERM, &act, NULL);
+    sigaction(SIGQUIT, &act, NULL);
+
+    act.sa_flags = SA_RESETHAND | SA_ONSTACK;
+    sigaction(SIGSEGV, &act, NULL);
+    sigaction(SIGBUS,  &act, NULL);
+}
+
+#endif
diff --git a/termsig.h b/termsig.h
new file mode 100644
index 0000000..1cc1cdb
--- /dev/null
+++ b/termsig.h
@@ -0,0 +1,15 @@
+/*
+ * Register cleanup handler.
+ *
+ * These handlers are *only* called for emergeny exits, in case we
+ * can't use the normal exit() and thus atexit() handlers don't work.
+ *
+ * These handlers are called directly from the signal handler and
+ * thus are limited to functions which can safely be called from
+ * signal context (see "man 7 signal").
+ *
+ */
+void fatalsig_register_handler(void (*func)(void));
+
+/* setup signal handlers, called from main() */
+void termsig_setup(void);
diff --git a/vl.c b/vl.c
index e929370..56de905 100644
--- a/vl.c
+++ b/vl.c
@@ -37,6 +37,7 @@
 #include "qemu-char.h"
 #include "block.h"
 #include "audio/audio.h"
+#include "termsig.h"
 
 #include <unistd.h>
 #include <fcntl.h>
@@ -7585,6 +7586,8 @@ static int main_loop(void)
                 timeout = 0;
             }
         } else {
+            if (shutdown_requested)
+                break;
             timeout = 10;
         }
 #ifdef CONFIG_PROFILER
@@ -9031,6 +9034,9 @@ int main(int argc, char **argv)
 #endif
     }
 
+    /* must be after terminal init, SDL changes signal handlers */
+    termsig_setup();
+
     /* Maintain compatibility with multiple stdio monitors */
 
     has_monitor = 0;
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Ian Jackson]

Gerd Hoffmann writes ("[Qemu-devel] [PATCH] Handle terminating signals."):
> This patch makes qemu handle signals better.  For normal signals
> (SIGINT, SIGTERM & friends) it just sets the request_shutdown flag,
> making the main_loop exit and qemu taking the usual exit route, with
> atexit handlers being called and so on, instead of qemu just being
> killed by the signal.
>
> +    sigaction(SIGINT,  &act, NULL);
> +    sigaction(SIGHUP,  &act, NULL);
> +    sigaction(SIGTERM, &act, NULL);
> +    sigaction(SIGQUIT, &act, NULL);

SIGQUIT should not be in this list.  QUIT does not mean `please
terminate'.  It's a signal used for debugging purposes and usually
means `please pretend this program took a SEGV'.  The default action
is to die and dump core, and this should be preserved.  Anyone who
deliberately sends qemu a SIGQUIT will be annoyed if it traps it and
no-one will send it accidentally.

Ian.

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Gerd Hoffmann]

Ian Jackson wrote:
> Gerd Hoffmann writes ("[Qemu-devel] [PATCH] Handle terminating signals."):
>> This patch makes qemu handle signals better.  For normal signals
>> (SIGINT, SIGTERM & friends) it just sets the request_shutdown flag,
>> making the main_loop exit and qemu taking the usual exit route, with
>> atexit handlers being called and so on, instead of qemu just being
>> killed by the signal.
>>
>> +    sigaction(SIGINT,  &act, NULL);
>> +    sigaction(SIGHUP,  &act, NULL);
>> +    sigaction(SIGTERM, &act, NULL);
>> +    sigaction(SIGQUIT, &act, NULL);
> 
> SIGQUIT should not be in this list.  QUIT does not mean `please
> terminate'.  It's a signal used for debugging purposes and usually
> means `please pretend this program took a SEGV'.  The default action
> is to die and dump core, and this should be preserved.

Ok, we could take the SIGSEGV cleanup route, then call abort().  That
should come close enougth.  Does that sound ok?

cheers,
  Gerd

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

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Ian Jackson]

Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH] Handle terminating signals."):
> Ian Jackson wrote:
> > SIGQUIT should not be in this list.  QUIT does not mean `please
> > terminate'.  It's a signal used for debugging purposes and usually
> > means `please pretend this program took a SEGV'.  The default action
> > is to die and dump core, and this should be preserved.
> 
> Ok, we could take the SIGSEGV cleanup route, then call abort().  That
> should come close enougth.  Does that sound ok?

No, because the program should not attempt to catch SEGV either.

Ian.

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Stefano Stabellini]

Ian Jackson wrote:

> Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH] Handle terminating signals."):
>> Ian Jackson wrote:
>>> SIGQUIT should not be in this list.  QUIT does not mean `please
>>> terminate'.  It's a signal used for debugging purposes and usually
>>> means `please pretend this program took a SEGV'.  The default action
>>> is to die and dump core, and this should be preserved.
>> Ok, we could take the SIGSEGV cleanup route, then call abort().  That
>> should come close enougth.  Does that sound ok?
> 
> No, because the program should not attempt to catch SEGV either.
> 

I agree.
The idea of trying a clean exit when a termination signal is received is
good, but do we really need the complicated fatalsig_call_handlers and
fatalsig_register_handler system?

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Gerd Hoffmann]

Ian Jackson wrote:
> Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH] Handle terminating signals."):
>> Ian Jackson wrote:
>>> SIGQUIT should not be in this list.  QUIT does not mean `please
>>> terminate'.  It's a signal used for debugging purposes and usually
>>> means `please pretend this program took a SEGV'.  The default action
>>> is to die and dump core, and this should be preserved.
>> Ok, we could take the SIGSEGV cleanup route, then call abort().  That
>> should come close enougth.  Does that sound ok?
> 
> No, because the program should not attempt to catch SEGV either.

Why not?  Can you change your attitude to say "no" without giving
reasons please?

cheers,
  Gerd

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

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[M. Warner Losh]

In message: <48A1D6A3.4050406@redhat.com>
            Gerd Hoffmann <kraxel@redhat.com> writes:
: Ian Jackson wrote:
: > Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH] Handle terminating signals."):
: >> Ian Jackson wrote:
: >>> SIGQUIT should not be in this list.  QUIT does not mean `please
: >>> terminate'.  It's a signal used for debugging purposes and usually
: >>> means `please pretend this program took a SEGV'.  The default action
: >>> is to die and dump core, and this should be preserved.
: >> Ok, we could take the SIGSEGV cleanup route, then call abort().  That
: >> should come close enougth.  Does that sound ok?
: > 
: > No, because the program should not attempt to catch SEGV either.
: 
: Why not?  Can you change your attitude to say "no" without giving
: reasons please?

The only portable thing one can do when catching SEGV is terminate the
program.  Otherwise, when the signal handler returns, SEGV happens
again...

Warner

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Gerd Hoffmann]

M. Warner Losh wrote:
> In message: <48A1D6A3.4050406@redhat.com>
>             Gerd Hoffmann <kraxel@redhat.com> writes:
> : > No, because the program should not attempt to catch SEGV either.
> : 
> : Why not?  Can you change your attitude to say "no" without giving
> : reasons please?
> 
> The only portable thing one can do when catching SEGV is terminate the
> program.  Otherwise, when the signal handler returns, SEGV happens
> again...

Returning from the signal handler isn't going to work, sure.  The only
thing I want do is cleaning up before exiting.

Most apps never ever have to care about that.  Sometimes there are good
reasons to attempt a cleanup even for a SEGV though.  The X-Server for
example attempts to put the gfx card into a sane state then.

cheers,
  Gerd

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

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Anthony Liguori]

Gerd Hoffmann wrote:
> M. Warner Losh wrote:
>   
>> In message: <48A1D6A3.4050406@redhat.com>
>>             Gerd Hoffmann <kraxel@redhat.com> writes:
>> : > No, because the program should not attempt to catch SEGV either.
>> : 
>> : Why not?  Can you change your attitude to say "no" without giving
>> : reasons please?
>>
>> The only portable thing one can do when catching SEGV is terminate the
>> program.  Otherwise, when the signal handler returns, SEGV happens
>> again...
>>     
>
> Returning from the signal handler isn't going to work, sure.  The only
> thing I want do is cleaning up before exiting.
>
> Most apps never ever have to care about that.  Sometimes there are good
> reasons to attempt a cleanup even for a SEGV though.  The X-Server for
> example attempts to put the gfx card into a sane state then.
>   

Note my previous suggestion of pipe() + fork() gives you the desired 
cleanup behaviour without having to deal with any of this silliness.  
IMHO, the real problem here is that Xen's domain-0 interface isn't based 
on file descriptors to begin with.  What modern Linux interface that 
requires a clean-up action isn't, afterall?

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>   

Re: [Qemu-devel] [PATCH] Handle terminating signals.

[Daniel P. Berrange]

On Tue, Aug 12, 2008 at 09:42:07PM +0200, Gerd Hoffmann wrote:
> M. Warner Losh wrote:
> > In message: <48A1D6A3.4050406@redhat.com>
> >             Gerd Hoffmann <kraxel@redhat.com> writes:
> > : > No, because the program should not attempt to catch SEGV either.
> > : 
> > : Why not?  Can you change your attitude to say "no" without giving
> > : reasons please?
> > 
> > The only portable thing one can do when catching SEGV is terminate the
> > program.  Otherwise, when the signal handler returns, SEGV happens
> > again...
> 
> Returning from the signal handler isn't going to work, sure.  The only
> thing I want do is cleaning up before exiting.
> 
> Most apps never ever have to care about that.  Sometimes there are good
> reasons to attempt a cleanup even for a SEGV though.  The X-Server for
> example attempts to put the gfx card into a sane state then.

The X server often fails to do this cleanup. This is why they're moving
mode setting into the kernel so it can be *reliably* reset. Relying on
a SEGV handler for cleanup is just doomed to fail

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] Handle terminating signals.

[Ian Jackson]

Gerd Hoffmann writes ("Re: [Qemu-devel] [PATCH] Handle terminating signals."):
> Ian Jackson wrote:
> > No, because the program should not attempt to catch SEGV either.
> 
> Why not?  Can you change your attitude to say "no" without giving
> reasons please?

I guess this is another one of those pieces of `obvious' wisdom which
no-one previously bothered writing down.  I tried to find a clear
online reference which explains why but I couldn't find one, so I'll
try to explain it here:

If your program gets a SEGV, that means its memory may be corrupted.
Because the signal is asynchronous and may happen at any point, the
values of variables and data structures may be arbitrary and of course
inconsistent (since perhaps the program was halfway through modifying
a data structure).  The contents of the stack cannot be relied on.
Even the stack pointer may be corrupted (that might be the cause of
the crash).

Technically, _any_ attempt to resume the mainline program execution
(whether by returning or longjmping out of the SEGV handler, or by
running the same code inside the signal handler) has undefined
behaviour.  That includes _any attempt to access a global variable_
(other than a variable which is specially marked and protected, and
of course where the signal blocking is used appropriately - which is
not and cannot be the case here).

This means that attempts to `recover' or `clean up' when you get SEGV
are at least as likely to make things worse as they are to make things
better.  Furthermore bugs tend to happen in complex corner cases,
which is precisely the kind of situation where trying to continue or
tidy up after a SEGV is likely to do harm.  Even if the recovery code
doesn't actually worsen the data corruption and loss, it will make the
situation more confusing by making additional changes to the system
state.

That means it's much harder for a system administrator to recover or
repair, and also much harder for the operating system or application's
crash recovery code to cope with.  Post-crash data recovery is a
well-developed field; there are well-understood strategies adopted by
humans and computers under various circumstances.  These are likely to
be defeated by attempts to `clean up' between the detection of a fatal
corruption and actual death.

Both as a system administrators and as a programmers I curse programs
which try to trap fatal bugs like this.  It makes it harder to figure
out why the system is failing; it makes it harder to restore the
system to a working and correct state; it makes it harder to avoid
data corruption; and in the worst it can also lead to a cavalier
attitude towards bugs.

Thus the best thing to do - usually the only sane thing to do - with
the coredumping signals (SEGV, BUS, ABRT, QUIT, as applicable and
available) is to leave them unblocked and set to SIG_DFL.

(SIGQUIT is a different argument because it's only ever sent
explicitly.  But its purpose is to aid debugging and get an
instantaneous coredump, which would be defeated by catching it.)

Ian.