Re: [Xen-devel] [Qemu-devel] [PATCH v5 2/4] shutdown: Prepare for use of an enum in reset/shutdown_request

[Eric Blake <eblake@xxxxxxxxxx>]

On 04/28/2017 09:42 AM, Markus Armbruster wrote:
> Eric Blake <eblake@xxxxxxxxxx> writes:
> 
>> We want to track why a guest was shutdown; in particular, being able
>> to tell the difference between a guest request (such as ACPI request)
>> and host request (such as SIGINT) will prove useful to libvirt.
>> Since all requests eventually end up changing shutdown_requested in
>> vl.c, the logical change is to make that value track the reason,
>> rather than its current 0/1 contents.
>>
>> Since command-line options control whether a reset request is turned
>> into a shutdown request instead, the same treatment is given to
>> reset_requested.
>>
>> This patch adds a QAPI enum ShutdownCause that describes reasons
>> that a shutdown can be requested, and changes qemu_system_reset() to
>> pass the reason through, although for now it is not reported.  The
>> next patch will actually wire things up to modify events to report
>> data based on the reason, and to pass the correct enum value in from
>> various call-sites that can trigger a reset/shutdown.  Since QAPI
>> generates enums starting at 0, it's easier if we use a different
>> number as our sentinel that no request has happened yet.  Most of
>> the changes are in vl.c, but xen was using things externally.
>>

>> -static int reset_requested;
>> -static int shutdown_requested, shutdown_signal;
>> +static int reset_requested = -1;
>> +static int shutdown_requested = -1, shutdown_signal;
> 
> Peeking ahead, I see that shutdown_requested and reset_requested take
> ShutdownCause values and -1.  The latter means "no shutdown requested".
> What about adding 'none' to ShutdownCause, with value 0, und use that
> instead of literal -1?  Would avoid the unusual "negative means false,
> non-negative means true".

Works nicely if the enum is internal-use only.  Gets a bit more awkward
if the enum is exposed to the end-user.

The fact that I let QAPI generate the enum in patch 3 is evidence that
I'm leaning towards exposing it to the end user (patch 4); if we want to
keep it internal-only, a better place for the enum might be in sysemu.h
(where we also have the weird '#define VMRESET_SILENT false' '#define
VMRESET_REPORT true' to name a boolean parameter).

> 
> PATCH 4 exposes ShutdownCause in event SHUTDOWN, and 'none' must not
> occur there.  However, if we ever add a query-shutdown to go with this
> event, we will need 'none' there.

So, query-shutdown would basically be: what is the last-reported
shutdown event (normally none, when the guest is still running; but if,
like libvirt, you start qemu -no-shutdown, it can then be the cause of
why we are in a shutdown/stopped state while waiting for final cleanup)?

How important/likely is such an event?  (Hmm, from libvirt's
perspective, events are usually reliable, but can be lost; if we can
restart libvirtd and reconnect to a qemu process that is hanging on to
life only because no one has cleaned it up yet, query-shutdown does seem
like a useful thing for libvirt to have at the time it reconnects to
that qemu process).

We could always include 'none' in the QAPI enum, then document in
'SHUTDOWN' and 'RESET' events that the cause will never be 'none'.  Doc
hacks like that feel a little unclean, but not so horrible as to be
unforgivable.

> 
> I'd be tempted to reshuffle declarations here, because shutdown_signal's
> int is a different one than reset_requested's and shutdown_requested,
> and the latter two's "negative means false, non-negative means true" is
> unusual enough to justify a comment.
...
> 
> Hmm.  In case we stick to literal -1: consider splitting this patch into
> a part that changes @shutdown_requested from zero/non-zero to
> negative/non-negative, and a part that uses ShutdownCause for the
> non-negative values.


You're definitely right that if the enum doesn't have a nice none=0
state, then reshuffling to the magic -1 as no request is awkward enough
to be done alone.

But part of the answer is also dependent on whether we want PATCH 4 or
not (or, as you brought up, the possibility of a query-shutdown command
with even more persistent storage of the last-reported event).


>> @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void)
>>  static int qemu_reset_requested(void)
>>  {
>>      int r = reset_requested;
>> -    if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
>> -        reset_requested = 0;
>> +    if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
>> +        reset_requested = -1;
>>          return r;
>>      }
>> -    return false;
>> +    return -1;
> 
> "return false" in a function returning int smells, good riddance.
> 

In one of my earlier drafts of the patch, I even tried to change the
return type from int to bool, but decided that keeping it as int worked
best (if I have to use the -1/cause dichotomy).  But you're also right
that with a 'none' value in the enum, I could directly return ShutdownCause.

>>  }
>>
>>  static int qemu_suspend_requested(void)
>> @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void)
>>      return r;
>>  }
>>
>> -void qemu_system_reset(bool report)
>> +/*
>> + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using
>> + * the @reason interpreted as ShutdownType for details.  Otherwise,
>> + * @report is VMRESET_SILENT and @reason is ignored.
>> + */
>> +void qemu_system_reset(bool report, int reason)
> 
> Why int reason and not ShutdownCause?  Hmm, peeking ahead, I see you
> pass -1 with VMRESET_SILENT.  Yet another place where you use int for
> type ShutdownCause + { -1 }.  Adding 'none' to ShutdownCause looks
> even more attractive to me now.

Yeah, it's getting to be that way to me to, even if it just means that I
may have volunteered myself into writing a query-shutdown QMP command.

>> @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation 
>> *info)
>>  void qemu_system_reset_request(void)
>>  {
>>      if (no_reboot) {
>> -        shutdown_requested = 1;
>> +        /* FIXME - add a parameter to allow callers to specify reason */
> 
> FIXME addressed in the next patch.  Mention in this one's commit
> message?

Sure. Something like "Mark a couple of places as FIXME where we have to
guess a value to use; a later patch will fix things to supply a correct
value".

> 
>> +        shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET;

I've also debated about splitting patch 3 into two parts: the event
member additions (affecting .json and vl.c) and the parameter additions
(affecting all other call-sites).  If I add the event parameter first,
then supplying a bogus value to the event means extra churn to qemu
iotests output files unless I change THIS line of code to guess
SHUTDOWN_CAUSE_HOST_QMP; the other option is to wire up parameter
passing first and event reporting last.

I'll wait for more inputs before respinning this series (I already did a
poor enough job slamming mailboxes by sending 3 iterations of the series
in one day).  As you mention, I'd still like to hear ideas for the
replay side of things, and I wouldn't mind if Dan has any ideas from the
libvirt/upper-layer stack usage side of things on the fate of patch 4.

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

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel