[Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Eric Blake]

qemu_kill_report() is already able to tell whether a shutdown
was triggered by guest action (no output) or by a host signal
(a message about termination is printed via error_report); but
this information is then lost.  Libvirt would like to be able
to distinguish between a SHUTDOWN event triggered solely by
guest request and one triggered by a SIGTERM on the host.

Enhance the SHUTDOWN event to pass the value of shutdown_signal
through to the monitor client, suitably remapped into a
platform-neutral string.  Note that mingw lacks decent signal
support, and will never report a signal because it never calls
qemu_system_killed().

See also https://bugzilla.redhat.com/1384007

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/event.json | 20 +++++++++++++++++++-
 vl.c            | 21 ++++++++++++++++++---
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index e80f3f4..6aad475 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -5,11 +5,29 @@
 ##

 ##
+# @ShutdownSignal:
+#
+# The list of host signal types known to cause qemu to shut down a guest.
+#
+# @int: SIGINT
+# @hup: SIGHUP
+# @term: SIGTERM
+#
+# Since: 2.10
+##
+{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }
+
+##
 # @SHUTDOWN:
 #
 # Emitted when the virtual machine has shut down, indicating that qemu is
 # about to exit.
 #
+# @signal: If present, the shutdown was (probably) triggered due to
+# the receipt of the given signal in the host, rather than by a guest
+# action (note that there is an inherent race with a guest choosing to
+# shut down near the same time the host sends a signal). (since 2.10)
+#
 # Note: If the command-line option "-no-shutdown" has been specified, qemu will
 # not exit, and a STOP event will eventually follow the SHUTDOWN event
 #
@@ -21,7 +39,7 @@
 #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
 #
 ##
-{ 'event': 'SHUTDOWN' }
+{ 'event': 'SHUTDOWN', 'data': { '*signal': 'ShutdownSignal' } }

 ##
 # @POWERDOWN:
diff --git a/vl.c b/vl.c
index 0b4ed52..af29b2c 100644
--- a/vl.c
+++ b/vl.c
@@ -1626,9 +1626,23 @@ static int qemu_shutdown_requested(void)
     return atomic_xchg(&shutdown_requested, 0);
 }

-static void qemu_kill_report(void)
+static ShutdownSignal qemu_kill_report(void)
 {
+    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
     if (!qtest_driver() && shutdown_signal != -1) {
+        switch (shutdown_signal) {
+        case SIGINT:
+            ss = SHUTDOWN_SIGNAL_INT;
+            break;
+#ifdef SIGHUP
+        case SIGHUP:
+            ss = SHUTDOWN_SIGNAL_HUP;
+            break;
+#endif
+        case SIGTERM:
+            ss = SHUTDOWN_SIGNAL_TERM;
+            break;
+        }
         if (shutdown_pid == 0) {
             /* This happens for eg ^C at the terminal, so it's worth
              * avoiding printing an odd message in that case.
@@ -1644,6 +1658,7 @@ static void qemu_kill_report(void)
         }
         shutdown_signal = -1;
     }
+    return ss;
 }

 static int qemu_reset_requested(void)
@@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
         qemu_system_suspend();
     }
     if (qemu_shutdown_requested()) {
-        qemu_kill_report();
-        qapi_event_send_shutdown(&error_abort);
+        ShutdownSignal ss = qemu_kill_report();
+        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
         if (no_shutdown) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
-- 
2.9.3

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Daniel P. Berrange]

On Thu, Apr 06, 2017 at 04:09:17PM -0500, Eric Blake wrote:
> qemu_kill_report() is already able to tell whether a shutdown
> was triggered by guest action (no output) or by a host signal
> (a message about termination is printed via error_report); but
> this information is then lost.  Libvirt would like to be able
> to distinguish between a SHUTDOWN event triggered solely by
> guest request and one triggered by a SIGTERM on the host.
> 
> Enhance the SHUTDOWN event to pass the value of shutdown_signal
> through to the monitor client, suitably remapped into a
> platform-neutral string.  Note that mingw lacks decent signal
> support, and will never report a signal because it never calls
> qemu_system_killed().

Is it conceivable that we find a non-signal based way to distinguish
guest initiated shutdown, from host OS initiated kill when on Win32 ?

If so, rather than including a 'signal' field in the event, it might
be better to just have a 'source': 'host|guest' field. ie we would
indicate /who/ initiated the shutdown, rather than /how/ it was
initiated.

Or perhaps we should include both the who & the how ?

> See also https://bugzilla.redhat.com/1384007
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/event.json | 20 +++++++++++++++++++-
>  vl.c            | 21 ++++++++++++++++++---
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/event.json b/qapi/event.json
> index e80f3f4..6aad475 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -5,11 +5,29 @@
>  ##
> 
>  ##
> +# @ShutdownSignal:
> +#
> +# The list of host signal types known to cause qemu to shut down a guest.
> +#
> +# @int: SIGINT
> +# @hup: SIGHUP
> +# @term: SIGTERM
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }
> +
> +##
>  # @SHUTDOWN:
>  #
>  # Emitted when the virtual machine has shut down, indicating that qemu is
>  # about to exit.
>  #
> +# @signal: If present, the shutdown was (probably) triggered due to
> +# the receipt of the given signal in the host, rather than by a guest
> +# action (note that there is an inherent race with a guest choosing to
> +# shut down near the same time the host sends a signal). (since 2.10)
> +#
>  # Note: If the command-line option "-no-shutdown" has been specified, qemu will
>  # not exit, and a STOP event will eventually follow the SHUTDOWN event
>  #
> @@ -21,7 +39,7 @@
>  #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
>  #
>  ##
> -{ 'event': 'SHUTDOWN' }
> +{ 'event': 'SHUTDOWN', 'data': { '*signal': 'ShutdownSignal' } }
> 
>  ##
>  # @POWERDOWN:
> diff --git a/vl.c b/vl.c
> index 0b4ed52..af29b2c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1626,9 +1626,23 @@ static int qemu_shutdown_requested(void)
>      return atomic_xchg(&shutdown_requested, 0);
>  }
> 
> -static void qemu_kill_report(void)
> +static ShutdownSignal qemu_kill_report(void)
>  {
> +    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
>      if (!qtest_driver() && shutdown_signal != -1) {
> +        switch (shutdown_signal) {
> +        case SIGINT:
> +            ss = SHUTDOWN_SIGNAL_INT;
> +            break;
> +#ifdef SIGHUP
> +        case SIGHUP:
> +            ss = SHUTDOWN_SIGNAL_HUP;
> +            break;
> +#endif
> +        case SIGTERM:
> +            ss = SHUTDOWN_SIGNAL_TERM;
> +            break;
> +        }
>          if (shutdown_pid == 0) {
>              /* This happens for eg ^C at the terminal, so it's worth
>               * avoiding printing an odd message in that case.
> @@ -1644,6 +1658,7 @@ static void qemu_kill_report(void)
>          }
>          shutdown_signal = -1;
>      }
> +    return ss;
>  }
> 
>  static int qemu_reset_requested(void)
> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>          qemu_system_suspend();
>      }
>      if (qemu_shutdown_requested()) {
> -        qemu_kill_report();
> -        qapi_event_send_shutdown(&error_abort);
> +        ShutdownSignal ss = qemu_kill_report();
> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
>          if (no_shutdown) {
>              vm_stop(RUN_STATE_SHUTDOWN);
>          } else {

If any to the above question means we keep '*signal', then consider this

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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] [PATCH] event: Add signal information to SHUTDOWN

[Eric Blake]

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

On 04/07/2017 04:35 AM, Daniel P. Berrange wrote:
> On Thu, Apr 06, 2017 at 04:09:17PM -0500, Eric Blake wrote:
>> qemu_kill_report() is already able to tell whether a shutdown
>> was triggered by guest action (no output) or by a host signal
>> (a message about termination is printed via error_report); but
>> this information is then lost.  Libvirt would like to be able
>> to distinguish between a SHUTDOWN event triggered solely by
>> guest request and one triggered by a SIGTERM on the host.
>>
>> Enhance the SHUTDOWN event to pass the value of shutdown_signal
>> through to the monitor client, suitably remapped into a
>> platform-neutral string.  Note that mingw lacks decent signal
>> support, and will never report a signal because it never calls
>> qemu_system_killed().
> 
> Is it conceivable that we find a non-signal based way to distinguish
> guest initiated shutdown, from host OS initiated kill when on Win32 ?
> 
> If so, rather than including a 'signal' field in the event, it might
> be better to just have a 'source': 'host|guest' field. ie we would
> indicate /who/ initiated the shutdown, rather than /how/ it was
> initiated.
> 
> Or perhaps we should include both the who & the how ?

It's always something that can be extended later (possibly by even
adding an 'unknown' to the how, if there is no real signal name for
mingw).  The 'who' is kind of implied by the presence or absence of the
optional 'how', and even though mingw doesn't have SIGHUP, it DOES have
SIGINT (C requires that), so it is still probably that someone
interested enough could wire up Ctrl-C on mingw host to trigger 'int' as
the 'how', rather than the current situation of Ctrl-C presumably doing
nothing and 'how' never being reported.  (I haven't ever actually tried
to run a guest with mingw as the host, to know if Ctrl-C even works...)

>> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>>          qemu_system_suspend();
>>      }
>>      if (qemu_shutdown_requested()) {
>> -        qemu_kill_report();
>> -        qapi_event_send_shutdown(&error_abort);
>> +        ShutdownSignal ss = qemu_kill_report();
>> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
>>          if (no_shutdown) {
>>              vm_stop(RUN_STATE_SHUTDOWN);
>>          } else {
> 
> If any to the above question means we keep '*signal', then consider this
> 
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> 

I still think for Unix-y hosts that '*signal' is useful (some people DO
care whether it was SIGTERM vs. SIGHUP that caused qemu to shutdown,
especially if the two signals are sent by two different agents and can
therefore let the management distinguish which agent asked for the guest
to be killed).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> qemu_kill_report() is already able to tell whether a shutdown
> was triggered by guest action (no output) or by a host signal
> (a message about termination is printed via error_report); but
> this information is then lost.  Libvirt would like to be able
> to distinguish between a SHUTDOWN event triggered solely by
> guest request and one triggered by a SIGTERM on the host.
>
> Enhance the SHUTDOWN event to pass the value of shutdown_signal
> through to the monitor client, suitably remapped into a
> platform-neutral string.  Note that mingw lacks decent signal

I understand the desire to distinguish between guest-initiated and
host-initiated shutdown, but I'm not sure why libvirt (or anyone) would
care for the exact signal.  Can you explain?

>                           Note that mingw lacks decent signal
> support, and will never report a signal because it never calls
> qemu_system_killed().

Awkward.

os-posix.c arranges for SIGINT, SIGHUP and SIGTERM to be caught.  Here's
the handler:

    static void termsig_handler(int signal, siginfo_t *info, void *c)
    {
        qemu_system_killed(info->si_signo, info->si_pid);
    }

    void qemu_system_killed(int signal, pid_t pid)
    {
        shutdown_signal = signal;
        shutdown_pid = pid;
        no_shutdown = 0;

        /* Cannot call qemu_system_shutdown_request directly because
         * we are in a signal handler.
         */
        shutdown_requested = 1;
        qemu_notify_event();
    }

The variables are all int or pid_t.  Works in practice (pedants might
ask for sig_atomic_t, but I won't).

In other words, these three signals are polite requests to terminate
QEMU.

Stefan, are there equivalent requests under Windows?  I guess there
might be one at least for SIGINT, namely whatever happens when you hit
^C on the console.

Could we arrange to run qemu_system_killed() then?

If not, could we at least distinguish between guest-initiated and
host-initiated shutdown?

> See also https://bugzilla.redhat.com/1384007
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/event.json | 20 +++++++++++++++++++-
>  vl.c            | 21 ++++++++++++++++++---
>  2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/event.json b/qapi/event.json
> index e80f3f4..6aad475 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -5,11 +5,29 @@
>  ##
>
>  ##
> +# @ShutdownSignal:
> +#
> +# The list of host signal types known to cause qemu to shut down a guest.
> +#
> +# @int: SIGINT
> +# @hup: SIGHUP
> +# @term: SIGTERM
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }

I'd call them sigint, sighup, sigterm, but it's a matter of taste.

> +
> +##
>  # @SHUTDOWN:
>  #
>  # Emitted when the virtual machine has shut down, indicating that qemu is
>  # about to exit.
>  #
> +# @signal: If present, the shutdown was (probably) triggered due to
> +# the receipt of the given signal in the host, rather than by a guest
> +# action (note that there is an inherent race with a guest choosing to
> +# shut down near the same time the host sends a signal). (since 2.10)
> +#

Is the "(probably)" due to just Windows, or are there other reasons for
uncertainty?

>  # Note: If the command-line option "-no-shutdown" has been specified, qemu will
>  # not exit, and a STOP event will eventually follow the SHUTDOWN event
>  #
> @@ -21,7 +39,7 @@
>  #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
>  #
>  ##
> -{ 'event': 'SHUTDOWN' }
> +{ 'event': 'SHUTDOWN', 'data': { '*signal': 'ShutdownSignal' } }
>
>  ##
>  # @POWERDOWN:
> diff --git a/vl.c b/vl.c
> index 0b4ed52..af29b2c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1626,9 +1626,23 @@ static int qemu_shutdown_requested(void)
>      return atomic_xchg(&shutdown_requested, 0);
>  }
>
> -static void qemu_kill_report(void)
> +static ShutdownSignal qemu_kill_report(void)
>  {
> +    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
>      if (!qtest_driver() && shutdown_signal != -1) {

Outside this patch's scope: could just as well use 0 instead of -1, as 0
can't be a valid signal number (kill() uses it for "check if we could
kill").

> +        switch (shutdown_signal) {
> +        case SIGINT:
> +            ss = SHUTDOWN_SIGNAL_INT;
> +            break;
> +#ifdef SIGHUP
> +        case SIGHUP:
> +            ss = SHUTDOWN_SIGNAL_HUP;
> +            break;
> +#endif
> +        case SIGTERM:
> +            ss = SHUTDOWN_SIGNAL_TERM;
> +            break;
> +        }
>          if (shutdown_pid == 0) {
>              /* This happens for eg ^C at the terminal, so it's worth
>               * avoiding printing an odd message in that case.
> @@ -1644,6 +1658,7 @@ static void qemu_kill_report(void)
>          }
>          shutdown_signal = -1;
>      }
> +    return ss;
>  }
>
>  static int qemu_reset_requested(void)
> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>          qemu_system_suspend();
>      }
>      if (qemu_shutdown_requested()) {
> -        qemu_kill_report();
> -        qapi_event_send_shutdown(&error_abort);
> +        ShutdownSignal ss = qemu_kill_report();
> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
>          if (no_shutdown) {
>              vm_stop(RUN_STATE_SHUTDOWN);
>          } else {

Why not send the event within qemu_kill_report()?

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Daniel P. Berrange]

On Wed, Apr 12, 2017 at 01:02:02PM +0200, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > qemu_kill_report() is already able to tell whether a shutdown
> > was triggered by guest action (no output) or by a host signal
> > (a message about termination is printed via error_report); but
> > this information is then lost.  Libvirt would like to be able
> > to distinguish between a SHUTDOWN event triggered solely by
> > guest request and one triggered by a SIGTERM on the host.
> >
> > Enhance the SHUTDOWN event to pass the value of shutdown_signal
> > through to the monitor client, suitably remapped into a
> > platform-neutral string.  Note that mingw lacks decent signal
> 
> I understand the desire to distinguish between guest-initiated and
> host-initiated shutdown, but I'm not sure why libvirt (or anyone) would
> care for the exact signal.  Can you explain?

Libvirt doesn't care from a functional POV - it merely needs to distinguish
host vs guest. Any signal info would merely informational and thus logged.
So signal info would realistically only be used someone is trying to
troubleshoot some problem and needs to understand exactly what triggered
it.


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] [PATCH] event: Add signal information to SHUTDOWN

[Eric Blake]

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

On 04/12/2017 06:02 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> qemu_kill_report() is already able to tell whether a shutdown
>> was triggered by guest action (no output) or by a host signal
>> (a message about termination is printed via error_report); but
>> this information is then lost.  Libvirt would like to be able
>> to distinguish between a SHUTDOWN event triggered solely by
>> guest request and one triggered by a SIGTERM on the host.
>>
>> Enhance the SHUTDOWN event to pass the value of shutdown_signal
>> through to the monitor client, suitably remapped into a
>> platform-neutral string.  Note that mingw lacks decent signal
> 
> I understand the desire to distinguish between guest-initiated and
> host-initiated shutdown, but I'm not sure why libvirt (or anyone) would
> care for the exact signal.  Can you explain?

If we don't care about the signal itself, a simple boolean (host vs.
guest) is just as easy to code up.  Or even code up a boolean now, and
then add signal information down the road if someone has a use case for
it (as Dan said, libvirt doesn't care, but someone on top of libvirt
might - but I haven't identified such a user at this point in time).

> 
>>                           Note that mingw lacks decent signal
>> support, and will never report a signal because it never calls
>> qemu_system_killed().
> 
> Awkward.
> 

> In other words, these three signals are polite requests to terminate
> QEMU.
> 
> Stefan, are there equivalent requests under Windows?  I guess there
> might be one at least for SIGINT, namely whatever happens when you hit
> ^C on the console.

Mingw has SIGINT (C99 requires it), and that's presumably what happens
for ^C,...

> 
> Could we arrange to run qemu_system_killed() then?

...but I don't know why it is not currently wired up to call
qemu_system_killed(), nor do I have enough Windows programming expertise
to try and write such a patch. But I think that is an orthogonal
improvement.  On the other hand, mingw has a definition for SIGTERM (but
I'm not sure how it gets triggered) and no definition at all for SIGHUP
(as evidenced by the #ifdef'fery in the patch to get it to compile under
docker targetting mingw).

> 
> If not, could we at least distinguish between guest-initiated and
> host-initiated shutdown?
> 
>> See also https://bugzilla.redhat.com/1384007
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qapi/event.json | 20 +++++++++++++++++++-
>>  vl.c            | 21 ++++++++++++++++++---
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/event.json b/qapi/event.json
>> index e80f3f4..6aad475 100644
>> --- a/qapi/event.json
>> +++ b/qapi/event.json
>> @@ -5,11 +5,29 @@
>>  ##
>>
>>  ##
>> +# @ShutdownSignal:
>> +#
>> +# The list of host signal types known to cause qemu to shut down a guest.
>> +#
>> +# @int: SIGINT
>> +# @hup: SIGHUP
>> +# @term: SIGTERM
>> +#
>> +# Since: 2.10
>> +##
>> +{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }
> 
> I'd call them sigint, sighup, sigterm, but it's a matter of taste.

And it goes away if we are okay with a simpler bool of host vs. guest.

> 
>> +
>> +##
>>  # @SHUTDOWN:
>>  #
>>  # Emitted when the virtual machine has shut down, indicating that qemu is
>>  # about to exit.
>>  #
>> +# @signal: If present, the shutdown was (probably) triggered due to
>> +# the receipt of the given signal in the host, rather than by a guest
>> +# action (note that there is an inherent race with a guest choosing to
>> +# shut down near the same time the host sends a signal). (since 2.10)
>> +#
> 
> Is the "(probably)" due to just Windows, or are there other reasons for
> uncertainty?

There are other reasons too: a guest can request shutdown immediately
before the host sends SIGINT. Based on when things are processed, you
could see either the guest or the host as the initiator.  And the race
is not entirely implausible - when trying to shut down a guest, libvirt
first tries to inform the guest to initiate things (whether by interrupt
or guest agent), but after a given amount of time, assumes the guest is
unresponsive and resorts to a signal to qemu. A heavily loaded guest
that takes its time in responding could easily overlap with the timeout
resorting to a host-side action.


>> -static void qemu_kill_report(void)
>> +static ShutdownSignal qemu_kill_report(void)
>>  {
>> +    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
>>      if (!qtest_driver() && shutdown_signal != -1) {
> 
> Outside this patch's scope: could just as well use 0 instead of -1, as 0
> can't be a valid signal number (kill() uses it for "check if we could
> kill").

Indeed.

>> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>>          qemu_system_suspend();
>>      }
>>      if (qemu_shutdown_requested()) {
>> -        qemu_kill_report();
>> -        qapi_event_send_shutdown(&error_abort);
>> +        ShutdownSignal ss = qemu_kill_report();
>> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
>>          if (no_shutdown) {
>>              vm_stop(RUN_STATE_SHUTDOWN);
>>          } else {
> 
> Why not send the event within qemu_kill_report()?

Sure, I can do that in v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/12/2017 06:02 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> qemu_kill_report() is already able to tell whether a shutdown
>>> was triggered by guest action (no output) or by a host signal
>>> (a message about termination is printed via error_report); but
>>> this information is then lost.  Libvirt would like to be able
>>> to distinguish between a SHUTDOWN event triggered solely by
>>> guest request and one triggered by a SIGTERM on the host.
>>>
>>> Enhance the SHUTDOWN event to pass the value of shutdown_signal
>>> through to the monitor client, suitably remapped into a
>>> platform-neutral string.  Note that mingw lacks decent signal
>> 
>> I understand the desire to distinguish between guest-initiated and
>> host-initiated shutdown, but I'm not sure why libvirt (or anyone) would
>> care for the exact signal.  Can you explain?
>
> If we don't care about the signal itself, a simple boolean (host vs.
> guest) is just as easy to code up.  Or even code up a boolean now, and
> then add signal information down the road if someone has a use case for
> it (as Dan said, libvirt doesn't care, but someone on top of libvirt
> might - but I haven't identified such a user at this point in time).

Starting with just a boolean should be safe.  Especially attractive if
it lets us sidestep Windows complications; more on that below.

>> 
>>>                           Note that mingw lacks decent signal
>>> support, and will never report a signal because it never calls
>>> qemu_system_killed().
>> 
>> Awkward.
>> 
>
>> In other words, these three signals are polite requests to terminate
>> QEMU.
>> 
>> Stefan, are there equivalent requests under Windows?  I guess there
>> might be one at least for SIGINT, namely whatever happens when you hit
>> ^C on the console.
>
> Mingw has SIGINT (C99 requires it), and that's presumably what happens
> for ^C,...
>
>> 
>> Could we arrange to run qemu_system_killed() then?
>
> ...but I don't know why it is not currently wired up to call
> qemu_system_killed(), nor do I have enough Windows programming expertise
> to try and write such a patch. But I think that is an orthogonal
> improvement.  On the other hand, mingw has a definition for SIGTERM (but
> I'm not sure how it gets triggered) and no definition at all for SIGHUP
> (as evidenced by the #ifdef'fery in the patch to get it to compile under
> docker targetting mingw).

If all we need is distingishing host- and guest-initiated shutdown, then
detecting the latter reliably lets us stay away from OS-specific stuff.
Can we do that?

>> 
>> If not, could we at least distinguish between guest-initiated and
>> host-initiated shutdown?
>> 
>>> See also https://bugzilla.redhat.com/1384007
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  qapi/event.json | 20 +++++++++++++++++++-
>>>  vl.c            | 21 ++++++++++++++++++---
>>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/qapi/event.json b/qapi/event.json
>>> index e80f3f4..6aad475 100644
>>> --- a/qapi/event.json
>>> +++ b/qapi/event.json
>>> @@ -5,11 +5,29 @@
>>>  ##
>>>
>>>  ##
>>> +# @ShutdownSignal:
>>> +#
>>> +# The list of host signal types known to cause qemu to shut down a guest.
>>> +#
>>> +# @int: SIGINT
>>> +# @hup: SIGHUP
>>> +# @term: SIGTERM
>>> +#
>>> +# Since: 2.10
>>> +##
>>> +{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }
>> 
>> I'd call them sigint, sighup, sigterm, but it's a matter of taste.
>
> And it goes away if we are okay with a simpler bool of host vs. guest.
>
>> 
>>> +
>>> +##
>>>  # @SHUTDOWN:
>>>  #
>>>  # Emitted when the virtual machine has shut down, indicating that qemu is
>>>  # about to exit.
>>>  #
>>> +# @signal: If present, the shutdown was (probably) triggered due to
>>> +# the receipt of the given signal in the host, rather than by a guest
>>> +# action (note that there is an inherent race with a guest choosing to
>>> +# shut down near the same time the host sends a signal). (since 2.10)
>>> +#
>> 
>> Is the "(probably)" due to just Windows, or are there other reasons for
>> uncertainty?
>
> There are other reasons too: a guest can request shutdown immediately
> before the host sends SIGINT. Based on when things are processed, you
> could see either the guest or the host as the initiator.  And the race
> is not entirely implausible - when trying to shut down a guest, libvirt
> first tries to inform the guest to initiate things (whether by interrupt
> or guest agent), but after a given amount of time, assumes the guest is
> unresponsive and resorts to a signal to qemu. A heavily loaded guest
> that takes its time in responding could easily overlap with the timeout
> resorting to a host-side action.

This race doesn't worry me.  If both host and guest have initiated a
shutdown, then reporting whichever of the two finishes first seems fair.

Additional ways to terminate QEMU: HMP and QMP command "quit", and the
various GUI controls such "close SDL window".

>
>>> -static void qemu_kill_report(void)
>>> +static ShutdownSignal qemu_kill_report(void)
>>>  {
>>> +    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
>>>      if (!qtest_driver() && shutdown_signal != -1) {
>> 
>> Outside this patch's scope: could just as well use 0 instead of -1, as 0
>> can't be a valid signal number (kill() uses it for "check if we could
>> kill").
>
> Indeed.
>
>>> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>>>          qemu_system_suspend();
>>>      }
>>>      if (qemu_shutdown_requested()) {
>>> -        qemu_kill_report();
>>> -        qapi_event_send_shutdown(&error_abort);
>>> +        ShutdownSignal ss = qemu_kill_report();
>>> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);
>>>          if (no_shutdown) {
>>>              vm_stop(RUN_STATE_SHUTDOWN);
>>>          } else {
>> 
>> Why not send the event within qemu_kill_report()?
>
> Sure, I can do that in v2.

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Eric Blake]

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

On 04/12/2017 08:52 AM, Markus Armbruster wrote:
>>> In other words, these three signals are polite requests to terminate
>>> QEMU.
>>>
>>> Stefan, are there equivalent requests under Windows?  I guess there
>>> might be one at least for SIGINT, namely whatever happens when you hit
>>> ^C on the console.
>>
>> Mingw has SIGINT (C99 requires it), and that's presumably what happens
>> for ^C,...
>>
>>>
>>> Could we arrange to run qemu_system_killed() then?
>>
>> ...but I don't know why it is not currently wired up to call
>> qemu_system_killed(), nor do I have enough Windows programming expertise
>> to try and write such a patch. But I think that is an orthogonal
>> improvement.  On the other hand, mingw has a definition for SIGTERM (but
>> I'm not sure how it gets triggered) and no definition at all for SIGHUP
>> (as evidenced by the #ifdef'fery in the patch to get it to compile under
>> docker targetting mingw).
> 
> If all we need is distingishing host- and guest-initiated shutdown, then
> detecting the latter reliably lets us stay away from OS-specific stuff.
> Can we do that?

I'll simplify what I can; I still can't guarantee that mingw will be
setting the bool correctly in all cases, but setting a bool is easier
than trying to set a signal name.


>> There are other reasons too: a guest can request shutdown immediately
>> before the host sends SIGINT. Based on when things are processed, you
>> could see either the guest or the host as the initiator.  And the race
>> is not entirely implausible - when trying to shut down a guest, libvirt
>> first tries to inform the guest to initiate things (whether by interrupt
>> or guest agent), but after a given amount of time, assumes the guest is
>> unresponsive and resorts to a signal to qemu. A heavily loaded guest
>> that takes its time in responding could easily overlap with the timeout
>> resorting to a host-side action.
> 
> This race doesn't worry me.  If both host and guest have initiated a
> shutdown, then reporting whichever of the two finishes first seems fair.

So maybe I just tone down the docs and not even mention it.

> 
> Additional ways to terminate QEMU: HMP and QMP command "quit", and the
> various GUI controls such "close SDL window".

Good points. I have no idea what exit path those take (if they
raise(SIGINT) internally, it's quite easy - but if they go through some
other exit path, then I'll need to wire in something else).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/12/2017 08:52 AM, Markus Armbruster wrote:
>>>> In other words, these three signals are polite requests to terminate
>>>> QEMU.
>>>>
>>>> Stefan, are there equivalent requests under Windows?  I guess there
>>>> might be one at least for SIGINT, namely whatever happens when you hit
>>>> ^C on the console.
>>>
>>> Mingw has SIGINT (C99 requires it), and that's presumably what happens
>>> for ^C,...
>>>
>>>>
>>>> Could we arrange to run qemu_system_killed() then?
>>>
>>> ...but I don't know why it is not currently wired up to call
>>> qemu_system_killed(), nor do I have enough Windows programming expertise
>>> to try and write such a patch. But I think that is an orthogonal
>>> improvement.  On the other hand, mingw has a definition for SIGTERM (but
>>> I'm not sure how it gets triggered) and no definition at all for SIGHUP
>>> (as evidenced by the #ifdef'fery in the patch to get it to compile under
>>> docker targetting mingw).
>> 
>> If all we need is distingishing host- and guest-initiated shutdown, then
>> detecting the latter reliably lets us stay away from OS-specific stuff.
>> Can we do that?
>
> I'll simplify what I can; I still can't guarantee that mingw will be
> setting the bool correctly in all cases, but setting a bool is easier
> than trying to set a signal name.
>
>>> There are other reasons too: a guest can request shutdown immediately
>>> before the host sends SIGINT. Based on when things are processed, you
>>> could see either the guest or the host as the initiator.  And the race
>>> is not entirely implausible - when trying to shut down a guest, libvirt
>>> first tries to inform the guest to initiate things (whether by interrupt
>>> or guest agent), but after a given amount of time, assumes the guest is
>>> unresponsive and resorts to a signal to qemu. A heavily loaded guest
>>> that takes its time in responding could easily overlap with the timeout
>>> resorting to a host-side action.
>> 
>> This race doesn't worry me.  If both host and guest have initiated a
>> shutdown, then reporting whichever of the two finishes first seems fair.
>
> So maybe I just tone down the docs and not even mention it.

Works for me.

>> Additional ways to terminate QEMU: HMP and QMP command "quit", and the
>> various GUI controls such "close SDL window".
>
> Good points. I have no idea what exit path those take (if they
> raise(SIGINT) internally, it's quite easy - but if they go through some
> other exit path, then I'll need to wire in something else).

Chasing down all host-initiated terminations looks like a fool's errand
to me.  But if we can reliably detect guest-initiated, we don't have to,
do we?

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Eric Blake]

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

On 04/12/2017 09:33 AM, Markus Armbruster wrote:

>>> Additional ways to terminate QEMU: HMP and QMP command "quit", and the
>>> various GUI controls such "close SDL window".
>>
>> Good points. I have no idea what exit path those take (if they
>> raise(SIGINT) internally, it's quite easy - but if they go through some
>> other exit path, then I'll need to wire in something else).
> 
> Chasing down all host-initiated terminations looks like a fool's errand
> to me.  But if we can reliably detect guest-initiated, we don't have to,
> do we?

I don't know if we can reliably detect guest-initiated. Host-signal
initiated was easy because of the modification to a global variable.
But hopefully I'll find some easy common point, and maybe have to add
another global variable to track whether we went through that point (as
the reporting point is different than the request point).  At any rate,
this conversation is useful, to make sure I find the right place.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Paolo Bonzini]

On 07/04/2017 17:35, Daniel P. Berrange wrote:
> Is it conceivable that we find a non-signal based way to distinguish
> guest initiated shutdown, from host OS initiated kill when on Win32 ?

As far as I know there's no such thing as a "soft kill" (SIGTERM) on
Win32, since there is no kill function (only raise).

The only interprocess "signals" are Ctrl-C/Ctrl-Break events for the
console, which are mapped to SIGINT or SIGBREAK respectively (the
difference is that the application cannot trap Ctrl-Break, while it can
disable the default Ctrl-C handler).  Likewise there are close-window,
logoff and system shutdown events that the application can trap, but I'm
not sure how much they are worth reporting in the SHUTDOWN event and I'm
also not sure that you get them if you don't have a console, as is the
case for qemu-system-x86_64w.exe.

Paolo

> If so, rather than including a 'signal' field in the event, it might
> be better to just have a 'source': 'host|guest' field. ie we would
> indicate /who/ initiated the shutdown, rather than /how/ it was
> initiated.

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Paolo Bonzini]

On 07/04/2017 05:09, Eric Blake wrote:
>  ##
> +# @ShutdownSignal:
> +#
> +# The list of host signal types known to cause qemu to shut down a guest.
> +#
> +# @int: SIGINT
> +# @hup: SIGHUP
> +# @term: SIGTERM
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'ShutdownSignal', 'data': [ 'int', 'hup', 'term' ] }

I follow Markus's suggestion and would even make them uppercase.
Uncommon, but I think justified in this case.

> +##
>  # @SHUTDOWN:
>  #
>  # Emitted when the virtual machine has shut down, indicating that qemu is
>  # about to exit.
>  #
> +# @signal: If present, the shutdown was (probably) triggered due to
> +# the receipt of the given signal in the host, rather than by a guest
> +# action (note that there is an inherent race with a guest choosing to
> +# shut down near the same time the host sends a signal). (since 2.10)
> +#
>  # Note: If the command-line option "-no-shutdown" has been specified, qemu will
>  # not exit, and a STOP event will eventually follow the SHUTDOWN event
>  #
> @@ -21,7 +39,7 @@
>  #      "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
>  #
>  ##
> -{ 'event': 'SHUTDOWN' }
> +{ 'event': 'SHUTDOWN', 'data': { '*signal': 'ShutdownSignal' } }
> 
>  ##
>  # @POWERDOWN:
> diff --git a/vl.c b/vl.c
> index 0b4ed52..af29b2c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1626,9 +1626,23 @@ static int qemu_shutdown_requested(void)
>      return atomic_xchg(&shutdown_requested, 0);
>  }
> 
> -static void qemu_kill_report(void)
> +static ShutdownSignal qemu_kill_report(void)
>  {
> +    ShutdownSignal ss = SHUTDOWN_SIGNAL__MAX;
>      if (!qtest_driver() && shutdown_signal != -1) {
> +        switch (shutdown_signal) {
> +        case SIGINT:
> +            ss = SHUTDOWN_SIGNAL_INT;
> +            break;
> +#ifdef SIGHUP
> +        case SIGHUP:
> +            ss = SHUTDOWN_SIGNAL_HUP;
> +            break;
> +#endif
> +        case SIGTERM:
> +            ss = SHUTDOWN_SIGNAL_TERM;
> +            break;
> +        }
>          if (shutdown_pid == 0) {
>              /* This happens for eg ^C at the terminal, so it's worth
>               * avoiding printing an odd message in that case.
> @@ -1644,6 +1658,7 @@ static void qemu_kill_report(void)
>          }
>          shutdown_signal = -1;
>      }
> +    return ss;
>  }
> 
>  static int qemu_reset_requested(void)
> @@ -1852,8 +1867,8 @@ static bool main_loop_should_exit(void)
>          qemu_system_suspend();
>      }
>      if (qemu_shutdown_requested()) {
> -        qemu_kill_report();
> -        qapi_event_send_shutdown(&error_abort);
> +        ShutdownSignal ss = qemu_kill_report();
> +        qapi_event_send_shutdown(ss < SHUTDOWN_SIGNAL__MAX, ss, &error_abort);

Maybe call qapi_event_send_shutdown from qemu_kill_report?  It is not
obvious that ShutdownSignal is unsigned (except to you and Laszlo of
course).

I think this patch is fine for now; calling qemu_system_killed() from
os-win32.c is an orthogonal improvement.

Paolo

>          if (no_shutdown) {
>              vm_stop(RUN_STATE_SHUTDOWN);
>          } else {
> 

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Paolo Bonzini]

On 12/04/2017 19:02, Markus Armbruster wrote:
>>                           Note that mingw lacks decent signal
>> support, and will never report a signal because it never calls
>> qemu_system_killed().
> 
> Awkward.

ISO C trivia of the day: "kill" is only a POSIX function.

> os-posix.c arranges for SIGINT, SIGHUP and SIGTERM to be caught.  Here's
> the handler:
> 
>     static void termsig_handler(int signal, siginfo_t *info, void *c)
>     {
>         qemu_system_killed(info->si_signo, info->si_pid);
>     }
> 
>     void qemu_system_killed(int signal, pid_t pid)
>     {
>         shutdown_signal = signal;
>         shutdown_pid = pid;
>         no_shutdown = 0;
> 
>         /* Cannot call qemu_system_shutdown_request directly because
>          * we are in a signal handler.
>          */
>         shutdown_requested = 1;
>         qemu_notify_event();
>     }
> 
> The variables are all int or pid_t.  Works in practice (pedants might
> ask for sig_atomic_t, but I won't).
> 
> In other words, these three signals are polite requests to terminate
> QEMU.
> 
> Stefan, are there equivalent requests under Windows?  I guess there
> might be one at least for SIGINT, namely whatever happens when you hit
> ^C on the console.

SIGINT and SIGBREAK exist, so you can just use a termsig_handler which
passes 0 for the pid.

Paolo

Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/04/2017 05:09, Eric Blake wrote:
>
>> qemu_kill_report() is already able to tell whether a shutdown
>> was triggered by guest action (no output) or by a host signal
>> (a message about termination is printed via error_report); but
>> this information is then lost.  Libvirt would like to be able
>> to distinguish between a SHUTDOWN event triggered solely by
>> guest request and one triggered by a SIGTERM on the host.
>>
>> Enhance the SHUTDOWN event to pass the value of shutdown_signal
>> through to the monitor client, suitably remapped into a
>> platform-neutral string.  Note that mingw lacks decent signal
>> support, and will never report a signal because it never calls
>> qemu_system_killed().
>>
>> See also https://bugzilla.redhat.com/1384007
>> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> I think this patch is fine for now; calling qemu_system_killed() from
> os-win32.c is an orthogonal improvement.

Fair enough.  However, I think the patch misses its stated goal even on
POSIX hosts: to let libvirt distinguish host- and guest-initiated
shutdown.  Shutdown on signal is only one kind of host-initiated
shutdown.  All the others remain indistinguishable from guest-initiated
shutdown.

To reach the goal, we can track all host-initiated shutdowns, or all
guest-initiated shutdowns.  Pick the one that's easier to track.  The
former probably involves messing with OS-specific code, the latter
probably doesn't.