[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN |
Date: |
Wed, 12 Apr 2017 15:52:44 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 04/12/2017 06:02 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>>> ---
>>> 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, Paolo Bonzini, 2017/04/13
Re: [Qemu-devel] [PATCH] event: Add signal information to SHUTDOWN, Paolo Bonzini, 2017/04/13