[Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Simon]

Hello,

Here is a short patch answering to Qemu wish-list issue 1217339.
<https://bugs.launchpad.net/qemu/+bug/1217339>
It makes Qemu to cleanly power off the guest when receiving a SIGHUP
signal, thus without requiring any monitor access which is currently
impossible (AFAIK).

The original issue mentions using SIGQUIT in its title, however as
Laszlo explained in the bug thread SIGQUIT is designed for debugging
purposes and is less about quitting than generating a core file. The
original request also explicitly mentioned that:

> If, for some reason SIGQUIT would not be accepted as the signal, take
> any free to use signal, like USR1.

Qemu currently maps SIGTERM, SIGINT and SIGHUP to a brutal shutdown of
the guest. This patch does not alter Qemu behavior for SIGTERM and
SIGQUIT, and maps SIGHUP to the clean power off of the guest.

IMHO SIGTERM and SIGINT behavior should not be altered:

- SIGTERM is the preferred alternative to a SIGKILL to shutdown a stuck
   guest.
- SIGINT is mostly a matter of user expectation as it handles the Ctrl-C
   sequence, I suppose that most users expect a brutal shutdown as it is
   now but this is just a random guess.

SIGHUP is a more versatile signal, also used for instance to tell a
daemon to reload its configuration files. From a functional point-of-
view, for me it makes sense to use a "hang-up" signal to power off a
guest, more than an impersonal USR1 signal, but this remains easily
changeable would SIGHUP not be suitable for this purpose.

With this patch applied, it becomes possible to easily and cleanly shut-
down Qemu virtual machines system-wide without involving any monitor:

1. Send SIGHUP to all Qemu processes so the guests power off cleanly.
2. After a few time send SIGTERM to the remaining Qemu processes to
    forcefully close stuck guests.
3. After a few time send SIGKILL to the remaining Qemu processes to
    forcefully close stuck Qemu hypervisor processes.

I find this more convenient than having to tinker with Qemu monitor to
implement step 1 as it must currently be done.

Signed-off-by: Simon Geusebroek <qemu.bugs@whitewinterwolf.com>
---
diff -ur a/vl.c b/vl.c
--- a/vl.c	2016-12-20 21:16:54.000000000 +0100
+++ b/vl.c	2017-03-14 16:02:51.959911847 +0100
@@ -1871,7 +1871,11 @@
      /* Cannot call qemu_system_shutdown_request directly because
       * we are in a signal handler.
       */
-    shutdown_requested = 1;
+    if (signal == SIGHUP) {
+        powerdown_requested = 1;
+    } else {
+        shutdown_requested = 1;
+    }
      qemu_notify_event();
  }

--

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Daniel P. Berrange]

On Wed, Mar 15, 2017 at 02:45:57PM +0100, Simon wrote:
> Hello,
> 
> Here is a short patch answering to Qemu wish-list issue 1217339.
> <https://bugs.launchpad.net/qemu/+bug/1217339>
> It makes Qemu to cleanly power off the guest when receiving a SIGHUP
> signal, thus without requiring any monitor access which is currently
> impossible (AFAIK).
> 
> The original issue mentions using SIGQUIT in its title, however as
> Laszlo explained in the bug thread SIGQUIT is designed for debugging
> purposes and is less about quitting than generating a core file. The
> original request also explicitly mentioned that:
> 
> > If, for some reason SIGQUIT would not be accepted as the signal, take
> > any free to use signal, like USR1.
> 
> Qemu currently maps SIGTERM, SIGINT and SIGHUP to a brutal shutdown of
> the guest. This patch does not alter Qemu behavior for SIGTERM and
> SIGQUIT, and maps SIGHUP to the clean power off of the guest.
> 
> IMHO SIGTERM and SIGINT behavior should not be altered:
> 
> - SIGTERM is the preferred alternative to a SIGKILL to shutdown a stuck
>   guest.
> - SIGINT is mostly a matter of user expectation as it handles the Ctrl-C
>   sequence, I suppose that most users expect a brutal shutdown as it is
>   now but this is just a random guess.
> 
> SIGHUP is a more versatile signal, also used for instance to tell a
> daemon to reload its configuration files. From a functional point-of-
> view, for me it makes sense to use a "hang-up" signal to power off a
> guest, more than an impersonal USR1 signal, but this remains easily
> changeable would SIGHUP not be suitable for this purpose.
> 
> With this patch applied, it becomes possible to easily and cleanly shut-
> down Qemu virtual machines system-wide without involving any monitor:
> 
> 1. Send SIGHUP to all Qemu processes so the guests power off cleanly.
> 2. After a few time send SIGTERM to the remaining Qemu processes to
>    forcefully close stuck guests.
> 3. After a few time send SIGKILL to the remaining Qemu processes to
>    forcefully close stuck Qemu hypervisor processes.
> 
> I find this more convenient than having to tinker with Qemu monitor to
> implement step 1 as it must currently be done.
> 
> Signed-off-by: Simon Geusebroek <qemu.bugs@whitewinterwolf.com>
> ---
> diff -ur a/vl.c b/vl.c
> --- a/vl.c	2016-12-20 21:16:54.000000000 +0100
> +++ b/vl.c	2017-03-14 16:02:51.959911847 +0100
> @@ -1871,7 +1871,11 @@
>      /* Cannot call qemu_system_shutdown_request directly because
>       * we are in a signal handler.
>       */
> -    shutdown_requested = 1;
> +    if (signal == SIGHUP) {
> +        powerdown_requested = 1;
> +    } else {
> +        shutdown_requested = 1;
> +    }
>      qemu_notify_event();
>  }

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Simon]

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 <qemu.bugs@whitewinterwolf.com>
---
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();
  }

--

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Peter Maydell]

On 15 March 2017 at 17:46, Simon <qemu.bugs@whitewinterwolf.com> 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

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Daniel P. Berrange]

On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
> On 15 March 2017 at 17:46, Simon <qemu.bugs@whitewinterwolf.com> 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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Peter Maydell]

On 15 March 2017 at 18:08, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
>> On 15 March 2017 at 17:46, Simon <qemu.bugs@whitewinterwolf.com> 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

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Simon]

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.

Re: [Qemu-devel] [Bug 1217339] [PATCH] Unix signal to send ACPI-shutdown to Guest

[Daniel P. Berrange]

On Wed, Mar 15, 2017 at 06:45:57PM +0000, Peter Maydell wrote:
> On 15 March 2017 at 18:08, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Wed, Mar 15, 2017 at 06:00:40PM +0000, Peter Maydell wrote:
> >> On 15 March 2017 at 17:46, Simon <qemu.bugs@whitewinterwolf.com> 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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [Bug 1217339] [PATCH v2] Unix signal to send ACPI-shutdown to Guest

[Simon]

Hi,

The patch below adds the new command-line option `-powerdown' which
changes the behavior for both SIGHUP and SIGINT signals to cause a clean
power down of the guest using an ACPI shutdown request.

If this option is not used, the original behavior is kept.

Regards,
Simon.

Signed-off-by: Simon Geusebroek <qemu.bugs@whitewinterwolf.com>
---
diff -ru a/qemu-options.hx b/qemu-options.hx
--- a/qemu-options.hx	2016-12-20 21:16:49.000000000 +0100
+++ b/qemu-options.hx	2017-03-18 14:17:35.759915414 +0100
@@ -3322,6 +3322,19 @@
  disk image.
  ETEXI

+DEF("powerdown", 0, QEMU_OPTION_powerdown, \
+    "-powerdown    Cleanly power down the guest on SIGINT or SIGHUP\n", 
\
+    QEMU_ARCH_ALL)
+STEXI
+@item -powerdown
+@findex -powerdown
+Don't forcefully shutdown the guest upon SIGINT (Ctrl-C) or SIGHUP 
(terminal
+closure) signals, but instead attempt to cleanly stop them using an 
ACPI
+shutdown request.
+Non ACPI-aware and stuck guests will remain up, the SIGTERM signal will 
still
+forcefully shutdown them.
+ETEXI
+
  DEF("loadvm", HAS_ARG, QEMU_OPTION_loadvm, \
      "-loadvm [tag|id]\n" \
      "                start right away with a saved state (loadvm in 
monitor)\n",
diff -ru a/vl.c b/vl.c
--- a/vl.c	2016-12-20 21:16:54.000000000 +0100
+++ b/vl.c	2017-03-18 13:48:21.227967255 +0100
@@ -164,6 +164,7 @@
  int fd_bootchk = 1;
  static int no_reboot;
  int no_shutdown = 0;
+int powerdown = 0;
  int cursor_hide = 1;
  int graphic_rotate = 0;
  const char *watchdog;
@@ -1871,7 +1872,11 @@
      /* Cannot call qemu_system_shutdown_request directly because
       * we are in a signal handler.
       */
-    shutdown_requested = 1;
+    if (powerdown == 1 && (signal == SIGINT || signal == SIGHUP)) {
+        powerdown_requested = 1;
+    } else {
+        shutdown_requested = 1;
+    }
      qemu_notify_event();
  }

@@ -3794,6 +3799,9 @@
              case QEMU_OPTION_no_shutdown:
                  no_shutdown = 1;
                  break;
+            case QEMU_OPTION_powerdown:
+                powerdown = 1;
+                break;
              case QEMU_OPTION_show_cursor:
                  cursor_hide = 0;
                  break;
--

Re: [Qemu-devel] [Bug 1217339] [PATCH v2] Unix signal to send ACPI-shutdown to Guest

[Daniel P. Berrange]

On Sat, Mar 18, 2017 at 02:41:16PM +0100, Simon wrote:
> Hi,
> 
> The patch below adds the new command-line option `-powerdown' which
> changes the behavior for both SIGHUP and SIGINT signals to cause a clean
> power down of the guest using an ACPI shutdown request.

If you need to add command line arguments IMHO this feature becomes
much less interesting - just use the QEMU monitor instead of inventing
an adhoc way to achieve something the monitor already does. The value of
using a signal handler comes from the fact that it would be unconditionally
available no matter what command line args were added.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [Bug 1217339] [PATCH v2] Unix signal to send ACPI-shutdown to Guest

[Simon]

Daniel P. Berrange:
> On Sat, Mar 18, 2017 at 02:41:16PM +0100, Simon wrote:
>> Hi,
>> 
>> The patch below adds the new command-line option `-powerdown' which
>> changes the behavior for both SIGHUP and SIGINT signals to cause a 
>> clean
>> power down of the guest using an ACPI shutdown request.
> 
> If you need to add command line arguments IMHO this feature becomes
> much less interesting - just use the QEMU monitor instead of inventing
> an adhoc way to achieve something the monitor already does. The value 
> of
> using a signal handler comes from the fact that it would be 
> unconditionally
> available no matter what command line args were added.

Hi Daniel,

Thank you for taking the time to answer me.

I am currently using the monitor to achieve this functionality, and this
boils down into doing something similar to this:

--------- 8< ----------------
if ! type 'socat' >/dev/null 2>&1
then
     echo "You must install 'socat'." >&2
     exit 1
fi
files=$( ps -e -o args | awk '$1 ~ /qemu-system-/' \
     | sed 's/.* -monitor unix:\([^,]*\).*/\1/;t;s/.*//' )
IFS='
'
for f in $files
do
     echo 'system_powerdown' | socat "UNIX-CONNECT:${f}" -
done
--------- 8< ----------------

In other words I check that required software is properly installed,
parse `ps' output to extract the path to the monitor socket file when it
is available, and pipe the `system_powerdown' command into it to
cleanly shut the VM down.

As a matter of comparison, here the same functionality implemented using
Unix signals instead of the monitor socket files:

--------- 8< ----------------
pkill -HUP qemu-system-
--------- 8< ----------------

Maybe it is just my own opinion, but I find the second approach cleaner
and more reliable.

Regarding the fact that it would require a specific parameter to be
enabled, I already use several parameters on a systematic basis with
Qemu (enable KVM, the USB tablet, set the keyboard layout, etc.).
I don't see the fact of adding a supplementary one to this set as an
annoyance.

On the contrary, I love Qemu's versatility and all these parameters are
here just to let users personalize Qemu behavior to best suit the way
they use it.

Regards,
Simon.