[Qemu-devel] [5055] Handle terminating signals (Gerd Hoffmann)

[Qemu-devel] [5055] Handle terminating signals (Gerd Hoffmann)

[Anthony Liguori]

Revision: 5055
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5055
Author:   aliguori
Date:     2008-08-21 20:08:03 +0000 (Thu, 21 Aug 2008)

Log Message:
-----------
Handle terminating signals (Gerd Hoffmann)

This patch makes qemu handle signals better.  It 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.

To avoid calling vm_start() from the signal handler main_loop() got an
additional check so qemu_system_shutdown_request() works even when the
vm is in stopped state.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/curses.c
    trunk/sdl.c
    trunk/vl.c

Modified: trunk/curses.c
===================================================================
--- trunk/curses.c	2008-08-21 19:33:09 UTC (rev 5054)
+++ trunk/curses.c	2008-08-21 20:08:03 UTC (rev 5055)
@@ -350,8 +350,6 @@
     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 */

Modified: trunk/sdl.c
===================================================================
--- trunk/sdl.c	2008-08-21 19:33:09 UTC (rev 5054)
+++ trunk/sdl.c	2008-08-21 20:08:03 UTC (rev 5055)
@@ -476,10 +476,8 @@
                 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 @@
         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;

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-08-21 19:33:09 UTC (rev 5054)
+++ trunk/vl.c	2008-08-21 20:08:03 UTC (rev 5055)
@@ -7621,6 +7621,8 @@
                 timeout = 0;
             }
         } else {
+            if (shutdown_requested)
+                break;
             timeout = 10;
         }
 #ifdef CONFIG_PROFILER
@@ -8185,6 +8187,26 @@
 
 #define MAX_NET_CLIENTS 32
 
+#ifndef _WIN32
+
+static void termsig_handler(int signal)
+{
+    qemu_system_shutdown_request();
+}
+
+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);
+}
+
+#endif
+
 int main(int argc, char **argv)
 {
 #ifdef CONFIG_GDBSTUB
@@ -9073,6 +9095,11 @@
 #endif
     }
 
+#ifndef _WIN32
+    /* must be after terminal init, SDL library changes signal handlers */
+    termsig_setup();
+#endif
+
     /* Maintain compatibility with multiple stdio monitors */
     if (!strcmp(monitor_device,"stdio")) {
         for (i = 0; i < MAX_SERIAL_PORTS; i++) {

[Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Jan Kiszka]

Anthony Liguori wrote:
> Revision: 5055
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5055
> Author:   aliguori
> Date:     2008-08-21 20:08:03 +0000 (Thu, 21 Aug 2008)
> 
> Log Message:
> -----------
> Handle terminating signals (Gerd Hoffmann)
> 
> This patch makes qemu handle signals better.  It 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.
> 
> To avoid calling vm_start() from the signal handler main_loop() got an
> additional check so qemu_system_shutdown_request() works even when the
> vm is in stopped state.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 

...

> Modified: trunk/vl.c
> ===================================================================
> --- trunk/vl.c	2008-08-21 19:33:09 UTC (rev 5054)
> +++ trunk/vl.c	2008-08-21 20:08:03 UTC (rev 5055)
> @@ -7621,6 +7621,8 @@
>                  timeout = 0;
>              }
>          } else {
> +            if (shutdown_requested)
> +                break;
>              timeout = 10;
>          }
>  #ifdef CONFIG_PROFILER

Could we define the policy that no patch is merged which introduces new
compiler warnings? Fix below remove the one caused by the hunk above,
but it still leaves some doubts for the semi-informed reader because the
"if (shutdown_requested)" block under vm_running also checks for
no_shutdown. Please confirm that leaving it out here was by intention.

Jan

--------

Always return EXCP_INTERRUPT when leaving main_loop due to
shutdown_requested.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 vl.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -7624,8 +7624,10 @@ static int main_loop(void)
                 timeout = 0;
             }
         } else {
-            if (shutdown_requested)
+            if (shutdown_requested) {
+                ret = EXCP_INTERRUPT;
                 break;
+            }
             timeout = 10;
         }
 #ifdef CONFIG_PROFILER

Re: [Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Gerd Hoffmann]

  Hi,

>>          } else {
>> +            if (shutdown_requested)
>> +                break;
>>              timeout = 10;

> Could we define the policy that no patch is merged which introduces new
> compiler warnings?

Just double-checked.  I don't get a warning for some strange reason.
Was wondered how a warning bypassed my attention.

You probably see "ret can be used uninitialized ...", right?

> --- a/vl.c
> +++ b/vl.c
> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>                  timeout = 0;
>              }
>          } else {
> -            if (shutdown_requested)
> +            if (shutdown_requested) {
> +                ret = EXCP_INTERRUPT;
>                  break;
> +            }

Fix looks fine to me.

cheers,
  Gerd

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

[Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Jan Kiszka]

Gerd Hoffmann wrote:
>   Hi,
> 
>>>          } else {
>>> +            if (shutdown_requested)
>>> +                break;
>>>              timeout = 10;
> 
>> Could we define the policy that no patch is merged which introduces new
>> compiler warnings?
> 
> Just double-checked.  I don't get a warning for some strange reason.
> Was wondered how a warning bypassed my attention.

I like colorgcc for this a lot (either distcc or icecream is said to
support this as well). :)

> 
> You probably see "ret can be used uninitialized ...", right?

Yep. Are you compiling qemu with gcc-3.3 (here: 3.3.3 hammer, x86_64 host)?

> 
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>>                  timeout = 0;
>>              }
>>          } else {
>> -            if (shutdown_requested)
>> +            if (shutdown_requested) {
>> +                ret = EXCP_INTERRUPT;
>>                  break;
>> +            }
> 
> Fix looks fine to me.

So we don't need to bother about no_shutdown in this path?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Gerd Hoffmann]

Jan Kiszka wrote:
>> Just double-checked.  I don't get a warning for some strange reason.
>> Was wondered how a warning bypassed my attention.
> 
> I like colorgcc for this a lot (either distcc or icecream is said to
> support this as well). :)

emacs does this too, and it even beams the cursor to the line in
question.  Assuming there is a warning in the first place.

>> You probably see "ret can be used uninitialized ...", right?
> 
> Yep. Are you compiling qemu with gcc-3.3 (here: 3.3.3 hammer, x86_64 host)?

compat-gcc-34 package here, --version says:
gcc34 (GCC) 3.4.6 20060404 (Red Hat 3.4.6-9)

> 
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>>>                  timeout = 0;
>>>              }
>>>          } else {
>>> -            if (shutdown_requested)
>>> +            if (shutdown_requested) {
>>> +                ret = EXCP_INTERRUPT;
>>>                  break;
>>> +            }
>> Fix looks fine to me.
> 
> So we don't need to bother about no_shutdown in this path?

I don't think so.  The no_shutdown makes qemu stop the vm instead of
exiting instantly, probably to allow analyzing the situation in case of
a guest-triggered shutdown.  There is no point in doing so in case the
vm already is in stopped state.

cheers,
  Gerd

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

Re: [Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Anthony Liguori]

Jan Kiszka wrote:
> Could we define the policy that no patch is merged which introduces new
> compiler warnings? Fix below remove the one caused by the hunk above,
> but it still leaves some doubts for the semi-informed reader because the
> "if (shutdown_requested)" block under vm_running also checks for
> no_shutdown. Please confirm that leaving it out here was by intention.
>   

I do check for warnings.  The version of GCC I'm using (3.4.6 20060404 
(Red Hat 3.4.6-9)) does not complain about these things.  We probably 
should add -Wall to the build and take the time to fix up all of the 
warnings that occur.

Regards,

Anthony Liguori

> Jan
>
> --------
>
> Always return EXCP_INTERRUPT when leaving main_loop due to
> shutdown_requested.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  vl.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: b/vl.c
> ===================================================================
> --- a/vl.c
> +++ b/vl.c
> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>                  timeout = 0;
>              }
>          } else {
> -            if (shutdown_requested)
> +            if (shutdown_requested) {
> +                ret = EXCP_INTERRUPT;
>                  break;
> +            }
>              timeout = 10;
>          }
>  #ifdef CONFIG_PROFILER
>
>
>   

[Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Could we define the policy that no patch is merged which introduces new
>> compiler warnings? Fix below remove the one caused by the hunk above,
>> but it still leaves some doubts for the semi-informed reader because the
>> "if (shutdown_requested)" block under vm_running also checks for
>> no_shutdown. Please confirm that leaving it out here was by intention.
>>   
> 
> I do check for warnings.  The version of GCC I'm using (3.4.6 20060404
> (Red Hat 3.4.6-9)) does not complain about these things.  We probably
> should add -Wall to the build and take the time to fix up all of the
> warnings that occur.

Makes /me wonder why 3.4 thinks there is no problem here (while this is
obviously wrong).

Being lazy and SuSE-based (which only provides a 3.3.3 for such legacy
use cases, IIRC), and also remembering vaguely that there used to be
warnings about 3.4 /wrt qemu, I kept 3.3.3.

BTW, do you also check for 64-bit issues? My feeling is that this - at
least - used to be a rare host platform for qemu contributors.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Jan Kiszka]

Gerd Hoffmann wrote:
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>>>>                  timeout = 0;
>>>>              }
>>>>          } else {
>>>> -            if (shutdown_requested)
>>>> +            if (shutdown_requested) {
>>>> +                ret = EXCP_INTERRUPT;
>>>>                  break;
>>>> +            }
>>> Fix looks fine to me.
>> So we don't need to bother about no_shutdown in this path?
> 
> I don't think so.  The no_shutdown makes qemu stop the vm instead of
> exiting instantly, probably to allow analyzing the situation in case of
> a guest-triggered shutdown.  There is no point in doing so in case the
> vm already is in stopped state.

OK, makes sense.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Anthony Liguori]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>   
>> I do check for warnings.  The version of GCC I'm using (3.4.6 20060404
>> (Red Hat 3.4.6-9)) does not complain about these things.  We probably
>> should add -Wall to the build and take the time to fix up all of the
>> warnings that occur.
>>     
>
> Makes /me wonder why 3.4 thinks there is no problem here (while this is
> obviously wrong).
>
> Being lazy and SuSE-based (which only provides a 3.3.3 for such legacy
> use cases, IIRC), and also remembering vaguely that there used to be
> warnings about 3.4 /wrt qemu, I kept 3.3.3.
>   

Could just be that more warnings are enabled by default with your 
particular version of GCC.

> BTW, do you also check for 64-bit issues? My feeling is that this - at
> least - used to be a rare host platform for qemu contributors.
>   

I run a 64-bit host.

I run Fedora 9 x86_64.  I have as many optional libraries installed as 
possible.  I also have a mingw32 cross compiler setup that I use to test 
the windows build.  The mingw32 compiler is gcc4 and throws an awful lot 
of warnings.

Regards,

Anthony Liguori

> Jan
>
>   

[Qemu-devel] Re: [5055] Handle terminating signals (Gerd Hoffmann)

[Jan Kiszka]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>> Revision: 5055
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5055
>> Author:   aliguori
>> Date:     2008-08-21 20:08:03 +0000 (Thu, 21 Aug 2008)
>>
>> Log Message:
>> -----------
>> Handle terminating signals (Gerd Hoffmann)
>>
>> This patch makes qemu handle signals better.  It 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.
>>
>> To avoid calling vm_start() from the signal handler main_loop() got an
>> additional check so qemu_system_shutdown_request() works even when the
>> vm is in stopped state.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
> 
> ...
> 
>> Modified: trunk/vl.c
>> ===================================================================
>> --- trunk/vl.c	2008-08-21 19:33:09 UTC (rev 5054)
>> +++ trunk/vl.c	2008-08-21 20:08:03 UTC (rev 5055)
>> @@ -7621,6 +7621,8 @@
>>                  timeout = 0;
>>              }
>>          } else {
>> +            if (shutdown_requested)
>> +                break;
>>              timeout = 10;
>>          }
>>  #ifdef CONFIG_PROFILER
> 
> Could we define the policy that no patch is merged which introduces new
> compiler warnings? Fix below remove the one caused by the hunk above,
> but it still leaves some doubts for the semi-informed reader because the
> "if (shutdown_requested)" block under vm_running also checks for
> no_shutdown. Please confirm that leaving it out here was by intention.
> 
> Jan
> 
> --------
> 
> Always return EXCP_INTERRUPT when leaving main_loop due to
> shutdown_requested.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  vl.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: b/vl.c
> ===================================================================
> --- a/vl.c
> +++ b/vl.c
> @@ -7624,8 +7624,10 @@ static int main_loop(void)
>                  timeout = 0;
>              }
>          } else {
> -            if (shutdown_requested)
> +            if (shutdown_requested) {
> +                ret = EXCP_INTERRUPT;
>                  break;
> +            }
>              timeout = 10;
>          }
>  #ifdef CONFIG_PROFILER

After discussing how this issue slipped in, I think it was forgotten to
actually merge the fix... :->

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux