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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 04/28/2017 09:42 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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

Yes, unless you need the generated ShutdownCause_lookup[].

> (where we also have the weird '#define VMRESET_SILENT false' '#define
> VMRESET_REPORT true' to name a boolean parameter).

Some people believe such defines make code more readable, others hate
them.  Regardless, they're unusual in QEMU.  Unusual is best avoided.

>> 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)?

Sounds right.

> 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).

Rule of thumb: if we need an event, we probably need a query, too.

> We could always include 'none' in the QAPI enum, then document in
> 'SHUTDOWN' and 'RESET' events that the cause will never be 'none'.

Yes.

>                                                                     Doc
> hacks like that feel a little unclean, but not so horrible as to be
> unforgivable.

I wouldn't call it an unclean hack.  For me, it's coping with an
insufficiently expressive type system: we can't define ShutdownCause + {
'none' } as a supertype of ShutdownCause.

Even if we could, I'm not sure it would be worth the bother.

>> 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).

Yes.

>>> @@ -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.

The reward for doing good work is more work.

>>> @@ -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".

Works for me, provided the meaning of "value" is clear from context.

>> 
>>> +        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

Your v3 and v4 cost me no time, don't worry.

> 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.

Okay.