[Bug,1217339] Unix signal to send ACPI-shutdown to Guest

Commit Message

Simon March 15, 2017, 5:46 p.m. UTC

Daniel P. Berrange:
> 
> While I understand your motivation this creates a semantic change for
> existing users of QEMU. IOW anyone who is currently relying on use
> of SIGHUP will experiance a regression when upgrading QEMU.
> 
> So if we want to signal to generate a clean shutdown, we need to pick
> one that QEMU hasn't already set a specific behaviour for.
> 
> SIGQUIT could be a valid option, or the super generic SIGUSR1
> 
> Regards,
> Daniel

Thanks for your answer Daniel.

OK for not using SIGHUP and keep SIGTERM, SIGINT and SIGHUP to have the
same behavior.

SIGQUIT is reserved for core files generation.

SIGUSR1 is already used in 'util/qemu-progress.c' to trigger a report
on ongoing jobs, so it does not seem usable.

SIGUSR2 is temporarily used in 'util/coroutine-sigaltstack.c' which
takes care however to preserve the original handler. I did not saw
any other place where it is used, so it seems to be a better candidate.

Here is a second version of my patch using SIGUSR2 to cleanly power off
the guest:

Signed-off-by: Simon Geusebroek <[e-mail]>
---
--

Comments

Peter Maydell March 15, 2017, 6 p.m. UTC | #1

On 15 March 2017 at 17:46, Simon <[e-mail]> wrote:
> OK for not using SIGHUP and keep SIGTERM, SIGINT and SIGHUP to have the
> same behavior.
>
> SIGQUIT is reserved for core files generation.
>
> SIGUSR1 is already used in 'util/qemu-progress.c' to trigger a report
> on ongoing jobs, so it does not seem usable.
>
> SIGUSR2 is temporarily used in 'util/coroutine-sigaltstack.c' which
> takes care however to preserve the original handler. I did not saw
> any other place where it is used, so it seems to be a better candidate.

I don't think we can use SIGUSR2 here -- as you say, it's used
in the sigaltstack version of coroutines, and so there would
be races if the user tried to use SIGUSR2 to power down the
machine while we happened to be starting a new coroutine.

SIGUSR1 is also no good, as it is used by main-loop.c as
the SIG_IPI.

thanks
-- PMM

Daniel P. Berrangé March 15, 2017, 6:08 p.m. UTC | #2

On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
> On 15 March 2017 at 17:46, Simon <[e-mail]> wrote:
> > OK for not using SIGHUP and keep SIGTERM, SIGINT and SIGHUP to have the
> > same behavior.
> >
> > SIGQUIT is reserved for core files generation.
> >
> > SIGUSR1 is already used in 'util/qemu-progress.c' to trigger a report
> > on ongoing jobs, so it does not seem usable.
> >
> > SIGUSR2 is temporarily used in 'util/coroutine-sigaltstack.c' which
> > takes care however to preserve the original handler. I did not saw
> > any other place where it is used, so it seems to be a better candidate.
> 
> I don't think we can use SIGUSR2 here -- as you say, it's used
> in the sigaltstack version of coroutines, and so there would
> be races if the user tried to use SIGUSR2 to power down the
> machine while we happened to be starting a new coroutine.
> 
> SIGUSR1 is also no good, as it is used by main-loop.c as
> the SIG_IPI.

Which means we'd be into the realm of having to pick  SIGRTMIN + N for
some arbitrary N >= 0


Regards,
Daniel

Peter Maydell March 15, 2017, 6:45 p.m. UTC | #3

On 15 March 2017 at 18:08, Daniel P. Berrange <[e-mail]> wrote:
> On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
>> On 15 March 2017 at 17:46, Simon <[e-mail]> wrote:
>> > OK for not using SIGHUP and keep SIGTERM, SIGINT and SIGHUP to have the
>> > same behavior.
>> >
>> > SIGQUIT is reserved for core files generation.
>> >
>> > SIGUSR1 is already used in 'util/qemu-progress.c' to trigger a report
>> > on ongoing jobs, so it does not seem usable.
>> >
>> > SIGUSR2 is temporarily used in 'util/coroutine-sigaltstack.c' which
>> > takes care however to preserve the original handler. I did not saw
>> > any other place where it is used, so it seems to be a better candidate.
>>
>> I don't think we can use SIGUSR2 here -- as you say, it's used
>> in the sigaltstack version of coroutines, and so there would
>> be races if the user tried to use SIGUSR2 to power down the
>> machine while we happened to be starting a new coroutine.
>>
>> SIGUSR1 is also no good, as it is used by main-loop.c as
>> the SIG_IPI.
>
> Which means we'd be into the realm of having to pick  SIGRTMIN + N for
> some arbitrary N >= 0

That's pretty nasty. Why can't we use SIGHUP, again?

thanks
-- PMM

Simon March 16, 2017, 10:25 a.m. UTC | #4

Hi Peter,

> Why can't we use SIGHUP, again?

I suppose for all those people who use non ACPI-aware guests in non
daemonized Qemu instances and usually close their terminal without
stopping Qemu first on the assumption that Qemu will stop itself
automatically: this wouldn't work anymore...

More seriously, Qemu is a very versatile piece of software. I can only
rely on the word of experienced Qemu maintainers who have a better view
than me on the general Qemu user-base to determine whether a change of
SIGHUP handling will have a negative effect or not.

Using SIGHUP was my initial proposition and is the easy way to go.

In case it is determined that this would have too much side effects and
that no other signal is usable, the only other way I see is to add a new
command-line option to let users select Qemu behavior.

Regards,
Simon.

Daniel P. Berrangé March 16, 2017, 10:26 a.m. UTC | #5

On Wed, Mar 15, 2017 at 06:45:57PM +0000, Peter Maydell wrote:
> On 15 March 2017 at 18:08, Daniel P. Berrange <[e-mail]> wrote:
> > On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
> >> On 15 March 2017 at 17:46, Simon <[e-mail]> wrote:
> >> > OK for not using SIGHUP and keep SIGTERM, SIGINT and SIGHUP to have the
> >> > same behavior.
> >> >
> >> > SIGQUIT is reserved for core files generation.
> >> >
> >> > SIGUSR1 is already used in 'util/qemu-progress.c' to trigger a report
> >> > on ongoing jobs, so it does not seem usable.
> >> >
> >> > SIGUSR2 is temporarily used in 'util/coroutine-sigaltstack.c' which
> >> > takes care however to preserve the original handler. I did not saw
> >> > any other place where it is used, so it seems to be a better candidate.
> >>
> >> I don't think we can use SIGUSR2 here -- as you say, it's used
> >> in the sigaltstack version of coroutines, and so there would
> >> be races if the user tried to use SIGUSR2 to power down the
> >> machine while we happened to be starting a new coroutine.
> >>
> >> SIGUSR1 is also no good, as it is used by main-loop.c as
> >> the SIG_IPI.
> >
> > Which means we'd be into the realm of having to pick  SIGRTMIN + N for
> > some arbitrary N >= 0
> 
> That's pretty nasty. Why can't we use SIGHUP, again?

Because QEMU already designate that as doing an immediate stop - ie it'll
allow QEMU block layer to flush pending I/O, but it will not wait for the
guest to shutdown.  If we change that behaviour we'll break anyone who
is already relying on SIGHUP - qemu might never exit at all if the guest
ignores the ACPI request


Regards,
Daniel

Patch

diff -ru qemu-2.8.0/os-posix.c qemu-new/os-posix.c
--- qemu-2.8.0/os-posix.c	2016-12-20 21:16:48.000000000 +0100
+++ qemu-new/os-posix.c	2017-03-15 17:45:28.290737575 +0100
@@ -72,6 +72,7 @@ 
      sigaction(SIGINT,  &act, NULL);
      sigaction(SIGHUP,  &act, NULL);
      sigaction(SIGTERM, &act, NULL);
+    sigaction(SIGUSR2, &act, NULL);
  }

  /* Find a likely location for support files using the location of the 
binary.
diff -ru qemu-2.8.0/vl.c qemu-new/vl.c
--- qemu-2.8.0/vl.c	2016-12-20 21:16:54.000000000 +0100
+++ qemu-new/vl.c	2017-03-15 17:45:20.866737459 +0100
@@ -1871,7 +1871,11 @@ 
      /* Cannot call qemu_system_shutdown_request directly because
       * we are in a signal handler.
       */
-    shutdown_requested = 1;
+    if (signal == SIGUSR2) {
+        powerdown_requested = 1;
+    } else {
+        shutdown_requested = 1;
+    }
      qemu_notify_event();
  }