diff mbox series

runstate: cleanup reboot and panic actions

Message ID 20210120143706.345821-1-pbonzini@redhat.com
State New
Headers show
Series runstate: cleanup reboot and panic actions | expand

Commit Message

Paolo Bonzini Jan. 20, 2021, 2:37 p.m. UTC
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(-)

Comments

no-reply@patchew.org Jan. 20, 2021, 2:42 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20210120143706.345821-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210120143706.345821-1-pbonzini@redhat.com
Subject: [PATCH] runstate: cleanup reboot and panic actions

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210118170308.282442-1-philmd@redhat.com -> patchew/20210118170308.282442-1-philmd@redhat.com
 - [tag update]      patchew/20210119175427.2050737-1-laurent@vivier.eu -> patchew/20210119175427.2050737-1-laurent@vivier.eu
 - [tag update]      patchew/20210120102556.125012-1-marcandre.lureau@redhat.com -> patchew/20210120102556.125012-1-marcandre.lureau@redhat.com
 - [tag update]      patchew/20210120105406.163074-1-danielhb413@gmail.com -> patchew/20210120105406.163074-1-danielhb413@gmail.com
 * [new tag]         patchew/20210120143706.345821-1-pbonzini@redhat.com -> patchew/20210120143706.345821-1-pbonzini@redhat.com
Switched to a new branch 'test'
202e902 runstate: cleanup reboot and panic actions

=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#117: FILE: softmmu/runstate.c:478:
+        || (panic_action == PANIC_ACTION_SHUTDOWN && shutdown_action == SHUTDOWN_ACTION_PAUSE)) {

total: 1 errors, 0 warnings, 83 lines checked

Commit 202e902ccc44 (runstate: cleanup reboot and panic actions) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210120143706.345821-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Eric Blake Jan. 20, 2021, 3:55 p.m. UTC | #2
On 1/20/21 8: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/qapi/run-state.json b/qapi/run-state.json
> index 1f3b329f05..43d66d700f 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -330,14 +330,14 @@
>  #
>  # Possible QEMU actions upon guest reboot
>  #
> -# @none: Reset the VM
> +# @reset: Reset the VM
>  #
> -# @shutdown: Shutdown the VM and exit
> +# @shutdown: Shutdown the VM and exit, according to the shutdown action
>  #
>  # Since: 6.0
>  ##
>  { 'enum': 'RebootAction',
> -  'data': [ 'none', 'shutdown' ] }
> +  'data': [ 'reset', 'shutdown' ] }

Given that the enum is new, we are not locked into back-compat, so
changing the names to be consistent is reasonable.

Reviewed-by: Eric Blake <eblake$redhat.com>


> +++ 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)) {

Long line.
Alejandro Jimenez Jan. 20, 2021, 6:09 p.m. UTC | #3
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) {
diff mbox series

Patch

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 1f3b329f05..43d66d700f 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -330,14 +330,14 @@ 
 #
 # Possible QEMU actions upon guest reboot
 #
-# @none: Reset the VM
+# @reset: Reset the VM
 #
-# @shutdown: Shutdown the VM and exit
+# @shutdown: Shutdown the VM and exit, according to the shutdown action
 #
 # Since: 6.0
 ##
 { 'enum': 'RebootAction',
-  'data': [ 'none', 'shutdown' ] }
+  'data': [ 'reset', 'shutdown' ] }
 
 ##
 # @ShutdownAction:
@@ -360,10 +360,12 @@ 
 #
 # @pause: Pause the VM
 #
+# @shutdown: Shutdown the VM and exit, according to the shutdown action
+#
 # Since: 6.0
 ##
 { 'enum': 'PanicAction',
-  'data': [ 'poweroff', 'pause', 'none' ] }
+  'data': [ 'pause', 'shutdown', 'none' ] }
 
 ##
 # @watchdog-set-action:
diff --git a/qemu-options.hx b/qemu-options.hx
index 62791f56d8..9172d51659 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3900,12 +3900,12 @@  SRST
 ERST
 
 DEF("action", HAS_ARG, QEMU_OPTION_action,
-    "-action reboot=none|shutdown\n"
-    "                   action when guest reboots [default=none]\n"
+    "-action reboot=reset|shutdown\n"
+    "                   action when guest reboots [default=reset]\n"
     "-action shutdown=poweroff|pause\n"
     "                   action when guest shuts down [default=poweroff]\n"
-    "-action panic=poweroff|pause|none\n"
-    "                   action when guest panics [default=poweroff]\n"
+    "-action panic=pause|shutdown|none\n"
+    "                   action when guest panics [default=shutdown]\n"
     "-action watchdog=reset|shutdown|poweroff|inject-nmi|pause|debug|none\n"
     "                   action when watchdog fires [default=reset]\n",
     QEMU_ARCH_ALL)
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
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);
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) {