Re: [Qemu-arm] [Qemu-devel] [PATCH v7 3/5] shutdown: Add source information to SHUTDOWN and RESET

[Eric Blake]

On 05/09/2017 06:56 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Time to wire up all the call sites that request a shutdown or
>> reset to use the enum added in the previous patch.
>>
>> It would have been less churn to keep the common case with no
>> arguments as meaning guest-triggered, and only modified the
>> host-triggered code paths, via a wrapper function, but then we'd
>> still have to audit that I didn't miss any host-triggered spots;
>> changing the signature forces us to double-check that I correctly
>> categorized all callers.
>>
>> Since command line options can change whether a guest reset request
>> causes an actual reset vs. a shutdown, it's easy to also add the
>> information to reset requests.
>>
>> Replay adds a FIXME to preserve the cause across the replay stream,
>> that will be tackled in the next patch.
>>

>> @@ -569,7 +569,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t 
>> val)
>>          default:
>>              if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
>>                  qapi_event_send_suspend_disk(&error_abort);
>> -                qemu_system_shutdown_request();
>> +                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> 
> I'm fine with using SHUTDOWN_CAUSE_GUEST_SHUTDOWN for suspend, but have
> you considered SHUTDOWN_CAUSE_GUEST_SUSPEND?

It was easy to do
s/qemu_system_shutdown_request()/qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN)/
for all hw/ files.  Harder would be picking a difference between
_SHUTDOWN and a new _SUSPEND. I can do it if hardware owners want the
distinction; but remember that this series will intentionally NOT expose
that distinction to QMP, so I don't know how much it will buy us.


>>  void qmp_stop(Error **errp)
>> @@ -105,7 +105,7 @@ void qmp_stop(Error **errp)
>>
>>  void qmp_system_reset(Error **errp)
>>  {
>> -    qemu_system_reset_request();
>> +    qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> 
> This is the only place where we pass something other than
> SHUTDOWN_CAUSE_GUEST_RESET.  We could avoid churn the obvious way, but I
> guess having the churn eases patch review.  Okay.

Yes, and that was the comment I made in the commit message about
changing the signature everywhere instead of adding wrappers that make
the common case become the default.


>> +++ b/replay/replay.c
>> @@ -51,7 +51,8 @@ bool replay_next_event_is(int event)
>>          switch (replay_state.data_kind) {
>>          case EVENT_SHUTDOWN:
>>              replay_finish_event();
>> -            qemu_system_shutdown_request();
>> +            /* FIXME - store actual reason */
>> +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
> 
> The temporary replay breakage is no big deal.  Still, can we avoid it by
> extending replay first, using a dummy value like
> SHUTDOWN_CAUSE_HOST_ERROR until the real cause becomes available?  Not
> sure it's worth a respin, though.
> 
>>              break;
>>          default:
>>              /* clock, time_t, checkpoint and other events */
> [...]
> 
> Reviewed-by: Markus Armbruster <address@hidden>
> 

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

signature.asc
Description: OpenPGP digital signature