[Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Peter Maydell]

Add the termination signals SIGINT, SIGHUP and SIGTERM to the
list of signals which we handle synchronously via a signalfd.
This avoids a race condition where if we took the SIGTERM
in the middle of qemu_shutdown_requested:
    int r = shutdown_requested;
[SIGTERM here...]
    shutdown_requested = 0;

then the setting of the shutdown_requested flag by
termsig_handler() would be lost and QEMU would fail to
shut down. This was causing 'make check' to hang occasionally.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
---
 main-loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/main-loop.c b/main-loop.c
index 3cc79f8..8746abc 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -84,6 +84,9 @@ static int qemu_signal_init(void)
     sigaddset(&set, SIGIO);
     sigaddset(&set, SIGALRM);
     sigaddset(&set, SIGBUS);
+    sigaddset(&set, SIGINT);
+    sigaddset(&set, SIGHUP);
+    sigaddset(&set, SIGTERM);
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 
     sigdelset(&set, SIG_IPI);
-- 
1.9.1

Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Paolo Bonzini]

Il 25/09/2014 17:51, Peter Maydell ha scritto:
> Add the termination signals SIGINT, SIGHUP and SIGTERM to the
> list of signals which we handle synchronously via a signalfd.
> This avoids a race condition where if we took the SIGTERM
> in the middle of qemu_shutdown_requested:
>     int r = shutdown_requested;
> [SIGTERM here...]
>     shutdown_requested = 0;
> 
> then the setting of the shutdown_requested flag by
> termsig_handler() would be lost and QEMU would fail to
> shut down. This was causing 'make check' to hang occasionally.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  main-loop.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/main-loop.c b/main-loop.c
> index 3cc79f8..8746abc 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -84,6 +84,9 @@ static int qemu_signal_init(void)
>      sigaddset(&set, SIGIO);
>      sigaddset(&set, SIGALRM);
>      sigaddset(&set, SIGBUS);
> +    sigaddset(&set, SIGINT);
> +    sigaddset(&set, SIGHUP);
> +    sigaddset(&set, SIGTERM);
>      pthread_sigmask(SIG_BLOCK, &set, NULL);
>  
>      sigdelset(&set, SIG_IPI);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Add the termination signals SIGINT, SIGHUP and SIGTERM to the
> list of signals which we handle synchronously via a signalfd.
> This avoids a race condition where if we took the SIGTERM
> in the middle of qemu_shutdown_requested:
>     int r = shutdown_requested;
> [SIGTERM here...]
>     shutdown_requested = 0;
>
> then the setting of the shutdown_requested flag by
> termsig_handler() would be lost and QEMU would fail to
> shut down. This was causing 'make check' to hang occasionally.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable@nongnu.org
<snip>

I've been testing it with my latest Travis patches (- the make check
once patch) and it seems a lot better now:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Peter Maydell]

On 26 September 2014 11:37, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Add the termination signals SIGINT, SIGHUP and SIGTERM to the
>> list of signals which we handle synchronously via a signalfd.
>> This avoids a race condition where if we took the SIGTERM
>> in the middle of qemu_shutdown_requested:
>>     int r = shutdown_requested;
>> [SIGTERM here...]
>>     shutdown_requested = 0;
>>
>> then the setting of the shutdown_requested flag by
>> termsig_handler() would be lost and QEMU would fail to
>> shut down. This was causing 'make check' to hang occasionally.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: qemu-stable@nongnu.org
> <snip>
>
> I've been testing it with my latest Travis patches (- the make check
> once patch) and it seems a lot better now:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>

Thanks; applied to master.

-- PMM

Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Gonglei]

Hi,

> Subject: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and
> SIGTERM synchronously
> 
> Add the termination signals SIGINT, SIGHUP and SIGTERM to the
> list of signals which we handle synchronously via a signalfd.
> This avoids a race condition where if we took the SIGTERM
> in the middle of qemu_shutdown_requested:
>     int r = shutdown_requested;
> [SIGTERM here...]
>     shutdown_requested = 0;
> 
> then the setting of the shutdown_requested flag by
> termsig_handler() would be lost and QEMU would fail to
> shut down. This was causing 'make check' to hang occasionally.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I met a really upset thing after this patch, which I can't using gdb debug
Qemu process well. I use gdb attaching a Qemu process, and press 'c' to continue
executing. When I want to set a breakpoint for debugging, then press 'ctrl + c',
the Qemu will exit, because the SIGINT signal is captured by Qemu now. :(

What's your opinion? Thanks.

Best regards,
-Gonglei

> Cc: qemu-stable@nongnu.org
> ---
>  main-loop.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/main-loop.c b/main-loop.c
> index 3cc79f8..8746abc 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -84,6 +84,9 @@ static int qemu_signal_init(void)
>      sigaddset(&set, SIGIO);
>      sigaddset(&set, SIGALRM);
>      sigaddset(&set, SIGBUS);
> +    sigaddset(&set, SIGINT);
> +    sigaddset(&set, SIGHUP);
> +    sigaddset(&set, SIGTERM);
>      pthread_sigmask(SIG_BLOCK, &set, NULL);
> 
>      sigdelset(&set, SIG_IPI);
> --
> 1.9.1
> 

Re: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and SIGTERM synchronously

[Jan Kiszka]

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

On 2014-10-25 13:34, Gonglei wrote:
> Hi,
> 
>> Subject: [Qemu-devel] [PATCH] main-loop.c: Handle SIGINT, SIGHUP and
>> SIGTERM synchronously
>>
>> Add the termination signals SIGINT, SIGHUP and SIGTERM to the
>> list of signals which we handle synchronously via a signalfd.
>> This avoids a race condition where if we took the SIGTERM
>> in the middle of qemu_shutdown_requested:
>>     int r = shutdown_requested;
>> [SIGTERM here...]
>>     shutdown_requested = 0;
>>
>> then the setting of the shutdown_requested flag by
>> termsig_handler() would be lost and QEMU would fail to
>> shut down. This was causing 'make check' to hang occasionally.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I met a really upset thing after this patch, which I can't using gdb debug
> Qemu process well. I use gdb attaching a Qemu process, and press 'c' to continue
> executing. When I want to set a breakpoint for debugging, then press 'ctrl + c',
> the Qemu will exit, because the SIGINT signal is captured by Qemu now. :(
> 
> What's your opinion? Thanks.

Confirmed, you cannot interrupt a running qemu from gdb anymore.

I think this patch has to be reverted and the original issue fixed by
making qemu_shutdown_requested signal-safe. Should be simple, just
convert shutdown_requested into a counter. Need not be atomic, all
happens over the main thread anyway.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]