All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
@ 2011-03-15 11:56 Gleb Natapov
  2011-03-25 12:04 ` Gleb Natapov
  2011-03-30 18:39 ` Peter Maydell
  0 siblings, 2 replies; 15+ messages in thread
From: Gleb Natapov @ 2011-03-15 11:56 UTC (permalink / raw)
  To: qemu-devel

Currently when rogue script kills QEMU process (using TERM/INT/HUP
signal) it looks indistinguishable from system shutdown. Lets report
that QEMU was killed and leave some clues about the killer identity.
  
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
  v1->v2:
    - print message from a main loop instead of signal handler
  v2->v3
    - use pid_t to store pid instead of int 

diff --git a/os-posix.c b/os-posix.c
index 38c29d1..36d937c 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -61,9 +61,9 @@ void os_setup_early_signal_handling(void)
     sigaction(SIGPIPE, &act, NULL);
 }
 
-static void termsig_handler(int signal)
+static void termsig_handler(int signal, siginfo_t *info, void *c)
 {
-    qemu_system_shutdown_request();
+    qemu_system_killed(info->si_signo, info->si_pid);
 }
 
 static void sigchld_handler(int signal)
@@ -76,7 +76,8 @@ void os_setup_signal_handling(void)
     struct sigaction act;
 
     memset(&act, 0, sizeof(act));
-    act.sa_handler = termsig_handler;
+    act.sa_sigaction = termsig_handler;
+    act.sa_flags = SA_SIGINFO;
     sigaction(SIGINT,  &act, NULL);
     sigaction(SIGHUP,  &act, NULL);
     sigaction(SIGTERM, &act, NULL);
diff --git a/sysemu.h b/sysemu.h
index 0a83ab9..f868500 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -66,6 +66,8 @@ void qemu_system_vmstop_request(int reason);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
+void qemu_system_killed(int signal, pid_t pid);
+void qemu_kill_report(void);
 extern qemu_irq qemu_system_powerdown;
 void qemu_system_reset(void);
 
diff --git a/vl.c b/vl.c
index 5e007a7..000c845 100644
--- a/vl.c
+++ b/vl.c
@@ -1213,7 +1213,8 @@ typedef struct QEMUResetEntry {
 static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
     QTAILQ_HEAD_INITIALIZER(reset_handlers);
 static int reset_requested;
-static int shutdown_requested;
+static int shutdown_requested, shutdown_signal = -1;
+static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
 static int vmstop_requested;
@@ -1225,6 +1226,15 @@ int qemu_shutdown_requested(void)
     return r;
 }
 
+void qemu_kill_report(void)
+{
+    if (shutdown_signal != -1) {
+        fprintf(stderr, "Got signal %d from pid %d\n",
+                         shutdown_signal, shutdown_pid);
+        shutdown_signal = -1;
+    }
+}
+
 int qemu_reset_requested(void)
 {
     int r = reset_requested;
@@ -1298,6 +1308,13 @@ void qemu_system_reset_request(void)
     qemu_notify_event();
 }
 
+void qemu_system_killed(int signal, pid_t pid)
+{
+    shutdown_signal = signal;
+    shutdown_pid = pid;
+    qemu_system_shutdown_request();
+}
+
 void qemu_system_shutdown_request(void)
 {
     shutdown_requested = 1;
@@ -1441,6 +1458,7 @@ static void main_loop(void)
             vm_stop(VMSTOP_DEBUG);
         }
         if (qemu_shutdown_requested()) {
+            qemu_kill_report();
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
             if (no_shutdown) {
                 vm_stop(VMSTOP_SHUTDOWN);
--
			Gleb.

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-15 11:56 [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal Gleb Natapov
@ 2011-03-25 12:04 ` Gleb Natapov
  2011-03-26 13:50   ` Blue Swirl
  2011-03-30 18:39 ` Peter Maydell
  1 sibling, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2011-03-25 12:04 UTC (permalink / raw)
  To: qemu-devel

Ping?

On Tue, Mar 15, 2011 at 01:56:04PM +0200, Gleb Natapov wrote:
> Currently when rogue script kills QEMU process (using TERM/INT/HUP
> signal) it looks indistinguishable from system shutdown. Lets report
> that QEMU was killed and leave some clues about the killer identity.
>   
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>   v1->v2:
>     - print message from a main loop instead of signal handler
>   v2->v3
>     - use pid_t to store pid instead of int 
> 
> diff --git a/os-posix.c b/os-posix.c
> index 38c29d1..36d937c 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -61,9 +61,9 @@ void os_setup_early_signal_handling(void)
>      sigaction(SIGPIPE, &act, NULL);
>  }
>  
> -static void termsig_handler(int signal)
> +static void termsig_handler(int signal, siginfo_t *info, void *c)
>  {
> -    qemu_system_shutdown_request();
> +    qemu_system_killed(info->si_signo, info->si_pid);
>  }
>  
>  static void sigchld_handler(int signal)
> @@ -76,7 +76,8 @@ void os_setup_signal_handling(void)
>      struct sigaction act;
>  
>      memset(&act, 0, sizeof(act));
> -    act.sa_handler = termsig_handler;
> +    act.sa_sigaction = termsig_handler;
> +    act.sa_flags = SA_SIGINFO;
>      sigaction(SIGINT,  &act, NULL);
>      sigaction(SIGHUP,  &act, NULL);
>      sigaction(SIGTERM, &act, NULL);
> diff --git a/sysemu.h b/sysemu.h
> index 0a83ab9..f868500 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -66,6 +66,8 @@ void qemu_system_vmstop_request(int reason);
>  int qemu_shutdown_requested(void);
>  int qemu_reset_requested(void);
>  int qemu_powerdown_requested(void);
> +void qemu_system_killed(int signal, pid_t pid);
> +void qemu_kill_report(void);
>  extern qemu_irq qemu_system_powerdown;
>  void qemu_system_reset(void);
>  
> diff --git a/vl.c b/vl.c
> index 5e007a7..000c845 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1213,7 +1213,8 @@ typedef struct QEMUResetEntry {
>  static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
>      QTAILQ_HEAD_INITIALIZER(reset_handlers);
>  static int reset_requested;
> -static int shutdown_requested;
> +static int shutdown_requested, shutdown_signal = -1;
> +static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int vmstop_requested;
> @@ -1225,6 +1226,15 @@ int qemu_shutdown_requested(void)
>      return r;
>  }
>  
> +void qemu_kill_report(void)
> +{
> +    if (shutdown_signal != -1) {
> +        fprintf(stderr, "Got signal %d from pid %d\n",
> +                         shutdown_signal, shutdown_pid);
> +        shutdown_signal = -1;
> +    }
> +}
> +
>  int qemu_reset_requested(void)
>  {
>      int r = reset_requested;
> @@ -1298,6 +1308,13 @@ void qemu_system_reset_request(void)
>      qemu_notify_event();
>  }
>  
> +void qemu_system_killed(int signal, pid_t pid)
> +{
> +    shutdown_signal = signal;
> +    shutdown_pid = pid;
> +    qemu_system_shutdown_request();
> +}
> +
>  void qemu_system_shutdown_request(void)
>  {
>      shutdown_requested = 1;
> @@ -1441,6 +1458,7 @@ static void main_loop(void)
>              vm_stop(VMSTOP_DEBUG);
>          }
>          if (qemu_shutdown_requested()) {
> +            qemu_kill_report();
>              monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
>              if (no_shutdown) {
>                  vm_stop(VMSTOP_SHUTDOWN);
> --
> 			Gleb.

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-25 12:04 ` Gleb Natapov
@ 2011-03-26 13:50   ` Blue Swirl
  2011-03-26 13:55     ` Gleb Natapov
  0 siblings, 1 reply; 15+ messages in thread
From: Blue Swirl @ 2011-03-26 13:50 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: qemu-devel

On Fri, Mar 25, 2011 at 2:04 PM, Gleb Natapov <gleb@redhat.com> wrote:
> Ping?

Does not work:
INT:
Got signal 951049944 from pid 0
TERM:
Got signal -1553068904 from pid 0
HUP:
Got signal 1 from pid 16185
Even here the pid is not correct, it should be 3098.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-26 13:50   ` Blue Swirl
@ 2011-03-26 13:55     ` Gleb Natapov
  2011-03-26 14:12       ` Blue Swirl
  0 siblings, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2011-03-26 13:55 UTC (permalink / raw)
  To: Blue Swirl; +Cc: qemu-devel

On Sat, Mar 26, 2011 at 03:50:46PM +0200, Blue Swirl wrote:
> On Fri, Mar 25, 2011 at 2:04 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > Ping?
> 
> Does not work:
> INT:
> Got signal 951049944 from pid 0
> TERM:
> Got signal -1553068904 from pid 0
You use SDL correct? This is SDL problem and I fixed it in SDL upstream.

> HUP:
> Got signal 1 from pid 16185
> Even here the pid is not correct, it should be 3098.
HUP should work. Why do you think that pid should be 3098? Bash has its
own build in kill command IIRC.

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-26 13:55     ` Gleb Natapov
@ 2011-03-26 14:12       ` Blue Swirl
  0 siblings, 0 replies; 15+ messages in thread
From: Blue Swirl @ 2011-03-26 14:12 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: qemu-devel

On Sat, Mar 26, 2011 at 3:55 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Sat, Mar 26, 2011 at 03:50:46PM +0200, Blue Swirl wrote:
>> On Fri, Mar 25, 2011 at 2:04 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> > Ping?
>>
>> Does not work:
>> INT:
>> Got signal 951049944 from pid 0
>> TERM:
>> Got signal -1553068904 from pid 0
> You use SDL correct? This is SDL problem and I fixed it in SDL upstream.

OK, with VNC it works.

>> HUP:
>> Got signal 1 from pid 16185
>> Even here the pid is not correct, it should be 3098.
> HUP should work. Why do you think that pid should be 3098? Bash has its
> own build in kill command IIRC.

Right, I used killall which isn't a builtin, sorry. Thanks, applied.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-15 11:56 [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal Gleb Natapov
  2011-03-25 12:04 ` Gleb Natapov
@ 2011-03-30 18:39 ` Peter Maydell
  2011-03-30 18:43   ` Gleb Natapov
                     ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Peter Maydell @ 2011-03-30 18:39 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: qemu-devel

On 15 March 2011 11:56, Gleb Natapov <gleb@redhat.com> wrote:
> Currently when rogue script kills QEMU process (using TERM/INT/HUP
> signal) it looks indistinguishable from system shutdown. Lets report
> that QEMU was killed and leave some clues about the killer identity.

Unfortunately this patch causes qemu to segfault when killed
via ^C (at least on my Ubuntu maverick system). This is because
it registers a signal handler with sigaction, but then later
the SDL library is initialised and it reinstalls our handler
with plain old signal:

        ohandler = signal(SIGINT, SDL_HandleSIG);
        if ( ohandler != SIG_DFL )
                signal(SIGINT, ohandler);

This is clearly buggy but on the other hand SDL is pretty widely
deployed and it's the default QEMU video output method, so I think
we need to work around it :-(

The most straightforward fix is to get the signal number from
argument one and not to bother printing the PID that killed us.

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:39 ` Peter Maydell
@ 2011-03-30 18:43   ` Gleb Natapov
  2011-03-30 18:53     ` Peter Maydell
  2011-03-30 18:49   ` Anthony Liguori
  2011-03-30 21:35   ` malc
  2 siblings, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2011-03-30 18:43 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

On Wed, Mar 30, 2011 at 07:39:31PM +0100, Peter Maydell wrote:
> On 15 March 2011 11:56, Gleb Natapov <gleb@redhat.com> wrote:
> > Currently when rogue script kills QEMU process (using TERM/INT/HUP
> > signal) it looks indistinguishable from system shutdown. Lets report
> > that QEMU was killed and leave some clues about the killer identity.
> 
> Unfortunately this patch causes qemu to segfault when killed
> via ^C (at least on my Ubuntu maverick system). This is because
> it registers a signal handler with sigaction, but then later
> the SDL library is initialised and it reinstalls our handler
> with plain old signal:
> 
>         ohandler = signal(SIGINT, SDL_HandleSIG);
>         if ( ohandler != SIG_DFL )
>                 signal(SIGINT, ohandler);
> 
I fixed this in SDL upstream.

> This is clearly buggy but on the other hand SDL is pretty widely
> deployed and it's the default QEMU video output method, so I think
> we need to work around it :-(
> 
> The most straightforward fix is to get the signal number from
> argument one and not to bother printing the PID that killed us.
> 
For debugging purposes pid is useful. We cam register signal handler
after SDL is initialized though (if waiting for SDL update is not an
option).

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:39 ` Peter Maydell
  2011-03-30 18:43   ` Gleb Natapov
@ 2011-03-30 18:49   ` Anthony Liguori
  2011-03-30 18:51     ` Gleb Natapov
  2011-03-30 21:35   ` malc
  2 siblings, 1 reply; 15+ messages in thread
From: Anthony Liguori @ 2011-03-30 18:49 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, Gleb Natapov

On 03/30/2011 01:39 PM, Peter Maydell wrote:
> On 15 March 2011 11:56, Gleb Natapov<gleb@redhat.com>  wrote:
>> Currently when rogue script kills QEMU process (using TERM/INT/HUP
>> signal) it looks indistinguishable from system shutdown. Lets report
>> that QEMU was killed and leave some clues about the killer identity.
> Unfortunately this patch causes qemu to segfault when killed
> via ^C (at least on my Ubuntu maverick system). This is because
> it registers a signal handler with sigaction, but then later
> the SDL library is initialised and it reinstalls our handler
> with plain old signal:
>
>          ohandler = signal(SIGINT, SDL_HandleSIG);
>          if ( ohandler != SIG_DFL )
>                  signal(SIGINT, ohandler);
>
> This is clearly buggy but on the other hand SDL is pretty widely
> deployed and it's the default QEMU video output method, so I think
> we need to work around it :-(
>
> The most straightforward fix is to get the signal number from
> argument one and not to bother printing the PID that killed us.

Or just #ifdefing this to 0 for now and then once an SDL version is 
released that works correctly, adding an appropriate guard.

Regards,

Anthony Liguori

> -- PMM
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:49   ` Anthony Liguori
@ 2011-03-30 18:51     ` Gleb Natapov
  0 siblings, 0 replies; 15+ messages in thread
From: Gleb Natapov @ 2011-03-30 18:51 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Peter Maydell, qemu-devel

On Wed, Mar 30, 2011 at 01:49:10PM -0500, Anthony Liguori wrote:
> On 03/30/2011 01:39 PM, Peter Maydell wrote:
> >On 15 March 2011 11:56, Gleb Natapov<gleb@redhat.com>  wrote:
> >>Currently when rogue script kills QEMU process (using TERM/INT/HUP
> >>signal) it looks indistinguishable from system shutdown. Lets report
> >>that QEMU was killed and leave some clues about the killer identity.
> >Unfortunately this patch causes qemu to segfault when killed
> >via ^C (at least on my Ubuntu maverick system). This is because
> >it registers a signal handler with sigaction, but then later
> >the SDL library is initialised and it reinstalls our handler
> >with plain old signal:
> >
> >         ohandler = signal(SIGINT, SDL_HandleSIG);
> >         if ( ohandler != SIG_DFL )
> >                 signal(SIGINT, ohandler);
> >
> >This is clearly buggy but on the other hand SDL is pretty widely
> >deployed and it's the default QEMU video output method, so I think
> >we need to work around it :-(
> >
> >The most straightforward fix is to get the signal number from
> >argument one and not to bother printing the PID that killed us.
> 
> Or just #ifdefing this to 0 for now and then once an SDL version is
> released that works correctly, adding an appropriate guard.
> 
I prefer to move signal init after SDL init.

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:43   ` Gleb Natapov
@ 2011-03-30 18:53     ` Peter Maydell
  2011-03-30 19:04       ` Gleb Natapov
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-03-30 18:53 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: qemu-devel

On 30 March 2011 19:43, Gleb Natapov <gleb@redhat.com> wrote:
> On Wed, Mar 30, 2011 at 07:39:31PM +0100, Peter Maydell wrote:
>> Unfortunately this patch causes qemu to segfault when killed
>> via ^C (at least on my Ubuntu maverick system). This is because
>> it registers a signal handler with sigaction, but then later
>> the SDL library is initialised and it reinstalls our handler
>> with plain old signal:
>>
>>         ohandler = signal(SIGINT, SDL_HandleSIG);
>>         if ( ohandler != SIG_DFL )
>>                 signal(SIGINT, ohandler);
>>
> I fixed this in SDL upstream.

Cool, but we can't rely on the SDL we're linked against being
a bugfixed version.

>> This is clearly buggy but on the other hand SDL is pretty widely
>> deployed and it's the default QEMU video output method, so I think
>> we need to work around it :-(
>>
>> The most straightforward fix is to get the signal number from
>> argument one and not to bother printing the PID that killed us.
>>
> For debugging purposes pid is useful. We cam register signal handler
> after SDL is initialized though (if waiting for SDL update is not an
> option).

I'm not convinced about the utility of printing the pid, personally.
Most programs get along fine without printing anything when
they receive a terminal signal. However you're right that it should
be straightforward enough to move the signal init. In fact it
looks like this got broken somewhere along the line, the
call to os_setup_signal_handling() is already preceded with a comment:

    /* must be after terminal init, SDL library changes signal handlers */
    os_setup_signal_handling();

...we just aren't actually after the terminal init any more :-(

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:53     ` Peter Maydell
@ 2011-03-30 19:04       ` Gleb Natapov
  2011-03-30 20:51         ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Gleb Natapov @ 2011-03-30 19:04 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

On Wed, Mar 30, 2011 at 07:53:41PM +0100, Peter Maydell wrote:
> On 30 March 2011 19:43, Gleb Natapov <gleb@redhat.com> wrote:
> > On Wed, Mar 30, 2011 at 07:39:31PM +0100, Peter Maydell wrote:
> >> Unfortunately this patch causes qemu to segfault when killed
> >> via ^C (at least on my Ubuntu maverick system). This is because
> >> it registers a signal handler with sigaction, but then later
> >> the SDL library is initialised and it reinstalls our handler
> >> with plain old signal:
> >>
> >>         ohandler = signal(SIGINT, SDL_HandleSIG);
> >>         if ( ohandler != SIG_DFL )
> >>                 signal(SIGINT, ohandler);
> >>
> > I fixed this in SDL upstream.
> 
> Cool, but we can't rely on the SDL we're linked against being
> a bugfixed version.
> 
> >> This is clearly buggy but on the other hand SDL is pretty widely
> >> deployed and it's the default QEMU video output method, so I think
> >> we need to work around it :-(
> >>
> >> The most straightforward fix is to get the signal number from
> >> argument one and not to bother printing the PID that killed us.
> >>
> > For debugging purposes pid is useful. We cam register signal handler
> > after SDL is initialized though (if waiting for SDL update is not an
> > option).
> 
> I'm not convinced about the utility of printing the pid, personally.
> Most programs get along fine without printing anything when
> they receive a terminal signal. However you're right that it should
Well qemu is a bit of special case. It is long running process that
takes huge amount of memory and, as suchm it becomes a target of various
monitoring script which, when configured incorrectly, start killing
perfectly valid guests. In addition killing of the guest looks exactly
like guest shutdown to management software because we call shutdow_request
in the signal handler.

> be straightforward enough to move the signal init. In fact it
> looks like this got broken somewhere along the line, the
> call to os_setup_signal_handling() is already preceded with a comment:
> 
>     /* must be after terminal init, SDL library changes signal handlers */
>     os_setup_signal_handling();
> 
> ...we just aren't actually after the terminal init any more :-(
> 
Exactly. This should do the trick (not tested).


diff --git a/vl.c b/vl.c
index 192a240..93aaccf 100644
--- a/vl.c
+++ b/vl.c
@@ -3148,9 +3148,6 @@ int main(int argc, char **argv, char **envp)
 
     cpu_synchronize_all_post_init();
 
-    /* must be after terminal init, SDL library changes signal handlers */
-    os_setup_signal_handling();
-
     set_numa_modes();
 
     current_machine = machine;
@@ -3206,6 +3203,9 @@ int main(int argc, char **argv, char **envp)
         break;
     }
 
+    /* must be after terminal init, SDL library changes signal handlers */
+    os_setup_signal_handling();
+
 #ifdef CONFIG_VNC
     /* init remote displays */
     if (vnc_display) {

--
			Gleb.

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 19:04       ` Gleb Natapov
@ 2011-03-30 20:51         ` Peter Maydell
  2011-03-31  9:04           ` Gleb Natapov
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2011-03-30 20:51 UTC (permalink / raw)
  To: Gleb Natapov; +Cc: qemu-devel

2011/3/30 Gleb Natapov <gleb@redhat.com>:
> On Wed, Mar 30, 2011 at 07:53:41PM +0100, Peter Maydell wrote:
>> I'm not convinced about the utility of printing the pid, personally.
>> Most programs get along fine without printing anything when
>> they receive a terminal signal.

> Well qemu is a bit of special case. It is long running process that
> takes huge amount of memory and, as suchm it becomes a target of various
> monitoring script which, when configured incorrectly, start killing
> perfectly valid guests. In addition killing of the guest looks exactly
> like guest shutdown to management software because we call shutdow_request
> in the signal handler.

That sounds like a flaw in the communication protocol between
qemu and the management software, which would be better fixed
by having qemu communicate the reason for exit directly (ie
not just by printing to stderr), surely?

> Exactly. This should do the trick (not tested).

Looks good, and a test shows I don't get the segfault any more.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

although I guess you'll want to submit it with a sensible git
commit message :-)

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 18:39 ` Peter Maydell
  2011-03-30 18:43   ` Gleb Natapov
  2011-03-30 18:49   ` Anthony Liguori
@ 2011-03-30 21:35   ` malc
  2011-03-31  5:35     ` Gleb Natapov
  2 siblings, 1 reply; 15+ messages in thread
From: malc @ 2011-03-30 21:35 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, Gleb Natapov

On Wed, 30 Mar 2011, Peter Maydell wrote:

> On 15 March 2011 11:56, Gleb Natapov <gleb@redhat.com> wrote:
> > Currently when rogue script kills QEMU process (using TERM/INT/HUP
> > signal) it looks indistinguishable from system shutdown. Lets report
> > that QEMU was killed and leave some clues about the killer identity.
> 
> Unfortunately this patch causes qemu to segfault when killed
> via ^C (at least on my Ubuntu maverick system). This is because
> it registers a signal handler with sigaction, but then later
> the SDL library is initialised and it reinstalls our handler
> with plain old signal:
> 
>         ohandler = signal(SIGINT, SDL_HandleSIG);
>         if ( ohandler != SIG_DFL )
>                 signal(SIGINT, ohandler);
> 
> This is clearly buggy but on the other hand SDL is pretty widely
> deployed and it's the default QEMU video output method, so I think
> we need to work around it :-(
> 
> The most straightforward fix is to get the signal number from
> argument one and not to bother printing the PID that killed us.
> 

Maybe using SDL_INIT_NOPARACHUTE is worth doing?

-- 
mailto:av1474@comtv.ru

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 21:35   ` malc
@ 2011-03-31  5:35     ` Gleb Natapov
  0 siblings, 0 replies; 15+ messages in thread
From: Gleb Natapov @ 2011-03-31  5:35 UTC (permalink / raw)
  To: malc; +Cc: Peter Maydell, qemu-devel

On Thu, Mar 31, 2011 at 01:35:31AM +0400, malc wrote:
> On Wed, 30 Mar 2011, Peter Maydell wrote:
> 
> > On 15 March 2011 11:56, Gleb Natapov <gleb@redhat.com> wrote:
> > > Currently when rogue script kills QEMU process (using TERM/INT/HUP
> > > signal) it looks indistinguishable from system shutdown. Lets report
> > > that QEMU was killed and leave some clues about the killer identity.
> > 
> > Unfortunately this patch causes qemu to segfault when killed
> > via ^C (at least on my Ubuntu maverick system). This is because
> > it registers a signal handler with sigaction, but then later
> > the SDL library is initialised and it reinstalls our handler
> > with plain old signal:
> > 
> >         ohandler = signal(SIGINT, SDL_HandleSIG);
> >         if ( ohandler != SIG_DFL )
> >                 signal(SIGINT, ohandler);
> > 
> > This is clearly buggy but on the other hand SDL is pretty widely
> > deployed and it's the default QEMU video output method, so I think
> > we need to work around it :-(
> > 
> > The most straightforward fix is to get the signal number from
> > argument one and not to bother printing the PID that killed us.
> > 
> 
> Maybe using SDL_INIT_NOPARACHUTE is worth doing?
> 
We do it already, but SDL mangles signal handlers anyway. The funny
thing is that parachute code actually correctly use sigaction when
available.

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal
  2011-03-30 20:51         ` Peter Maydell
@ 2011-03-31  9:04           ` Gleb Natapov
  0 siblings, 0 replies; 15+ messages in thread
From: Gleb Natapov @ 2011-03-31  9:04 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel

On Wed, Mar 30, 2011 at 09:51:29PM +0100, Peter Maydell wrote:
> 2011/3/30 Gleb Natapov <gleb@redhat.com>:
> > On Wed, Mar 30, 2011 at 07:53:41PM +0100, Peter Maydell wrote:
> >> I'm not convinced about the utility of printing the pid, personally.
> >> Most programs get along fine without printing anything when
> >> they receive a terminal signal.
> 
> > Well qemu is a bit of special case. It is long running process that
> > takes huge amount of memory and, as suchm it becomes a target of various
> > monitoring script which, when configured incorrectly, start killing
> > perfectly valid guests. In addition killing of the guest looks exactly
> > like guest shutdown to management software because we call shutdow_request
> > in the signal handler.
> 
> That sounds like a flaw in the communication protocol between
> qemu and the management software, which would be better fixed
> by having qemu communicate the reason for exit directly (ie
> not just by printing to stderr), surely?
> 
Yes, but this is more complex and changes QMP protocol.

> > Exactly. This should do the trick (not tested).
> 
> Looks good, and a test shows I don't get the segfault any more.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> although I guess you'll want to submit it with a sensible git
> commit message :-)
> 
Will do.

--
			Gleb.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-03-31  9:04 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-15 11:56 [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal Gleb Natapov
2011-03-25 12:04 ` Gleb Natapov
2011-03-26 13:50   ` Blue Swirl
2011-03-26 13:55     ` Gleb Natapov
2011-03-26 14:12       ` Blue Swirl
2011-03-30 18:39 ` Peter Maydell
2011-03-30 18:43   ` Gleb Natapov
2011-03-30 18:53     ` Peter Maydell
2011-03-30 19:04       ` Gleb Natapov
2011-03-30 20:51         ` Peter Maydell
2011-03-31  9:04           ` Gleb Natapov
2011-03-30 18:49   ` Anthony Liguori
2011-03-30 18:51     ` Gleb Natapov
2011-03-30 21:35   ` malc
2011-03-31  5:35     ` Gleb Natapov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.