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:

> 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.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution
> v3: new patch
> ---
>  qapi-schema.json        | 23 +++++++++++++++++++++++
>  include/sysemu/sysemu.h |  2 +-
>  vl.c                    | 44 ++++++++++++++++++++++++++++----------------
>  hw/i386/xen/xen-hvm.c   |  9 ++++++---
>  migration/colo.c        |  2 +-
>  migration/savevm.c      |  2 +-
>  6 files changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 01b087f..a4ebdd1 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2304,6 +2304,29 @@
>  { 'command': 'system_powerdown' }
>
>  ##
> +# @ShutdownCause:
> +#
> +# Enumeration of various causes for shutdown.
> +#
> +# @host-qmp: Reaction to a QMP command, such as 'quit'
> +# @host-signal: Reaction to a signal, such as SIGINT
> +# @host-ui: Reaction to a UI event, such as closing the window
> +# @host-replay: The host is replaying an earlier shutdown event
> +# @host-error: Qemu encountered an error that prevents further use of the guest
> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or
> +#                  other hardware-specific action
> +# @guest-reset: The guest requested a reset, and the command line
> +#               response to a reset is to instead trigger a shutdown
> +# @guest-panic: The guest panicked, and the command line response to
> +#               a panic is to trigger a shutdown
> +#
> +# Since: 2.10
> +##
> +{ 'enum': 'ShutdownCause',
> +  'data': [ 'host-qmp', 'host-signal', 'host-ui', 'host-replay', 'host-error',
> +            'guest-shutdown', 'guest-reset', 'guest-panic' ] }
> +
> +##
>  # @cpu:
>  #
>  # This command is a nop that is only provided for the purposes of compatibility.
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 16175f7..00a907f 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -65,7 +65,7 @@ bool qemu_vmstop_requested(RunState *r);
>  int qemu_shutdown_requested_get(void);
>  int qemu_reset_requested_get(void);
>  void qemu_system_killed(int signal, pid_t pid);
> -void qemu_system_reset(bool report);
> +void qemu_system_reset(bool report, int reason);
>  void qemu_system_guest_panicked(GuestPanicInformation *info);
>  size_t qemu_target_page_size(void);
>
> diff --git a/vl.c b/vl.c
> index 879786a..2b95b7f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1597,8 +1597,8 @@ void vm_state_notify(int running, RunState state)
>      }
>  }
>
> -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".

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.

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.

>  static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
> @@ -1624,7 +1624,7 @@ int qemu_reset_requested_get(void)
>
>  static int qemu_shutdown_requested(void)
>  {
> -    return atomic_xchg(&shutdown_requested, 0);
> +    return atomic_xchg(&shutdown_requested, -1);
>  }

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.

>
>  static void qemu_kill_report(void)
> @@ -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.

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

>  {
>      MachineClass *mc;
>
> @@ -1700,6 +1705,7 @@ void qemu_system_reset(bool report)
>          qemu_devices_reset();
>      }
>      if (report) {
> +        assert(reason >= 0);
>          qapi_event_send_reset(&error_abort);
>      }
>      cpu_synchronize_all_post_reset();
> @@ -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?

> +        shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET;
>      } else {
> -        reset_requested = 1;
> +        reset_requested = SHUTDOWN_CAUSE_GUEST_RESET;
>      }
>      cpu_stop_current();
>      qemu_notify_event();
> @@ -1807,7 +1814,7 @@ void qemu_system_killed(int signal, pid_t pid)
>      /* Cannot call qemu_system_shutdown_request directly because
>       * we are in a signal handler.
>       */
> -    shutdown_requested = 1;
> +    shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL;
>      qemu_notify_event();
>  }
>
> @@ -1815,7 +1822,8 @@ void qemu_system_shutdown_request(void)
>  {
>      trace_qemu_system_shutdown_request();
>      replay_shutdown_request();
> -    shutdown_requested = 1;
> +    /* FIXME - add a parameter to allow callers to specify reason */

Likewise.

> +    shutdown_requested = SHUTDOWN_CAUSE_GUEST_SHUTDOWN;
>      qemu_notify_event();
>  }
>
> @@ -1846,13 +1854,16 @@ void qemu_system_debug_request(void)
>  static bool main_loop_should_exit(void)
>  {
>      RunState r;
> +    int request;
> +
>      if (qemu_debug_requested()) {
>          vm_stop(RUN_STATE_DEBUG);
>      }
>      if (qemu_suspend_requested()) {
>          qemu_system_suspend();
>      }
> -    if (qemu_shutdown_requested()) {
> +    request = qemu_shutdown_requested();
> +    if (request >= 0) {
>          qemu_kill_report();
>          qapi_event_send_shutdown(&error_abort);
>          if (no_shutdown) {
> @@ -1861,9 +1872,10 @@ static bool main_loop_should_exit(void)
>              return true;
>          }
>      }
> -    if (qemu_reset_requested()) {
> +    request = qemu_reset_requested();
> +    if (request >= 0) {
>          pause_all_vcpus();
> -        qemu_system_reset(VMRESET_REPORT);
> +        qemu_system_reset(VMRESET_REPORT, request);
>          resume_all_vcpus();
>          if (!runstate_check(RUN_STATE_RUNNING) &&
>                  !runstate_check(RUN_STATE_INMIGRATE)) {
> @@ -1872,7 +1884,7 @@ static bool main_loop_should_exit(void)
>      }
>      if (qemu_wakeup_requested()) {
>          pause_all_vcpus();
> -        qemu_system_reset(VMRESET_SILENT);
> +        qemu_system_reset(VMRESET_SILENT, -1);
>          notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
>          wakeup_reason = QEMU_WAKEUP_REASON_NONE;
>          resume_all_vcpus();
> @@ -4684,7 +4696,7 @@ int main(int argc, char **argv, char **envp)
>         reading from the other reads, because timer polling functions query
>         clock values from the log. */
>      replay_checkpoint(CHECKPOINT_RESET);
> -    qemu_system_reset(VMRESET_SILENT);
> +    qemu_system_reset(VMRESET_SILENT, -1);
>      register_global_state();
>      if (replay_mode != REPLAY_MODE_NONE) {
>          replay_vmstate_init();
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index b1c05ff..3a6484c 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1089,11 +1089,14 @@ static void cpu_handle_ioreq(void *opaque)
>           * causes Xen to powerdown the domain.
>           */
>          if (runstate_is_running()) {
> -            if (qemu_shutdown_requested_get()) {
> +            int request;
> +
> +            if (qemu_shutdown_requested_get() >= 0) {
>                  destroy_hvm_domain(false);
>              }
> -            if (qemu_reset_requested_get()) {
> -                qemu_system_reset(VMRESET_REPORT);
> +            request = qemu_reset_requested_get();
> +            if (request >= 0) {
> +                qemu_system_reset(VMRESET_REPORT, request);
>                  destroy_hvm_domain(true);
>              }
>          }
> diff --git a/migration/colo.c b/migration/colo.c
> index c19eb3f..17a5482 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -611,7 +611,7 @@ void *colo_process_incoming_thread(void *opaque)
>          }
>
>          qemu_mutex_lock_iothread();
> -        qemu_system_reset(VMRESET_SILENT);
> +        qemu_system_reset(VMRESET_SILENT, -1);
>          vmstate_loading = true;
>          if (qemu_loadvm_state(fb) < 0) {
>              error_report("COLO: loadvm failed");
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 03ae1bd..dcbaf00 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2292,7 +2292,7 @@ int load_vmstate(const char *name)
>          return -EINVAL;
>      }
>
> -    qemu_system_reset(VMRESET_SILENT);
> +    qemu_system_reset(VMRESET_SILENT, -1);
>      mis->from_src_file = f;
>
>      aio_context_acquire(aio_context);