| Message ID | 20210120143706.345821-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | runstate: cleanup reboot and panic actions | expand |
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
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.
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 --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) {
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(-)