Re: [Qemu-devel] [PATCH 2/3] qapi: split host-qmp into quit and system-reset

[Markus Armbruster <armbru@redhat.com>]

Cc: Pavel to assess possible impact on replay.

Cc: Eric to give him a chance to correct misunderstandings of
ShutdownCause.

Dominik Csapak <d.csapak@proxmox.com> writes:

> it is interesting to know whether the shutdown cause was 'quit' or
> 'reset', especially when using --no-reboot

Are you sure it *is* interesting?  I suspect it *will be* only after the
next patch.

Please start your sentences with a capital letter and end them with
punctuation.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  qapi/run-state.json | 10 ++++++----
>  qmp.c               |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 883bed167c..c215b6ef83 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -68,7 +68,9 @@
>  #
>  # @host-error: An error prevented further use of guest
>  #
> -# @host-qmp: Reaction to a QMP command, like 'quit'
> +# @host-qmp-quit: Reaction to the QMP command 'quit'
> +#
> +# @host-qmp-system-reset: Reaction to the QMP command 'system_reset'
>  #
>  # @host-signal: Reaction to a signal, such as SIGINT
>  #
> @@ -88,9 +90,9 @@
>  #
>  ##
>  { 'enum': 'ShutdownCause',
> -  'data': [ 'none', 'host-error', 'host-qmp', 'host-signal', 'host-ui',
> -            'guest-shutdown', 'guest-reset', 'guest-panic',
> -            'subsystem-reset'] }
> +  'data': [ 'none', 'host-error', 'host-qmp-quit', 'host-qmp-system-reset',
> +            'host-signal', 'host-ui', 'guest-shutdown', 'guest-reset',
> +            'guest-panic', 'subsystem-reset'] }
>  
>  ##
>  # @StatusInfo:
> diff --git a/qmp.c b/qmp.c
> index e7c0a2fd60..82298f6cb0 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -88,7 +88,7 @@ UuidInfo *qmp_query_uuid(Error **errp)
>  void qmp_quit(Error **errp)
>  {
>      no_shutdown = 0;
> -    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_QMP);
> +    qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_QMP_QUIT);
>  }
>  
>  void qmp_stop(Error **errp)
> @@ -109,7 +109,7 @@ void qmp_stop(Error **errp)
>  
>  void qmp_system_reset(Error **errp)
>  {
> -    qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP);
> +    qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET);
>  }
>  
>  void qmp_system_powerdown(Error **erp)

Let's see how these guys are used.

qemu_system_shutdown_request() and qemu_system_reset_request() put their
argument in @shutdown_requested or @reset_requested.  There's some
replay magic going on in qemu_system_shutdown_request().

main_loop_should_exit() retrieves them with qemu_shutdown_requested()
and qemu_reset_requested().  @shutdown_requested is only passed to
shutdown_caused_by_guest().  @reset_requested is only passed to
qemu_system_reset().  None of these functions is affected by your
change.  There's some replay magic going on in
qemu_system_reset_requested().

Xen's cpu_handle_ioreq() retrieves them with
qemu_shutdown_requested_get() and qemu_reset_requested_get().  None of
these functions is affected by your change.

Looks like ShutdownCause is overengineered[*]: we're only ever
interested in none, host, guest.  Your PATCH 3 will change that.  Okay,
but your commit message is misleading: this patch has no interesting
effect now.  The change becomes visible only after PATCH 3.

I'd swap PATCH 2 and 3, because that would make writing non-misleading
commit messages easier for me.


[*] Goes back to Eric's commit 7af88279e49..08fba7ac9b6, which were
surely done for a reason.  Perhaps I'm just confused.