[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 05/09/2017 06:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@xxxxxxxxxx> 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

It was easy to do
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 <armbru@xxxxxxxxxx>

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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.