[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v7 3/5] shutdown: Add source informat
From: |
Eric Blake |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v7 3/5] shutdown: Add source information to SHUTDOWN and RESET |
Date: |
Tue, 9 May 2017 09:07:26 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
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