Re: [PATCH] runstate: cleanup reboot and panic actions

[Alejandro Jimenez]

On 1/20/2021 9:37 AM, Paolo Bonzini wrote:
> The possible choices for panic, reset and watchdog actions are inconsistent.
>
> "-action panic=poweroff" should be renamed to "-action panic=shutdown"
> on the command line.  This is because "-action panic=poweroff" and
> "-action watchdog=poweroff" have slightly different semantics, the first
> does an unorderly exit while the second goes through qemu_cleanup().  With
> this change, -no-shutdown would not have to change "-action panic=pause"
> "pause", just like it does not have to change the reset action.
>
> "-action reboot=none" should be renamed to "-action reboot=reset".
> This should be self explanatory, since for example "-action panic=none"
> lets the guest proceed without taking any action.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   qapi/run-state.json       | 10 ++++++----
>   qemu-options.hx           |  8 ++++----
>   softmmu/runstate-action.c |  4 ++--
>   softmmu/runstate.c        |  7 ++++---
>   softmmu/vl.c              |  2 +-
>   5 files changed, 17 insertions(+), 14 deletions(-)

> diff --git a/softmmu/runstate-action.c b/softmmu/runstate-action.c
> index 99ce880886..ae0761a9c3 100644
> --- a/softmmu/runstate-action.c
> +++ b/softmmu/runstate-action.c
> @@ -13,9 +13,9 @@
>   #include "qapi/error.h"
>   #include "qemu/option_int.h"
>   
> -RebootAction reboot_action = REBOOT_ACTION_NONE;
> +RebootAction reboot_action = REBOOT_ACTION_RESET;
>   ShutdownAction shutdown_action = SHUTDOWN_ACTION_POWEROFF;
> -PanicAction panic_action = PANIC_ACTION_POWEROFF;
> +PanicAction panic_action = PANIC_ACTION_SHUTDOWN;
>   
>   /*
>    * Receives actions to be applied for specific guest events

Hi Paolo,
Since you have removed the need for "fixing" the panic action when 
-no-shutdown (or its equivalent action/qmp command) is issued, I'd also 
remove the following comment on qmp_set_action():

     /* Process shutdown last, in case the panic action needs to be 
altered */
     if (has_shutdown) {
         shutdown_action = shutdown;
     }

> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 6177693a30..beee050815 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -471,14 +471,15 @@ void qemu_system_guest_panicked(GuestPanicInformation 
> *info)
>       }
>       /*
>        * TODO:  Currently the available panic actions are: none, pause, and
> -     * poweroff, but in principle debug and reset could be supported as well.
> +     * shutdown, but in principle debug and reset could be supported as well.
>        * Investigate any potential use cases for the unimplemented actions.
>        */
> -    if (panic_action == PANIC_ACTION_PAUSE) {
> +    if (panic_action == PANIC_ACTION_PAUSE
> +        || (panic_action == PANIC_ACTION_SHUTDOWN && shutdown_action == 
> SHUTDOWN_ACTION_PAUSE)) {
>           qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE,
>                                           !!info, info);
>           vm_stop(RUN_STATE_GUEST_PANICKED);
> -    } else if (panic_action == PANIC_ACTION_POWEROFF) {
> +    } else if (panic_action == PANIC_ACTION_SHUTDOWN) {
>           qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
>                                          !!info, info);
>           vm_stop(RUN_STATE_GUEST_PANICKED);

I see that you are moving forward with only sending the GUEST_PANICKED 
event once.

Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 7ddf405d76..59304261cf 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3202,7 +3202,7 @@ void qemu_init(int argc, char **argv, char **envp)
>                   break;
>               case QEMU_OPTION_no_shutdown:
>                   olist = qemu_find_opts("action");
> -                qemu_opts_parse_noisily(olist, "panic=pause,shutdown=pause", 
> false);
> +                qemu_opts_parse_noisily(olist, "shutdown=pause", false);
>                   break;
>               case QEMU_OPTION_uuid:
>                   if (qemu_uuid_parse(optarg, &qemu_uuid) < 0) {