Hi everybody,
The series far from being complete. But it finally passes the first simple tests so I think it's time to get the first feedback.
The series started out with the request to be able to rebuild the initramfs for all installed kernels. As 'reset-crashkernel' already had a --kernel=ALL option I didn't want to introduce a second way to do the same thing. My problem was the way --kernel=ALL was implemented, especially its hard dependency on grubby which might not be installed on all systems. This lead to basically a rewrite of the crashkernel handling with now (hopefully) simpler, better to understand code.
An other problem I had with the former implementation of --kernel=ALL was that the command line parser was self-written. This made the UI much more rigid than it had to be. So I changed it to use getopt instead. In order to prevent unnecessary duplicate code I also wrote a framework around it (which might be a little bit over engineered for what we need but once I started I refused to give up plus it gives bash-completion almost for free, which is nice ;-)). While at it I also added some basic options most users expect, like --help.
Finally I've merged multiple commands to make the UI (hopefully) a little bit cleaner and more intuitive. For example I've never understood why 'status' only retuned a simple yes/no but not other important information about the current state like 'git status' does.
As stated in the beginning there is still a lot to do. In particular:
* Test cases still need to be adjusted (and new ones added). * The documentation needs to be reviewed and adjusted. * The patches don't consider Pingfans 64k work, yet. So they will need some adjustments (they are currently based on 29fe563 ("kdump.conf: redirect unknown architecture warning to stderr")) * I've noticed a bug in the bash completion where subsubcommands aren't completed correctly. * And of course the series need a lot more testing. For the moment I've only made some very simple sanity checks. So I would be surprised if the code isn't still riddled with bugs...
Anyway, this is quite a big change and opinions on what a good UI is are very subjective. That's why I want to get feedback early before I waste too much time on something which isn't wanted.
Happy to hear your opinion (good and bad).
Thanks Philipp
Philipp Rudo (26): kdumpctl: Fix various shellcheck findings kdump-lib: make is_zstd_command_available more generic kdump-lib: simplify _get_kdump_kernel_version kdumpctl: drop _get_current_running_kernel_path kdumpctl: drop condrestart subcommand kdumpctl: make all subcommands execute exactly one function kdumpctl: Restructure subcommand handling and and usage kdumpctl: simplify _update_kernel_cmdline kdumpctl: Prevent option --fadump on non-PPC in reset_crashkernel kdumpctl: Stop updating grub config in reset_crashkernel kdumpctl: Simplify fadump handling in reset_crashkernel kdumpctl: Use getopt for option parsing in reset_crashkernel kdumpctl: Add simple bash-completion for kdumpctl kdumpctl: Make --kernel a global option kdumpctl: Add global -h/--help option kdumpctl: Add --kver option kdumpctl: Reduce dependencies from grubby for --kernel kdumpctl: merge reset_crashkernel_{after_update,for_installed_kernel} kdumpctl: Allow --kernel with 'start' and 'rebuild' kdumpctl: Add --quiet/--verbose option kdumpctl: Rework the 'status' subcommand kdumpctl: Merge _upgrade_crashkernel into the upgrade hook kdumpctl: Drop special handling for CoreOS in reset_crashkernel kdumpctl: Determine "old" value within _update_kernel_cmdline kdumpctl: Introduce new crashkernel subcommand REMOVEME: add test subsubcommands
92-crashkernel.install | 2 +- kdump-lib.sh | 27 +- kdumpctl | 1094 ++++++++++++++++++++++++-------------- kdumpctl.bash-completion | 125 +++++ kexec-tools.spec | 16 +- mkdumprd | 2 +- mkfadumprd | 2 +- 7 files changed, 865 insertions(+), 403 deletions(-) create mode 100755 kdumpctl.bash-completion
This includes fixes for
SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns. SC2005 (style): Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. SC2162 (info): read without -r will mangle backslashes. SC2086 (info): Double quote to prevent globbing and word splitting.
In addition add some source hints to prevent false positive findings.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 10 +++++----- kdumpctl | 5 ++++- 2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index c6bf378..fe04eec 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -197,7 +197,7 @@ get_bind_mount_source() local _fsroot _src_nofsroot
_mnt=$(df "$1" | tail -1 | awk '{print $NF}') - _path=${1#$_mnt} + _path=${1#"$_mnt"}
_src=$(get_mount_info SOURCE target "$_mnt" -f) _opt=$(get_mount_info OPTIONS target "$_mnt" -f) @@ -214,7 +214,7 @@ get_bind_mount_source() echo "$_mnt$_path" && return fi
- _fsroot=${_src#${_src_nofsroot}[} + _fsroot=${_src#"${_src_nofsroot}"[} _fsroot=${_fsroot%]} _mnt=$(get_mount_info TARGET source "$_src_nofsroot" -f)
@@ -223,7 +223,7 @@ get_bind_mount_source() local _subvol _subvol=${_opt#*subvol=} _subvol=${_subvol%,*} - _fsroot=${_fsroot#$_subvol} + _fsroot=${_fsroot#"$_subvol"} fi echo "$_mnt$_fsroot$_path" } @@ -270,7 +270,7 @@ kdump_get_persistent_dev() dev=$(blkid -L "${dev#LABEL=}") ;; esac - echo $(get_persistent_dev "$dev") + get_persistent_dev "$dev" }
is_ostree() @@ -766,7 +766,7 @@ get_recommend_size() while read -r -d , range; do # need to use non-default IFS as double spaces are used as a # single delimiter while commas aren't... - IFS=, read start start_unit end end_unit size <<< \ + IFS=, read -r start start_unit end end_unit size <<< \ "$(echo "$range" | sed -n "s/([0-9]+)([GT]?)-([0-9]*)([GT]?):([0-9]+[MG])/\1,\2,\3,\4,\5/p")"
# aka. 102400T diff --git a/kdumpctl b/kdumpctl index 5043a7b..af8f7c9 100755 --- a/kdumpctl +++ b/kdumpctl @@ -29,6 +29,7 @@ if [[ -f /etc/sysconfig/kdump ]]; then fi
[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut +# shellcheck source=/dev/null . $dracutbasedir/dracut-functions.sh
if [[ ${__SOURCED__:+x} ]]; then @@ -36,7 +37,9 @@ if [[ ${__SOURCED__:+x} ]]; then else KDUMP_LIB_PATH=/lib/kdump fi +# shellcheck source=./kdump-lib.sh . $KDUMP_LIB_PATH/kdump-lib.sh +# shellcheck source=./kdump-logger.sh . $KDUMP_LIB_PATH/kdump-logger.sh
#initiate the kdump logger @@ -247,7 +250,7 @@ restore_default_initrd() if [[ $default_checksum != "$backup_checksum" ]]; then dwarn "WARNING: checksum mismatch! Can't restore original initrd.." else - rm -f $INITRD_CHECKSUM_LOCATION + rm -f "$INITRD_CHECKSUM_LOCATION" if mv "$DEFAULT_INITRD_BAK" "$DEFAULT_INITRD"; then derror "Restoring original initrd as fadump mode is disabled." sync -f "$DEFAULT_INITRD"
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
This includes fixes for
SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns. SC2005 (style): Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. SC2162 (info): read without -r will mangle backslashes. SC2086 (info): Double quote to prevent globbing and word splitting.
In addition add some source hints to prevent false positive findings.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdump-lib.sh | 10 +++++----- kdumpctl | 5 ++++- 2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index c6bf378..fe04eec 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -197,7 +197,7 @@ get_bind_mount_source() local _fsroot _src_nofsroot
_mnt=$(df "$1" | tail -1 | awk '{print $NF}')
_path=${1#$_mnt}
_path=${1#"$_mnt"} _src=$(get_mount_info SOURCE target "$_mnt" -f) _opt=$(get_mount_info OPTIONS target "$_mnt" -f)@@ -214,7 +214,7 @@ get_bind_mount_source() echo "$_mnt$_path" && return fi
_fsroot=${_src#${_src_nofsroot}[}
_fsroot=${_src#"${_src_nofsroot}"[} _fsroot=${_fsroot%]} _mnt=$(get_mount_info TARGET source "$_src_nofsroot" -f)@@ -223,7 +223,7 @@ get_bind_mount_source() local _subvol _subvol=${_opt#*subvol=} _subvol=${_subvol%,*}
_fsroot=${_fsroot#$_subvol}
_fsroot=${_fsroot#"$_subvol"} fi echo "$_mnt$_fsroot$_path"} @@ -270,7 +270,7 @@ kdump_get_persistent_dev() dev=$(blkid -L "${dev#LABEL=}") ;; esac
echo $(get_persistent_dev "$dev")
get_persistent_dev "$dev"}
is_ostree() @@ -766,7 +766,7 @@ get_recommend_size() while read -r -d , range; do # need to use non-default IFS as double spaces are used as a # single delimiter while commas aren't...
IFS=, read start start_unit end end_unit size <<< \
IFS=, read -r start start_unit end end_unit size <<< \ "$(echo "$range" | sed -n "s/\([0-9]\+\)\([GT]\?\)-\([0-9]*\)\([GT]\?\):\([0-9]\+[MG]\)/\1,\2,\3,\4,\5/p")" # aka. 102400Tdiff --git a/kdumpctl b/kdumpctl index 5043a7b..af8f7c9 100755 --- a/kdumpctl +++ b/kdumpctl @@ -29,6 +29,7 @@ if [[ -f /etc/sysconfig/kdump ]]; then fi
[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut +# shellcheck source=/dev/null . $dracutbasedir/dracut-functions.sh
if [[ ${__SOURCED__:+x} ]]; then @@ -36,7 +37,9 @@ if [[ ${__SOURCED__:+x} ]]; then else KDUMP_LIB_PATH=/lib/kdump fi +# shellcheck source=./kdump-lib.sh . $KDUMP_LIB_PATH/kdump-lib.sh +# shellcheck source=./kdump-logger.sh . $KDUMP_LIB_PATH/kdump-logger.sh
#initiate the kdump logger @@ -247,7 +250,7 @@ restore_default_initrd() if [[ $default_checksum != "$backup_checksum" ]]; then dwarn "WARNING: checksum mismatch! Can't restore original initrd.." else
rm -f $INITRD_CHECKSUM_LOCATION
rm -f "$INITRD_CHECKSUM_LOCATION" if mv "$DEFAULT_INITRD_BAK" "$DEFAULT_INITRD"; then derror "Restoring original initrd as fadump mode is disabled." sync -f "$DEFAULT_INITRD"-- 2.40.1
Reviewed-by: Pingfan Liu piliu@redhat.com
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 4 ++-- mkdumprd | 2 +- mkfadumprd | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index fe04eec..a39d3d4 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -48,9 +48,9 @@ is_squash_available() done }
-is_zstd_command_available() +has_command() { - [[ -x "$(command -v zstd)" ]] + [[ -x $(command -v "$1") ]] }
dracut_have_option() diff --git a/mkdumprd b/mkdumprd index f93874d..4feac28 100644 --- a/mkdumprd +++ b/mkdumprd @@ -443,7 +443,7 @@ handle_default_dump_target if ! have_compression_in_dracut_args; then if is_squash_available && dracut_have_option "--squash-compressor"; then add_dracut_arg "--squash-compressor" "zstd" - elif is_zstd_command_available; then + elif has_command zstd; then add_dracut_arg "--compress" "zstd" fi fi diff --git a/mkfadumprd b/mkfadumprd index 719e150..dd6840f 100755 --- a/mkfadumprd +++ b/mkfadumprd @@ -64,7 +64,7 @@ _dracut_isolate_args=(
# Use zstd compression method, if available if ! have_compression_in_dracut_args; then - if is_zstd_command_available; then + if has_command zstd; then _dracut_isolate_args+=(--compress zstd) fi fi
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
The abstracted function is also used by the following patches, so it makes sense to me.
Reviewed-by: Pingfan Liu piliu@redhat.com
Signed-off-by: Philipp Rudo prudo@redhat.com
kdump-lib.sh | 4 ++-- mkdumprd | 2 +- mkfadumprd | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index fe04eec..a39d3d4 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -48,9 +48,9 @@ is_squash_available() done }
-is_zstd_command_available() +has_command() {
[[ -x "$(command -v zstd)" ]]
[[ -x $(command -v "$1") ]]}
dracut_have_option() diff --git a/mkdumprd b/mkdumprd index f93874d..4feac28 100644 --- a/mkdumprd +++ b/mkdumprd @@ -443,7 +443,7 @@ handle_default_dump_target if ! have_compression_in_dracut_args; then if is_squash_available && dracut_have_option "--squash-compressor"; then add_dracut_arg "--squash-compressor" "zstd"
elif is_zstd_command_available; then
elif has_command zstd; then add_dracut_arg "--compress" "zstd" fifi diff --git a/mkfadumprd b/mkfadumprd index 719e150..dd6840f 100755 --- a/mkfadumprd +++ b/mkfadumprd @@ -64,7 +64,7 @@ _dracut_isolate_args=(
# Use zstd compression method, if available if ! have_compression_in_dracut_args; then
if is_zstd_command_available; then
if has_command zstd; then _dracut_isolate_args+=(--compress zstd) fifi
2.40.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
Only check whether modules for a given kernel version are installed instead of searching for a kernel image. It's safer to assume that every kernel uses kernel modules compared to that it follows certain naming conventions. Furthermore it is much more lightweight and thus allows to determine the KDUMP_KERNELVER much earlier for every command in kdumpctl.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index a39d3d4..c95cc94 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -11,6 +11,11 @@ fi FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled" FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"
+_is_valid_kver() +{ + [[ -f /usr/lib/modules/$1/modules.dep ]] +} + is_uki() { local img @@ -543,7 +548,7 @@ _get_kdump_kernel_version()
_version_nondebug=${_version%+debug} _version_nondebug=${_version_nondebug%-debug} - if [[ -f "$(prepare_kdump_kernel "$_version_nondebug")" ]]; then + if _is_valid_kver "$_version_nondebug"; then dinfo "Use of debug kernel detected. Trying to use $_version_nondebug" echo "$_version_nondebug" else
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
Only check whether modules for a given kernel version are installed instead of searching for a kernel image. It's safer to assume that every kernel uses kernel modules compared to that it follows certain naming
Since prepare_kdump_kernel() is still used in the main path, this does a little help in fact.
conventions. Furthermore it is much more lightweight and thus allows to determine the KDUMP_KERNELVER much earlier for every command in kdumpctl.
Anyway, the performance is a justification.
Reviewed-by: Pingfan Liu piliu@redhat.com
Signed-off-by: Philipp Rudo prudo@redhat.com
kdump-lib.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index a39d3d4..c95cc94 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -11,6 +11,11 @@ fi FADUMP_ENABLED_SYS_NODE="/sys/kernel/fadump_enabled" FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"
+_is_valid_kver() +{
[[ -f /usr/lib/modules/$1/modules.dep ]]+}
is_uki() { local img @@ -543,7 +548,7 @@ _get_kdump_kernel_version()
_version_nondebug=${_version%+debug} _version_nondebug=${_version_nondebug%-debug}
if [[ -f "$(prepare_kdump_kernel "$_version_nondebug")" ]]; then
if _is_valid_kver "$_version_nondebug"; then dinfo "Use of debug kernel detected. Trying to use $_version_nondebug" echo "$_version_nondebug" else-- 2.40.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
_get_current_running_kernel_path is identical to _find_kernel_path_by_release $(uname -r) so simply use this instead of defining a new function.
While at it simplify reset_crashkernel slightly. This changes the behavior of the function for the case when KDUMP_KERNELVER was defined but no kernel with this version was installed. Before, the missing kernel was silently ignored and the currently running kernel was used instead. Now kdumpctl will exit with an error.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/kdumpctl b/kdumpctl index af8f7c9..b689e2d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1418,18 +1418,6 @@ _find_kernel_path_by_release() echo -n "$_kernel_path" }
-_get_current_running_kernel_path() -{ - local _release _path - - _release=$(uname -r) - if _path=$(_find_kernel_path_by_release "$_release"); then - echo -n "$_path" - else - return 1 - fi -} - _update_kernel_cmdline() { local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4 @@ -1624,14 +1612,11 @@ reset_crashkernel() # - use KDUMP_KERNELVER if it's defined # - use current running kernel if [[ -z $_grubby_kernel_path ]]; then - if [[ -z $KDUMP_KERNELVER ]] || - ! _kernel_path=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then - if ! _kernel_path=$(_get_current_running_kernel_path); then - derror "no running kernel found" - exit 1 - fi + [[ -n $KDUMP_KERNELVER ]] || KDUMP_KERNELVER=$(uname -r) + if ! _kernels=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then + derror "no kernel for version $KDUMP_KERNELVER found" + exit 1 fi - _kernels=$_kernel_path else _kernels=$(_get_all_kernels_from_grubby "$_grubby_kernel_path") fi
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
_get_current_running_kernel_path is identical to _find_kernel_path_by_release $(uname -r) so simply use this instead of defining a new function.
While at it simplify reset_crashkernel slightly. This changes the behavior of the function for the case when KDUMP_KERNELVER was defined but no kernel with this version was installed. Before, the missing kernel was silently ignored and the currently running kernel was used instead. Now kdumpctl will exit with an error.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/kdumpctl b/kdumpctl index af8f7c9..b689e2d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1418,18 +1418,6 @@ _find_kernel_path_by_release() echo -n "$_kernel_path" }
-_get_current_running_kernel_path() -{
local _release _path_release=$(uname -r)if _path=$(_find_kernel_path_by_release "$_release"); thenecho -n "$_path"elsereturn 1fi-}
_update_kernel_cmdline() { local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4 @@ -1624,14 +1612,11 @@ reset_crashkernel() # - use KDUMP_KERNELVER if it's defined # - use current running kernel if [[ -z $_grubby_kernel_path ]]; then
if [[ -z $KDUMP_KERNELVER ]] ||! _kernel_path=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); thenif ! _kernel_path=$(_get_current_running_kernel_path); thenderror "no running kernel found"exit 1fi
[[ -n $KDUMP_KERNELVER ]] || KDUMP_KERNELVER=$(uname -r)if ! _kernels=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); thenderror "no kernel for version $KDUMP_KERNELVER found"exit 1 fi
_kernels=$_kernel_path else _kernels=$(_get_all_kernels_from_grubby "$_grubby_kernel_path") fi-- 2.40.1
Reviewed-by: Pingfan Liu piliu@redhat.com
condrestart is a left over from the time of SysVinit that is no longer needed since the kexec-tools switched to systemd (10c91a1 ("Removing sysvinit files") plus the one before). What's especially intriguing is that from the beginning (0112f36 ("- Add a kdump sysconfig file and init script - Spec file additions for pre/post install/uninstall")) the sub-command never did any actual work (other than not returning an error). Thus simply remove the condrestart sub-command.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 2 -- 1 file changed, 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index b689e2d..79bd78a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1789,8 +1789,6 @@ main() rebuild) rebuild ;; - condrestart) ;; - propagate) propagate_ssh_key ;;
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
condrestart is a left over from the time of SysVinit that is no longer needed since the kexec-tools switched to systemd (10c91a1 ("Removing sysvinit files") plus the one before). What's especially intriguing is that from the beginning (0112f36 ("- Add a kdump sysconfig file and init script - Spec file additions for pre/post install/uninstall")) the sub-command never did any actual work (other than not returning an error). Thus simply remove the condrestart sub-command.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 2 -- 1 file changed, 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index b689e2d..79bd78a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1789,8 +1789,6 @@ main() rebuild) rebuild ;;
condrestart) ;;propagate) propagate_ssh_key ;;-- 2.40.1
Reviewed-by: Pingfan Liu piliu@redhat.com
This allows to restructure the subcommand handling later on which is a prereq to introduce the new CLI parsing.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 102 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 43 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 79bd78a..7b04ba2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1030,7 +1030,7 @@ check_final_action_config() esac }
-start() +__start() { check_dump_feasibility || return parse_config || return @@ -1066,7 +1066,32 @@ start() return 0 }
-reload() +_start() +{ + if ! __start; then + derror "Starting kdump: [FAILED]" + exit 1 + fi +} + +_status() +{ + EXIT_CODE=0 + is_kernel_loaded "$DEFAULT_DUMP_MODE" + case "$?" in + 0) + dinfo "Kdump is operational" + EXIT_CODE=0 + ;; + 1) + dinfo "Kdump is not operational" + EXIT_CODE=3 + ;; + esac + exit $EXIT_CODE +} + +_reload() { if ! is_kernel_loaded "$DEFAULT_DUMP_MODE"; then dwarn "Kdump was not running: [WARNING]" @@ -1145,7 +1170,7 @@ reload_fadump() return 1 }
-stop() +__stop() { if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then stop_fadump || return @@ -1157,7 +1182,21 @@ stop() return 0 }
-rebuild() +_stop() +{ + if ! _stop; then + derror "Stopping kdump: [FAILED]" + exit 1 + fi +} + +_restart() +{ + _start + _stop +} + +_rebuild() { parse_config || return 1 check_and_wait_network_ready || return 1 @@ -1682,12 +1721,14 @@ _update_crashkernel() fi }
- -# shellcheck disable=SC2154 # false positive when dereferencing an array reset_crashkernel_after_update() { local _kernel
+ if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then + return + fi + if ! _is_bootloader_installed; then return fi @@ -1720,6 +1761,10 @@ reset_crashkernel_for_installed_kernel() { local _installed_kernel
+ if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then + return + fi + # During package install, only try to reset crashkernel for osbuild # thus to avoid calling grubby when installing os via anaconda if ! _is_bootloader_installed && ! _is_osbuild; then @@ -1747,47 +1792,22 @@ main()
case "$1" in start) - if ! start; then - derror "Starting kdump: [FAILED]" - exit 1 - fi + _start ;; stop) - if ! stop; then - derror "Stopping kdump: [FAILED]" - exit 1 - fi + _stop ;; status) - EXIT_CODE=0 - is_kernel_loaded "$DEFAULT_DUMP_MODE" - case "$?" in - 0) - dinfo "Kdump is operational" - EXIT_CODE=0 - ;; - 1) - dinfo "Kdump is not operational" - EXIT_CODE=3 - ;; - esac - exit $EXIT_CODE + _status ;; reload) - reload + _reload ;; restart) - if ! stop; then - derror "Stopping kdump: [FAILED]" - exit 1 - fi - if ! start; then - derror "Starting kdump: [FAILED]" - exit 1 - fi + _restart ;; rebuild) - rebuild + _rebuild ;; propagate) propagate_ssh_key @@ -1806,14 +1826,10 @@ main() reset_crashkernel "$@" ;; _reset-crashkernel-after-update) - if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then - reset_crashkernel_after_update - fi + reset_crashkernel_after_update ;; _reset-crashkernel-for-installed_kernel) - if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then - reset_crashkernel_for_installed_kernel "$2" - fi + reset_crashkernel_for_installed_kernel "$2" ;; *) dinfo $"Usage: $0 {estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
Collect all functions executed for the subcommands in an array. This is a prereq to restructure the CLI parsing later on.
While at it introduce a usage function to give the user a consistent feedback on user errors.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 82 ++++++++++++++++++++++++-------------------------------- 1 file changed, 35 insertions(+), 47 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 7b04ba2..64ec9e7 100755 --- a/kdumpctl +++ b/kdumpctl @@ -16,6 +16,7 @@ KDUMP_INITRD="" TARGET_INITRD="" #kdump shall be the default dump mode DEFAULT_DUMP_MODE="kdump" +PROGNAME=${0##*/}
standard_kexec_args="-d -p"
@@ -24,6 +25,23 @@ KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"
declare -A OPT
+declare -A cmds_global +cmds_global=() +cmds_global[start]=_start +cmds_global[stop]=_stop +cmds_global[status]=_status +cmds_global[reload]=_reload +cmds_global[restart]=_restart +cmds_global[rebuild]=_rebuild +cmds_global[propagate]=propagate_ssh_key +cmds_global[showmem]=show_reserved_mem +cmds_global[estimate]=do_estimate +cmds_global[get-default-crashkernel]=get_default_crashkernel +cmds_global[reset-crashkernel]=reset_crashkernel +cmds_global[_reset-crashkernel-after-update]=reset_crashkernel_after_update +# shellcheck disable=SC2034 +cmds_global[_reset-crashkernel-for-installed_kernel]=reset_crashkernel_for_installed_kernel + if [[ -f /etc/sysconfig/kdump ]]; then . /etc/sysconfig/kdump fi @@ -55,6 +73,15 @@ trap ' exit $ret; ' EXIT
+_usage() +{ + [[ -n "$*" ]] && dinfo "$*" + dinfo "Usage: $PROGNAME [OPTIONS] COMMAND" + dinfo "See '$PROGNAME --help' or 'man $PROGNAME' for more details" + + exit 1 +} + _get_dracut_arg() { local shortopt longopt n tmp @@ -1787,55 +1814,16 @@ reset_crashkernel_for_installed_kernel()
main() { + local cmd + + [[ -n $1 ]] || _usage "No sub-command provided" + cmd=${cmds_global[$1]} + [[ -n $cmd ]] || _usage "Unknown option or sub-command '$1' provided" + shift + # Determine if the dump mode is kdump or fadump determine_dump_mode - - case "$1" in - start) - _start - ;; - stop) - _stop - ;; - status) - _status - ;; - reload) - _reload - ;; - restart) - _restart - ;; - rebuild) - _rebuild - ;; - propagate) - propagate_ssh_key - ;; - showmem) - show_reserved_mem - ;; - estimate) - do_estimate - ;; - get-default-crashkernel) - get_default_crashkernel "$2" - ;; - reset-crashkernel) - shift - reset_crashkernel "$@" - ;; - _reset-crashkernel-after-update) - reset_crashkernel_after_update - ;; - _reset-crashkernel-for-installed_kernel) - reset_crashkernel_for_installed_kernel "$2" - ;; - *) - dinfo $"Usage: $0 {estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}" - exit 1 - ;; - esac + $cmd "$@" }
if [[ ${__SOURCED__:+x} ]]; then
_update_kernel_cmdline handles two cmdline parameters at once. This does not only make the function itself but also its callers more complicated than necessary. For example in _update_crashkernel the fadump gets "updated" to the value that has been read from grubby. Thus simplify _update_kernel_cmdline to only update one parameter at once.
While at shorten some variable named in the callers.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 106 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 50 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 64ec9e7..e108781 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1486,21 +1486,33 @@ _find_kernel_path_by_release()
_update_kernel_cmdline() { - local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4 + local _kernel _param _old _new + + _kernel="$1" + _param="$2" + _old="$3" + _new="$4" + + # _new = "" removes a _param, so _new = _old = "" is fine + [[ -n "$_new" ]] && [[ "$_old" == "$_new" ]] && return 1
if is_ostree; then - if rpm-ostree kargs | grep -q "crashkernel="; then - rpm-ostree kargs --replace="crashkernel=$_crashkernel" + if [[ -z "$_new" ]]; then + rpm-ostree kargs --delete="$_param=$_old" + elif [[ -n "$_old" ]]; then + rpm-ostree kargs --replace="$_param=$_new" else - rpm-ostree kargs --append="crashkernel=$_crashkernel" + rpm-ostree kargs --append="$_param=$_new" fi - else - grubby --args "crashkernel=$_crashkernel" --update-kernel "$_kernel_path" - if [[ $_dump_mode == kdump ]]; then - grubby --remove-args="fadump" --update-kernel "$_kernel_path" + elif has_command grubby; then + if [[ -n "$_new" ]]; then + grubby --update-kernel "$_kernel" --args "$_param=$_new" else - grubby --args="fadump=$_fadump_val" --update-kernel "$_kernel_path" + grubby --update-kernel "$_kernel" --remove-args "$_param" fi + else + derror "Unable to update kernel command line" + exit 1 fi
# Don't use "[[ CONDITION ]] && COMMAND" which returns 1 under false CONDITION @@ -1592,9 +1604,11 @@ _read_kernel_arg_in_grub_etc_default()
reset_crashkernel() { - local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel _kernels - local _old_crashkernel _new_crashkernel _new_dump_mode _crashkernel_changed - local _new_fadump_val _old_fadump_val _what_is_updated + local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels + local _dump_mode _new_dump_mode + local _has_changed _needs_reboot + local _old_ck _new_ck + local _old_fadump _new_fadump
for _opt in "$@"; do case "$_opt" in @@ -1632,14 +1646,10 @@ reset_crashkernel() # modifying the kernel command line so there is no need for kexec-tools # to repeat it. if is_ostree; then - _old_crashkernel=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') - _new_dump_mode=kdump - _new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") - if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then - _update_kernel_cmdline "" "$_new_crashkernel" "$_new_dump_mode" "" - if [[ $_reboot == yes ]]; then - systemctl reboot - fi + _old_ck=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') + _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) + if _update_kernel_cmdline "" crashkernel "$_old_ck" "$_new_ck"; then + [[ $_reboot == yes ]] && systemctl reboot fi return fi @@ -1688,35 +1698,33 @@ reset_crashkernel() fi
for _kernel in $_kernels; do + _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") - _new_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") + _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode - _new_crashkernel=$_crashkernel - _new_fadump_val=$_fadump_val + _new_ck=$_crashkernel + _new_fadump=$_fadump_val fi
- _old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) - _old_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - if [[ $_old_crashkernel != "$_new_crashkernel" || $_old_fadump_val != "$_new_fadump_val" ]]; then - _update_kernel_cmdline "$_kernel" "$_new_crashkernel" "$_new_dump_mode" "$_new_fadump_val" - if [[ $_reboot != yes ]]; then - if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then - _what_is_updated="Updated crashkernel=$_new_crashkernel" - else - # This case happens only when switching between fadump=on and fadump=nocma - _what_is_updated="Updated fadump=$_new_fadump_val" - fi - dwarn "$_what_is_updated for kernel=$_kernel. Please reboot the system for the change to take effect." - fi - _crashkernel_changed=yes + _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then + _has_changed="Updated fadump=$_new_fadump" + fi + if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then + _has_changed="Updated crashkernel=$_new_ck" + fi + if [[ -n "$_has_changed" ]]; then + _needs_reboot=1 + dwarn "$_has_changed for kernel=$_kernel. Please reboot the system for the change to take effect." fi done
- if [[ $_reboot == yes && $_crashkernel_changed == yes ]]; then - reboot + if [[ $_reboot == yes && $_needs_reboot == 1 ]]; then + systemctl reboot fi }
@@ -1732,19 +1740,17 @@ _is_bootloader_installed()
_update_crashkernel() { - local _kernel _dump_mode _old_default_crashkernel _new_default_crashkernel _fadump_val _msg + local _kernel _dump_mode _msg + local _old_ck _new_ck
_kernel=$1 _dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _old_default_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) - _new_default_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - if [[ $_old_default_crashkernel != "$_new_default_crashkernel" ]]; then - _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - if _update_kernel_cmdline "$_kernel" "$_new_default_crashkernel" "$_dump_mode" "$_fadump_val"; then - _msg="For kernel=$_kernel, crashkernel=$_new_default_crashkernel now. Please reboot the system for the change to take effet." - _msg+=" Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." - dinfo "$_msg" - fi + _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") + if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then + _msg="For kernel=$_kernel, crashkernel=$_new_ck now. Please reboot the system for the change to take effect." + _msg+=" Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." + dinfo "$_msg" fi }
Prevent the --fadump option to be used on non-PPC systems. This not only prevents user errors but also guarantees that _dump_mode and _fadump_val are empty on these systems.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e108781..e9086b2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1613,6 +1613,10 @@ reset_crashkernel() for _opt in "$@"; do case "$_opt" in --fadump=*) + if [[ $(uname -m) != ppc64le ]]; then + derror "Option --fadump only valid on PPC" + exit 1 + fi _val=${_opt#*=} if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then _fadump_val=$_val @@ -1654,12 +1658,7 @@ reset_crashkernel() return fi
- # For non-ppc64le systems, the dump mode is always kdump since only ppc64le - # has FADump. - if [[ -z $_dump_mode && $(uname -m) != ppc64le ]]; then - _dump_mode=kdump - _fadump_val=off - fi + [[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
# If the dump mode is determined, we can also know the default crashkernel value if [[ -n $_dump_mode ]]; then
On Sat, Jun 17, 2023 at 5:28 PM Philipp Rudo prudo@redhat.com wrote:
Prevent the --fadump option to be used on non-PPC systems. This not only prevents user errors but also guarantees that _dump_mode and _fadump_val are empty on these systems.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e108781..e9086b2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1613,6 +1613,10 @@ reset_crashkernel() for _opt in "$@"; do case "$_opt" in --fadump=*)
if [[ $(uname -m) != ppc64le ]]; thenderror "Option --fadump only valid on PPC"exit 1fi _val=${_opt#*=} if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then _fadump_val=$_val@@ -1654,12 +1658,7 @@ reset_crashkernel() return fi
# For non-ppc64le systems, the dump mode is always kdump since only ppc64le# has FADump.if [[ -z $_dump_mode && $(uname -m) != ppc64le ]]; then_dump_mode=kdump_fadump_val=offfi
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump # If the dump mode is determined, we can also know the default crashkernel value if [[ -n $_dump_mode ]]; then-- 2.40.1
Reviewed-by: Pingfan Liu piliu@redhat.com
With multiple kernel variants on the same architecture, e.g. the 4k and 64k kernel on aarch64, we can no longer assume that the crashkernel value for the currently running kernel will work for all installed kernels. This also means that we can no longer update the grub config as we don't know which value to set it to. Thus get the crashkernel value for each kernel and stop updating the grub config.
While at it merge the _new_fadump and _fadump_val variables and remove _read_kernel_arg_in_grub_etc_default which has no user.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 96 +++----------------------------------------------------- 1 file changed, 5 insertions(+), 91 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e9086b2..6008df8 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1542,69 +1542,9 @@ _get_all_kernels_from_grubby() echo -n "$_kernels" }
-GRUB_ETC_DEFAULT="/etc/default/grub" -# Update a kernel parameter in default grub conf -# -# If a value is specified, it will be inserted in the end. Otherwise it -# would remove given kernel parameter. -# -# Note this function doesn't address the following cases, -# 1. The kernel ignores everything on the command line after a '--'. So -# simply adding the new entry to the end will fail if the cmdline -# contains a --. -# 2. If the value for a parameter contains spaces it can be quoted using -# double quotes, for example param="value with spaces". This will -# break the [^[:space:]"] regex for the value. -# 3. Dashes and underscores in the parameter name are equivalent. So -# some_parameter and some-parameter are identical. -# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. -# 5. Some kernel parameters, e.g. quiet, doesn't have value -# -# $1: the name of the kernel command line parameter -# $2: new value. If empty, given parameter would be removed -_update_kernel_arg_in_grub_etc_default() -{ - local _para=$1 _val=$2 _para_val - - if [[ $(uname -m) == s390x ]]; then - return - fi - - if [[ -n $_val ]]; then - _para_val="$_para=$_val" - fi - - # Update the command line /etc/default/grub, i.e. - # on the line that starts with 'GRUB_CMDLINE_LINUX=', - # 1) remove $para=$val if the it's the first arg - # 2) remove all occurences of $para=$val - # 3) insert $_para_val to end - # 4) remove duplicate spaces left over by 1) or 2) or 3) - # 5) remove space at the beginning of the string left over by 1) or 2) or 3) - # 6) remove space at the end of the string left over by 1) or 2) or 3) - sed -i -E "/^GRUB_CMDLINE_LINUX=/ { - s/"${_para}=[^[:space:]"]*/"/g; - s/[[:space:]]+${_para}=[^[:space:]"]*/ /g; - s/"$/ ${_para_val}"/ - s/[[:space:]]+/ /g; - s/(")[[:space:]]+/\1/g; - s/[[:space:]]+(")/\1/g; - }" "$GRUB_ETC_DEFAULT" -} - -# Read the kernel arg in default grub conf. - -# Note reading a kernel parameter that doesn't have a value isn't supported. -# -# $1: the name of the kernel command line parameter -_read_kernel_arg_in_grub_etc_default() -{ - sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]"]${1}=([^[:space:]"]*).*$/\1/p" "$GRUB_ETC_DEFAULT" -} - reset_crashkernel() { - local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels + local _opt _val _reboot _grubby_kernel_path _kernel _kernels local _dump_mode _new_dump_mode local _has_changed _needs_reboot local _old_ck _new_ck @@ -1617,13 +1557,12 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi - _val=${_opt#*=} - if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then - _fadump_val=$_val - else + _new_fadump=${_opt#*=} + if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then derror "failed to determine dump mode" exit fi + [[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=} @@ -1660,29 +1599,6 @@ reset_crashkernel()
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
- # If the dump mode is determined, we can also know the default crashkernel value - if [[ -n $_dump_mode ]]; then - _crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - fi - - # If --kernel-path=ALL, update GRUB_CMDLINE_LINUX in /etc/default/grub. - # - # An exception case is when the ppc64le user doesn't specify the fadump value. - # In this case, the dump mode would be determined by parsing the kernel - # command line of the kernel(s) to be updated thus don't update GRUB_CMDLINE_LINUX. - # - # The following code has been simplified because of what has been done early, - # - set the dump mode as kdump for non-ppc64le cases - # - retrieved the default crashkernel value for given dump mode - if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then - _update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel" - # remove the fadump if fadump is disabled - if [[ $_fadump_val == off ]]; then - _fadump_val="" - fi - _update_kernel_arg_in_grub_etc_default fadump "$_fadump_val" - fi - # If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel @@ -1700,15 +1616,13 @@ reset_crashkernel() _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode - _new_ck=$_crashkernel - _new_fadump=$_fadump_val fi
_old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then _has_changed="Updated fadump=$_new_fadump"
On Sat, Jun 17, 2023 at 5:29 PM Philipp Rudo prudo@redhat.com wrote:
With multiple kernel variants on the same architecture, e.g. the 4k and 64k kernel on aarch64, we can no longer assume that the crashkernel value for the currently running kernel will work for all installed kernels. This also means that we can no longer update the grub config as we don't know which value to set it to. Thus get the crashkernel value for each kernel and stop updating the grub config.
I think this patch should be the final stop of this series. Before it, we should provide an alternative interface to grub, which should be "kdumpctl crashkernel" sub-command.
But rebasing may be annoying, so I am fine with it.
Reviewed-by: Pingfan Liu piliu@redhat.com
While at it merge the _new_fadump and _fadump_val variables and remove _read_kernel_arg_in_grub_etc_default which has no user.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 96 +++----------------------------------------------------- 1 file changed, 5 insertions(+), 91 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e9086b2..6008df8 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1542,69 +1542,9 @@ _get_all_kernels_from_grubby() echo -n "$_kernels" }
-GRUB_ETC_DEFAULT="/etc/default/grub" -# Update a kernel parameter in default grub conf -# -# If a value is specified, it will be inserted in the end. Otherwise it -# would remove given kernel parameter. -# -# Note this function doesn't address the following cases, -# 1. The kernel ignores everything on the command line after a '--'. So -# simply adding the new entry to the end will fail if the cmdline -# contains a --. -# 2. If the value for a parameter contains spaces it can be quoted using -# double quotes, for example param="value with spaces". This will -# break the [^[:space:]"] regex for the value. -# 3. Dashes and underscores in the parameter name are equivalent. So -# some_parameter and some-parameter are identical. -# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. -# 5. Some kernel parameters, e.g. quiet, doesn't have value -# -# $1: the name of the kernel command line parameter -# $2: new value. If empty, given parameter would be removed -_update_kernel_arg_in_grub_etc_default() -{
local _para=$1 _val=$2 _para_valif [[ $(uname -m) == s390x ]]; thenreturnfiif [[ -n $_val ]]; then_para_val="$_para=$_val"fi# Update the command line /etc/default/grub, i.e.# on the line that starts with 'GRUB_CMDLINE_LINUX=',# 1) remove $para=$val if the it's the first arg# 2) remove all occurences of $para=$val# 3) insert $_para_val to end# 4) remove duplicate spaces left over by 1) or 2) or 3)# 5) remove space at the beginning of the string left over by 1) or 2) or 3)# 6) remove space at the end of the string left over by 1) or 2) or 3)sed -i -E "/^GRUB_CMDLINE_LINUX=/ {s/\"${_para}=[^[:space:]\"]*/\"/g;s/[[:space:]]+${_para}=[^[:space:]\"]*/ /g;s/\"$/ ${_para_val}\"/s/[[:space:]]+/ /g;s/(\")[[:space:]]+/\1/g;s/[[:space:]]+(\")/\1/g;}" "$GRUB_ETC_DEFAULT"-}
-# Read the kernel arg in default grub conf.
-# Note reading a kernel parameter that doesn't have a value isn't supported. -# -# $1: the name of the kernel command line parameter -_read_kernel_arg_in_grub_etc_default() -{
sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]\"]${1}=([^[:space:]\"]*).*$/\1/p" "$GRUB_ETC_DEFAULT"-}
reset_crashkernel() {
local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels
local _opt _val _reboot _grubby_kernel_path _kernel _kernels local _dump_mode _new_dump_mode local _has_changed _needs_reboot local _old_ck _new_ck@@ -1617,13 +1557,12 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi
_val=${_opt#*=}if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then_fadump_val=$_valelse
_new_fadump=${_opt#*=}if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then derror "failed to determine dump mode" exit fi[[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=}@@ -1660,29 +1599,6 @@ reset_crashkernel()
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
# If the dump mode is determined, we can also know the default crashkernel valueif [[ -n $_dump_mode ]]; then_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")fi# If --kernel-path=ALL, update GRUB_CMDLINE_LINUX in /etc/default/grub.## An exception case is when the ppc64le user doesn't specify the fadump value.# In this case, the dump mode would be determined by parsing the kernel# command line of the kernel(s) to be updated thus don't update GRUB_CMDLINE_LINUX.## The following code has been simplified because of what has been done early,# - set the dump mode as kdump for non-ppc64le cases# - retrieved the default crashkernel value for given dump modeif [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then_update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel"# remove the fadump if fadump is disabledif [[ $_fadump_val == off ]]; then_fadump_val=""fi_update_kernel_arg_in_grub_etc_default fadump "$_fadump_val"fi# If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel@@ -1700,15 +1616,13 @@ reset_crashkernel() _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode_new_ck=$_crashkernel_new_fadump=$_fadump_val fi _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel)
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then _has_changed="Updated fadump=$_new_fadump"-- 2.40.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
Hi Pingfan,
sorry for the late answer.
On Thu, 6 Jul 2023 10:20:10 +0800 Pingfan Liu piliu@redhat.com wrote:
On Sat, Jun 17, 2023 at 5:29 PM Philipp Rudo prudo@redhat.com wrote:
With multiple kernel variants on the same architecture, e.g. the 4k and 64k kernel on aarch64, we can no longer assume that the crashkernel value for the currently running kernel will work for all installed kernels. This also means that we can no longer update the grub config as we don't know which value to set it to. Thus get the crashkernel value for each kernel and stop updating the grub config.
I think this patch should be the final stop of this series. Before it, we should provide an alternative interface to grub, which should be "kdumpctl crashkernel" sub-command.
But rebasing may be annoying, so I am fine with it.
Sorry, but I don't fully understand your comment here. Especially, what you mean with "alternative interface to grub".
In general my long term goal is to get rid of grubby on the default paths and edit the BLS config files directly. Then we still need some --grub option that tells kdumpctl to also update the grub config. But in that case the user explicitly tells us to do the update so I think it is much safer to assume that grubby is installed. I've only omitted to include this feature in this series because the series is already is too long the way it is right now (and I already have some other additional patches in the pipeline for v2). Is that what you meant?
Finally, I'm totally fine to split out the patches to a separate series to get them applied early. If you prefer I can also move the two subcommand handling patches (patch 7 & 8) after this patch. Only the patch that simplifies _update_kernel_cmdline I would prefer to keep where it is as moving it it will cause conflicts I'd like to avoid.
Reviewed-by: Pingfan Liu piliu@redhat.com
Thanks a lot for the review!
Philipp
While at it merge the _new_fadump and _fadump_val variables and remove _read_kernel_arg_in_grub_etc_default which has no user.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 96 +++----------------------------------------------------- 1 file changed, 5 insertions(+), 91 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e9086b2..6008df8 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1542,69 +1542,9 @@ _get_all_kernels_from_grubby() echo -n "$_kernels" }
-GRUB_ETC_DEFAULT="/etc/default/grub" -# Update a kernel parameter in default grub conf -# -# If a value is specified, it will be inserted in the end. Otherwise it -# would remove given kernel parameter. -# -# Note this function doesn't address the following cases, -# 1. The kernel ignores everything on the command line after a '--'. So -# simply adding the new entry to the end will fail if the cmdline -# contains a --. -# 2. If the value for a parameter contains spaces it can be quoted using -# double quotes, for example param="value with spaces". This will -# break the [^[:space:]"] regex for the value. -# 3. Dashes and underscores in the parameter name are equivalent. So -# some_parameter and some-parameter are identical. -# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. -# 5. Some kernel parameters, e.g. quiet, doesn't have value -# -# $1: the name of the kernel command line parameter -# $2: new value. If empty, given parameter would be removed -_update_kernel_arg_in_grub_etc_default() -{
local _para=$1 _val=$2 _para_valif [[ $(uname -m) == s390x ]]; thenreturnfiif [[ -n $_val ]]; then_para_val="$_para=$_val"fi# Update the command line /etc/default/grub, i.e.# on the line that starts with 'GRUB_CMDLINE_LINUX=',# 1) remove $para=$val if the it's the first arg# 2) remove all occurences of $para=$val# 3) insert $_para_val to end# 4) remove duplicate spaces left over by 1) or 2) or 3)# 5) remove space at the beginning of the string left over by 1) or 2) or 3)# 6) remove space at the end of the string left over by 1) or 2) or 3)sed -i -E "/^GRUB_CMDLINE_LINUX=/ {s/\"${_para}=[^[:space:]\"]*/\"/g;s/[[:space:]]+${_para}=[^[:space:]\"]*/ /g;s/\"$/ ${_para_val}\"/s/[[:space:]]+/ /g;s/(\")[[:space:]]+/\1/g;s/[[:space:]]+(\")/\1/g;}" "$GRUB_ETC_DEFAULT"-}
-# Read the kernel arg in default grub conf.
-# Note reading a kernel parameter that doesn't have a value isn't supported. -# -# $1: the name of the kernel command line parameter -_read_kernel_arg_in_grub_etc_default() -{
sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]\"]${1}=([^[:space:]\"]*).*$/\1/p" "$GRUB_ETC_DEFAULT"-}
reset_crashkernel() {
local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels
local _opt _val _reboot _grubby_kernel_path _kernel _kernels local _dump_mode _new_dump_mode local _has_changed _needs_reboot local _old_ck _new_ck@@ -1617,13 +1557,12 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi
_val=${_opt#*=}if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then_fadump_val=$_valelse
_new_fadump=${_opt#*=}if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then derror "failed to determine dump mode" exit fi[[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=}@@ -1660,29 +1599,6 @@ reset_crashkernel()
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
# If the dump mode is determined, we can also know the default crashkernel valueif [[ -n $_dump_mode ]]; then_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")fi# If --kernel-path=ALL, update GRUB_CMDLINE_LINUX in /etc/default/grub.## An exception case is when the ppc64le user doesn't specify the fadump value.# In this case, the dump mode would be determined by parsing the kernel# command line of the kernel(s) to be updated thus don't update GRUB_CMDLINE_LINUX.## The following code has been simplified because of what has been done early,# - set the dump mode as kdump for non-ppc64le cases# - retrieved the default crashkernel value for given dump modeif [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then_update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel"# remove the fadump if fadump is disabledif [[ $_fadump_val == off ]]; then_fadump_val=""fi_update_kernel_arg_in_grub_etc_default fadump "$_fadump_val"fi# If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel@@ -1700,15 +1616,13 @@ reset_crashkernel() _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode_new_ck=$_crashkernel_new_fadump=$_fadump_val fi _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel)
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then _has_changed="Updated fadump=$_new_fadump"-- 2.40.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
On Tue, Jul 11, 2023 at 8:42 PM Philipp Rudo prudo@redhat.com wrote:
Hi Pingfan,
sorry for the late answer.
On Thu, 6 Jul 2023 10:20:10 +0800 Pingfan Liu piliu@redhat.com wrote:
On Sat, Jun 17, 2023 at 5:29 PM Philipp Rudo prudo@redhat.com wrote:
With multiple kernel variants on the same architecture, e.g. the 4k and 64k kernel on aarch64, we can no longer assume that the crashkernel value for the currently running kernel will work for all installed kernels. This also means that we can no longer update the grub config as we don't know which value to set it to. Thus get the crashkernel value for each kernel and stop updating the grub config.
I think this patch should be the final stop of this series. Before it, we should provide an alternative interface to grub, which should be "kdumpctl crashkernel" sub-command.
But rebasing may be annoying, so I am fine with it.
Sorry, but I don't fully understand your comment here. Especially, what you mean with "alternative interface to grub".
Some background can be found in a bug (https://bugzilla.redhat.com/show_bug.cgi?id=2212320 ) In summary, currently, the upgrading of grub2 will re-generate the entries and the content comes from several files, including "/etc/kernel/cmdline". That is the place where kexec-tools exports "crashkernel=" to grub components.
So if you drop that, there is no way for grub to get "crashkernel=" at this point. But I admitted the grub has some breakage for the time being, and we are trying to figure out how to resolve it. (Maybe calling a interface exported by kexec-tools)
In general my long term goal is to get rid of grubby on the default paths and edit the BLS config files directly. Then we still need some --grub option that tells kdumpctl to also update the grub config. But in that case the user explicitly tells us to do the update so I think it is much safer to assume that grubby is installed. I've only omitted to include this feature in this series because the series is already is too long the way it is right now (and I already have some other additional patches in the pipeline for v2). Is that what you meant?
No, I just meant to re-order this patch to the rear of this series. But as said, there has already breakage here, and I has no strong opinion on this.
Finally, I'm totally fine to split out the patches to a separate series to get them applied early. If you prefer I can also move the two subcommand handling patches (patch 7 & 8) after this patch. Only the patch that simplifies _update_kernel_cmdline I would prefer to keep where it is as moving it it will cause conflicts I'd like to avoid.
Yes, I think it is a good idea to split the series and make it partially merged faster. From my point, [1-5, 9] can be treated as minal fix or trim. [6,7,8,10,11] focus on optimization of reset_crashkernel(). And the rest is the essence of this series, which introduces a new command.
Reviewed-by: Pingfan Liu piliu@redhat.com
Thanks a lot for the review!
YW.
Thanks,
Pingfan
Philipp
While at it merge the _new_fadump and _fadump_val variables and remove _read_kernel_arg_in_grub_etc_default which has no user.
Signed-off-by: Philipp Rudo prudo@redhat.com
kdumpctl | 96 +++----------------------------------------------------- 1 file changed, 5 insertions(+), 91 deletions(-)
diff --git a/kdumpctl b/kdumpctl index e9086b2..6008df8 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1542,69 +1542,9 @@ _get_all_kernels_from_grubby() echo -n "$_kernels" }
-GRUB_ETC_DEFAULT="/etc/default/grub" -# Update a kernel parameter in default grub conf -# -# If a value is specified, it will be inserted in the end. Otherwise it -# would remove given kernel parameter. -# -# Note this function doesn't address the following cases, -# 1. The kernel ignores everything on the command line after a '--'. So -# simply adding the new entry to the end will fail if the cmdline -# contains a --. -# 2. If the value for a parameter contains spaces it can be quoted using -# double quotes, for example param="value with spaces". This will -# break the [^[:space:]"] regex for the value. -# 3. Dashes and underscores in the parameter name are equivalent. So -# some_parameter and some-parameter are identical. -# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. -# 5. Some kernel parameters, e.g. quiet, doesn't have value -# -# $1: the name of the kernel command line parameter -# $2: new value. If empty, given parameter would be removed -_update_kernel_arg_in_grub_etc_default() -{
local _para=$1 _val=$2 _para_valif [[ $(uname -m) == s390x ]]; thenreturnfiif [[ -n $_val ]]; then_para_val="$_para=$_val"fi# Update the command line /etc/default/grub, i.e.# on the line that starts with 'GRUB_CMDLINE_LINUX=',# 1) remove $para=$val if the it's the first arg# 2) remove all occurences of $para=$val# 3) insert $_para_val to end# 4) remove duplicate spaces left over by 1) or 2) or 3)# 5) remove space at the beginning of the string left over by 1) or 2) or 3)# 6) remove space at the end of the string left over by 1) or 2) or 3)sed -i -E "/^GRUB_CMDLINE_LINUX=/ {s/\"${_para}=[^[:space:]\"]*/\"/g;s/[[:space:]]+${_para}=[^[:space:]\"]*/ /g;s/\"$/ ${_para_val}\"/s/[[:space:]]+/ /g;s/(\")[[:space:]]+/\1/g;s/[[:space:]]+(\")/\1/g;}" "$GRUB_ETC_DEFAULT"-}
-# Read the kernel arg in default grub conf.
-# Note reading a kernel parameter that doesn't have a value isn't supported. -# -# $1: the name of the kernel command line parameter -_read_kernel_arg_in_grub_etc_default() -{
sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]\"]${1}=([^[:space:]\"]*).*$/\1/p" "$GRUB_ETC_DEFAULT"-}
reset_crashkernel() {
local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels
local _opt _val _reboot _grubby_kernel_path _kernel _kernels local _dump_mode _new_dump_mode local _has_changed _needs_reboot local _old_ck _new_ck@@ -1617,13 +1557,12 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi
_val=${_opt#*=}if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then_fadump_val=$_valelse
_new_fadump=${_opt#*=}if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then derror "failed to determine dump mode" exit fi[[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=}@@ -1660,29 +1599,6 @@ reset_crashkernel()
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
# If the dump mode is determined, we can also know the default crashkernel valueif [[ -n $_dump_mode ]]; then_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")fi# If --kernel-path=ALL, update GRUB_CMDLINE_LINUX in /etc/default/grub.## An exception case is when the ppc64le user doesn't specify the fadump value.# In this case, the dump mode would be determined by parsing the kernel# command line of the kernel(s) to be updated thus don't update GRUB_CMDLINE_LINUX.## The following code has been simplified because of what has been done early,# - set the dump mode as kdump for non-ppc64le cases# - retrieved the default crashkernel value for given dump modeif [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then_update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel"# remove the fadump if fadump is disabledif [[ $_fadump_val == off ]]; then_fadump_val=""fi_update_kernel_arg_in_grub_etc_default fadump "$_fadump_val"fi# If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel@@ -1700,15 +1616,13 @@ reset_crashkernel() _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode_new_ck=$_crashkernel_new_fadump=$_fadump_val fi _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel)
_new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then _has_changed="Updated fadump=$_new_fadump"-- 2.40.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
When handling fadump there are three cases we need to consider
1) When running on non-ppc64le systems In this case _dump_mode=kdump and _new_fadump='' always. In other words we don't need to care updating the fadump parameter on the kernel command line. We could always remove it like the code did so far. But should we remove it when we never set it? In particular as that might overwrite a change explicitly made by the user (for whatever reason)
2) When running on ppc64le and the user provided --fadump option In this case _new_fadump and _dump_mode are set accordingly to what the user provided. Thus we need to update both the crashkernel (in case fadump was turned on/off) and the fadump (in case the fadump mode changed) parameters.
3) When running on ppc64le but the user did not provide --fadump option In this case both _new_fadump='' and _dump_mode=''. In this case we take over the previously set fadump parameter. Which means that we don't need to update the fadump parameter at all. We do need to check whether the _new_dump_mode is fadump or kdump though so we use the correct (new) default crashkernel value.
In the three cases only the second one needs to update the fadump parameter. Reflect this discussion in code.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 6008df8..23e6e5a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1545,10 +1545,10 @@ _get_all_kernels_from_grubby() reset_crashkernel() { local _opt _val _reboot _grubby_kernel_path _kernel _kernels - local _dump_mode _new_dump_mode + local _dump_mode local _has_changed _needs_reboot local _old_ck _new_ck - local _old_fadump _new_fadump + local _old_fadump _new_fadump _opt_fadump
for _opt in "$@"; do case "$_opt" in @@ -1557,12 +1557,11 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi - _new_fadump=${_opt#*=} - if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then + _opt_fadump=${_opt#*=} + if ! _dump_mode=$(get_dump_mode_by_fadump_val $_opt_fadump); then derror "failed to determine dump mode" exit fi - [[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=} @@ -1597,8 +1596,6 @@ reset_crashkernel() return fi
- [[ $(uname -m) != ppc64le ]] && _dump_mode=kdump - # If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel @@ -1614,19 +1611,28 @@ reset_crashkernel()
for _kernel in $_kernels; do _has_changed="" - if [[ -z $_dump_mode ]]; then - _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + if [[ $(uname -m) == ppc64le ]]; then + if [[ -n "$_opt_fadump" ]]; then + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") + _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + _new_fadump="$_opt_fadump" + [[ "$_new_fadump" == off ]] && _new_fadump="" + if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then + if [[ -n "$_new_fadump" ]]; then + _has_changed="Updated fadump=$_new_fadump" + else + _has_changed="Removed fadump" + fi + fi + else + _dump_mode="$(get_dump_mode_by_kernel "$_kernel")" + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") + fi else - _new_dump_mode=$_dump_mode + _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) fi
_old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") - _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then - _has_changed="Updated fadump=$_new_fadump" - fi if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then _has_changed="Updated crashkernel=$_new_ck" fi
In preparation to rewrite the CLI switch from a self written option parser to getopt. This not only makes it easier to add options to other subcommands (or globally) but also gives more flexibility to the user. For example there is no more requirement to pass option arguments with a '=', i.e. like --option=argument.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 200 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 160 insertions(+), 40 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 23e6e5a..754f180 100755 --- a/kdumpctl +++ b/kdumpctl @@ -42,6 +42,37 @@ cmds_global[_reset-crashkernel-after-update]=reset_crashkernel_after_update # shellcheck disable=SC2034 cmds_global[_reset-crashkernel-for-installed_kernel]=reset_crashkernel_for_installed_kernel
+# shellcheck disable=SC2034 +opts_short_reset_crashkernel="" +# shellcheck disable=SC2034 +opts_long_reset_crashkernel=( + --long "fadump:" + --long "kernel:" + --long "reboot" +) + +parse_args_reset_crashkernel() +{ + case $1 in + --fadump) + if [[ $(uname -a) != ppc64le ]]; then + derror "Option --fadump only valid on PPC" + exit 1 + fi + OPT[fadump]="$2" + return 2 + ;; + --kernel) + OPT[kernel]="$2" + return 2 + ;; + --reboot) + OPT[reboot]=yes + return 1 + ;; + esac +} + if [[ -f /etc/sysconfig/kdump ]]; then . /etc/sysconfig/kdump fi @@ -73,6 +104,11 @@ trap ' exit $ret; ' EXIT
+has_function() +{ + declare -F "$1" > /dev/null +} + _usage() { [[ -n "$*" ]] && dinfo "$*" @@ -82,6 +118,113 @@ _usage() exit 1 }
+# shellcheck disable=SC2120 +_parse_args() +{ + local _tmp _parser _ret + + _tmp=$( getopt \ + -o "+$__shortopts" \ + "${__longopts[@]}" \ + -- "${__argv[@]}" + ) + + # shellcheck disable=SC2181 + if (($? != 0)); then + _usage + exit 1 + fi + + eval set -- "$_tmp" + while :; do + if [[ "$1" == -- ]]; then + shift + break + fi + + for _parser in "${__parsers[@]}"; do + $_parser "$@" + _ret=$? + if [[ $_ret -gt 0 ]]; then + shift $_ret + continue 2 + fi + done + + # This point should never be reached, unless there is an option + # defined for getopt that none of the parsers can handle. + stderr "Missing parser for option $1" + exit + done + __argv=("$@") +} + +__switch_cmd() +{ + local -n _shortopts _longopts _cmds + local _cmd _parser k + + _cmd=$1 + + [[ -n "$_cmd" ]] || return + + _shortopts=opts_short_${_cmd} + _longopts=opts_long_${_cmd} + _parser=parse_args_${_cmd} + _cmds=cmds_${_cmd} + + has_function "$_parser" || return + + __shortopts+="$_shortopts" + __longopts+=("${_longopts[@]}") + __parsers+=("$_parser") + + __cmds=() + for k in "${!_cmds[@]}"; do + __cmds[$k]="${_cmds[$k]}" + done +} + +parse_args() +{ + # Caution: These are global variables within the parser + local -a __argv __longopts __parsers + local -A __cmds + local __shortopts + + # Return via name references + local -n __args __func + local _cmd _tmp_func + + __cmd="$1"; shift + __func="$1"; shift + __args="$1"; shift + __argv=("$@") + + while :; do + __cmd=${__cmd//-/_} + __switch_cmd "$__cmd" + _parse_args + + [[ ${#__argv[@]} -eq 0 ]] && break + [[ -n "${__cmds["${__argv[0]}"]}" ]] || break + _tmp_func="${__cmds["${__argv[0]}"]}" + if [[ "$__cmd" == global ]]; then + __cmd="${__argv[0]}" + else + __cmd="${__cmd}_${__argv[0]}" + fi + unset "__argv[0]" + # Fix indices in the array + __argv=("${__argv[@]}") + done + + __func="$_tmp_func" + __args="${__argv[*]}" + + return +} + _get_dracut_arg() { local shortopt longopt n tmp @@ -1544,42 +1687,18 @@ _get_all_kernels_from_grubby()
reset_crashkernel() { - local _opt _val _reboot _grubby_kernel_path _kernel _kernels - local _dump_mode + local _kernel _kernels _dump_mode local _has_changed _needs_reboot local _old_ck _new_ck - local _old_fadump _new_fadump _opt_fadump - - for _opt in "$@"; do - case "$_opt" in - --fadump=*) - if [[ $(uname -m) != ppc64le ]]; then - derror "Option --fadump only valid on PPC" - exit 1 - fi - _opt_fadump=${_opt#*=} - if ! _dump_mode=$(get_dump_mode_by_fadump_val $_opt_fadump); then - derror "failed to determine dump mode" - exit - fi - ;; - --kernel=*) - _val=${_opt#*=} - if ! _valid_grubby_kernel_path $_val; then - derror "Invalid $_opt, please specify a valid kernel path, ALL or DEFAULT" - exit - fi - _grubby_kernel_path=$_val - ;; - --reboot) - _reboot=yes - ;; - *) - derror "$_opt not recognized" - exit 1 - ;; - esac - done + local _old_fadump _new_fadump + local func args + + parse_args reset-crashkernel func args "$@" + eval set -- "$args" + + if [[ -n "$1" ]]; then + _usage "Additional parameter '$1' provided." + fi
# 1. OSTree systems use "rpm-ostree kargs" instead of grubby to manage kernel command # line. --kernel=ALL doesn't make sense for OStree. @@ -1591,7 +1710,7 @@ reset_crashkernel() _old_ck=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) if _update_kernel_cmdline "" crashkernel "$_old_ck" "$_new_ck"; then - [[ $_reboot == yes ]] && systemctl reboot + [[ ${OPT[reboot]} == yes ]] && systemctl reboot fi return fi @@ -1599,23 +1718,24 @@ reset_crashkernel() # If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel - if [[ -z $_grubby_kernel_path ]]; then + if [[ -z ${OPT[kernel]} ]]; then [[ -n $KDUMP_KERNELVER ]] || KDUMP_KERNELVER=$(uname -r) if ! _kernels=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then derror "no kernel for version $KDUMP_KERNELVER found" exit 1 fi else - _kernels=$(_get_all_kernels_from_grubby "$_grubby_kernel_path") + _kernels=$(_get_all_kernels_from_grubby "${OPT[kernel]}") fi
for _kernel in $_kernels; do _has_changed="" if [[ $(uname -m) == ppc64le ]]; then - if [[ -n "$_opt_fadump" ]]; then + if [[ -n "${OPT[fadump]}" ]]; then + _dump_mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - _new_fadump="$_opt_fadump" + _new_fadump="${OPT[fadump]}" [[ "$_new_fadump" == off ]] && _new_fadump="" if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then if [[ -n "$_new_fadump" ]]; then @@ -1642,7 +1762,7 @@ reset_crashkernel() fi done
- if [[ $_reboot == yes && $_needs_reboot == 1 ]]; then + if [[ ${OPT[reboot]} == yes && $_needs_reboot == 1 ]]; then systemctl reboot fi }
With the arrays introduced for getopt it's pretty simple to add some bash completion. Let's make our lives not harder than they have to be and add some.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 3 ++ kdumpctl.bash-completion | 107 +++++++++++++++++++++++++++++++++++++++ kexec-tools.spec | 4 ++ 3 files changed, 114 insertions(+) create mode 100755 kdumpctl.bash-completion
diff --git a/kdumpctl b/kdumpctl index 754f180..6b545e5 100755 --- a/kdumpctl +++ b/kdumpctl @@ -73,6 +73,9 @@ parse_args_reset_crashkernel() esac }
+# Only do the bare minimum when sourced during bash completion +[[ ${COMP_LINE:+x} ]] && return + if [[ -f /etc/sysconfig/kdump ]]; then . /etc/sysconfig/kdump fi diff --git a/kdumpctl.bash-completion b/kdumpctl.bash-completion new file mode 100755 index 0000000..b0fe92e --- /dev/null +++ b/kdumpctl.bash-completion @@ -0,0 +1,107 @@ +#!/usr/bin/bash +# shellcheck disable=SC1091 + +source /usr/share/bash-completion/bash_completion + +_is_defined() +{ + declare -p "$1" &> /dev/null +} + +_is_subcmd() +{ + local _cmd _subcmd + local -n _cmds + + _cmd="$1" + _subcmd="$2" + + _cmds=cmds_${_cmd} + _is_defined "cmds_${_cmd}" || return + + [[ -n "${_subcmd}" ]] || return + [[ -n "${_cmds["$_subcmd"]}" ]] +} + +_add_cmds() +{ + local -n _cmds + local _cmd + + _is_defined "$1" || return + + _cmds="$1" + for _cmd in "${!_cmds[@]}"; do + [[ "${_cmd:0:1}" == "_" ]] && unset '_cmds[$_cmd]' + done + + # shellcheck disable=SC2207 + COMPREPLY+=($(compgen -W "${!_cmds[*]}" -- "$cur")) +} + +_add_opts() +{ + local _opt + local -a _list + local -n _opts + + _is_defined "$1" || return + + _opts="$1" + for _opt in "${_opts[@]}"; do + [[ "$_opt" == "--long" ]] && continue + _list+=("--${_opt%:}") + done + + # shellcheck disable=SC2207 + COMPREPLY+=($(compgen -W "${_list[*]}" -- "$cur")) +} + +__kdumpctl() { + # shellcheck disable=SC2034 + local cur prev words cword + local _word _cmd _longopts + + _init_completion || return + + source /usr/bin/kdumpctl + + # make the completion work with --opt= style options. Where the '=' gets + # split into its own word. I.e. --opt= becomes ( '--opt' '=' ) and + # --opt=foo becomes ( '--opt' '=' 'foo' ). Hence adjust cur and prev for + # both cases. + [[ $cur == = ]] && cur="" + [[ $prev == = ]] && prev=${words[-3]} + + case "$prev" in + --fadump) + # shellcheck disable=SC2207 + COMPREPLY+=($(compgen -W "on off nocma" -- "$cur")) + return + ;; + --kernel) + _filedir + # shellcheck disable=SC2207 + COMPREPLY+=($(compgen -W "ALL DEFAULT" -- "$cur")) + return + ;; + esac + + _cmd=global + _add_opts opts_long_global + for _word in "${words[@]}"; do + _longopts=opts_long_${_word} + _is_defined "$_longopts" && _add_opts "$_longopts" + + if _is_subcmd "$_cmd" "$_word"; then + if [[ "$_cmd" == global ]]; then + _cmd="$_word" + else + _cmd="${_cmd}_${_word}" + fi + fi + done + + _is_defined "cmds_${_cmd}" && _add_cmds "cmds_${_cmd}" +} && + complete -F __kdumpctl kdumpctl diff --git a/kexec-tools.spec b/kexec-tools.spec index d7d2947..277b5fc 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -39,6 +39,7 @@ Source34: crashkernel-howto.txt Source35: kdump-migrate-action.sh Source36: kdump-restart.sh Source37: 60-fadump.install +Source38: kdumpctl.bash-completion
####################################### # These are sources for mkdumpramfs @@ -172,6 +173,7 @@ mkdir -p -m755 $RPM_BUILD_ROOT%{_mandir}/man8/ mkdir -p -m755 $RPM_BUILD_ROOT%{_mandir}/man5/ mkdir -p -m755 $RPM_BUILD_ROOT%{_docdir} mkdir -p -m755 $RPM_BUILD_ROOT%{_datadir}/kdump +mkdir -p -m755 $RPM_BUILD_ROOT%{_datadir}/bash-completion/completions/ mkdir -p -m755 $RPM_BUILD_ROOT%{_udevrulesdir} mkdir -p $RPM_BUILD_ROOT%{_unitdir} mkdir -p -m755 $RPM_BUILD_ROOT%{_bindir} @@ -191,6 +193,7 @@ install -m 644 kdump.sysconfig $RPM_BUILD_ROOT%{_sysconfdir}/sysconfig/kdump install -m 644 kexec/kexec.8 $RPM_BUILD_ROOT%{_mandir}/man8/kexec.8 install -m 644 %{SOURCE12} $RPM_BUILD_ROOT%{_mandir}/man8/mkdumprd.8 install -m 644 %{SOURCE25} $RPM_BUILD_ROOT%{_mandir}/man8/kdumpctl.8 +install -m 755 %{SOURCE38} $RPM_BUILD_ROOT%{_datadir}/bash-completion/completions/kdumpctl install -m 755 %{SOURCE20} $RPM_BUILD_ROOT%{_prefix}/lib/kdump/kdump-lib.sh install -m 755 %{SOURCE23} $RPM_BUILD_ROOT%{_prefix}/lib/kdump/kdump-lib-initramfs.sh install -m 755 %{SOURCE31} $RPM_BUILD_ROOT%{_prefix}/lib/kdump/kdump-logger.sh @@ -368,6 +371,7 @@ fi %dir %{_sysconfdir}/kdump/pre.d %dir %{_sysconfdir}/kdump/post.d %dir %{_sharedstatedir}/kdump +%{_datadir}/bash-completion/completions/kdumpctl %{_mandir}/man8/kdumpctl.8.gz %{_mandir}/man8/kexec.8.gz %ifarch %{ix86} x86_64 ppc64 s390x ppc64le aarch64
In order to be able to use --kernel with other subcommands create global options and move it there.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 6b545e5..8ca2614 100755 --- a/kdumpctl +++ b/kdumpctl @@ -42,12 +42,28 @@ cmds_global[_reset-crashkernel-after-update]=reset_crashkernel_after_update # shellcheck disable=SC2034 cmds_global[_reset-crashkernel-for-installed_kernel]=reset_crashkernel_for_installed_kernel
+# shellcheck disable=SC2034 +opts_short_global="" +# shellcheck disable=SC2034 +opts_long_global=( + --long "kernel:" +) + +parse_args_global() +{ + case $1 in + --kernel) + OPT[kernel]="$2" + return 2 + ;; + esac +} + # shellcheck disable=SC2034 opts_short_reset_crashkernel="" # shellcheck disable=SC2034 opts_long_reset_crashkernel=( --long "fadump:" - --long "kernel:" --long "reboot" )
@@ -62,10 +78,6 @@ parse_args_reset_crashkernel() OPT[fadump]="$2" return 2 ;; - --kernel) - OPT[kernel]="$2" - return 2 - ;; --reboot) OPT[reboot]=yes return 1 @@ -1696,9 +1708,6 @@ reset_crashkernel() local _old_fadump _new_fadump local func args
- parse_args reset-crashkernel func args "$@" - eval set -- "$args" - if [[ -n "$1" ]]; then _usage "Additional parameter '$1' provided." fi @@ -1862,16 +1871,22 @@ reset_crashkernel_for_installed_kernel()
main() { - local cmd + local func args + + parse_args global func args "$@" + eval set -- "$args"
- [[ -n $1 ]] || _usage "No sub-command provided" - cmd=${cmds_global[$1]} - [[ -n $cmd ]] || _usage "Unknown option or sub-command '$1' provided" - shift + if [[ -z "$func" ]]; then + if [[ -z "$1" ]]; then + _usage "No command provided." + else + _usage "Unknown command '$1' provided." + fi + fi
# Determine if the dump mode is kdump or fadump determine_dump_mode - $cmd "$@" + $func "$@" }
if [[ ${__SOURCED__:+x} ]]; then
Most users will expect such an option. So add it.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/kdumpctl b/kdumpctl index 8ca2614..6987dec 100755 --- a/kdumpctl +++ b/kdumpctl @@ -43,15 +43,20 @@ cmds_global[_reset-crashkernel-after-update]=reset_crashkernel_after_update cmds_global[_reset-crashkernel-for-installed_kernel]=reset_crashkernel_for_installed_kernel
# shellcheck disable=SC2034 -opts_short_global="" +opts_short_global="h" # shellcheck disable=SC2034 opts_long_global=( + --long "help" --long "kernel:" )
parse_args_global() { case $1 in + -h|--help) + _help + exit 0 + ;; --kernel) OPT[kernel]="$2" return 2 @@ -133,6 +138,38 @@ _usage() exit 1 }
+_help() +{ + cat << EOF +Usage: $PROGNAME [OPTIONS] COMMAND + +Controll interface for the kdump service +Options: + -h, --help Print this help message + --kernel <kernel image> + The kernel image to use. This option allows the special keyword + ALL to allow editing all installed kernels (not applicaple with + all commands). + +Options for reset-crashkernel: + --fadump [on|off|nocma] + The mode fadump should run in (only valid for PPC). + --reboot Automatically reboot if the kernel command line changed. + +Common commands: + status Print current status of the service + start Start the service + stop Stop the service + restart Equivalent to stop; start + rebuild Force rebuild the kdump initramfs + propagate Setup key authentication for ssh dump targets + reset-crashkernel + Reset the crashkernel parameter to its default value + +See 'man $PROGNAME' for more details and the full list of commands. +EOF +} + # shellcheck disable=SC2120 _parse_args() {
Add the --kver option. It is identical to setting KDUMP_KERNELVER in /etc/sysconfig/kdump. Providing --kver on the command line takes precedence over KDUMP_KERNELVER if both are used.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 17 +++++++++++++++++ kdumpctl.bash-completion | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+)
diff --git a/kdumpctl b/kdumpctl index 6987dec..15b4544 100755 --- a/kdumpctl +++ b/kdumpctl @@ -48,6 +48,7 @@ opts_short_global="h" opts_long_global=( --long "help" --long "kernel:" + --long "kver:" )
parse_args_global() @@ -61,6 +62,10 @@ parse_args_global() OPT[kernel]="$2" return 2 ;; + --kver) + KDUMP_KERNELVER="$2" + return 2 + ;; esac }
@@ -150,6 +155,9 @@ Options: The kernel image to use. This option allows the special keyword ALL to allow editing all installed kernels (not applicaple with all commands). + --kver <kernel version> + The kernel version to use. This option is only required if the + version cannot be determined from the kernel image.
Options for reset-crashkernel: --fadump [on|off|nocma] @@ -1906,6 +1914,11 @@ reset_crashkernel_for_installed_kernel() _update_crashkernel "$_installed_kernel" }
+_is_valid_kver() +{ + [[ -f /usr/lib/modules/$1/modules.dep ]] +} + main() { local func args @@ -1921,6 +1934,10 @@ main() fi fi
+ if [[ -n "$KDUMP_KERNELVER" ]] && ! _is_valid_kver "$KDUMP_KERNELVER"; then + _usage "Invalid kernel version $KDUMP_KERNELVER provided" + fi + # Determine if the dump mode is kdump or fadump determine_dump_mode $func "$@" diff --git a/kdumpctl.bash-completion b/kdumpctl.bash-completion index b0fe92e..8e7d099 100755 --- a/kdumpctl.bash-completion +++ b/kdumpctl.bash-completion @@ -57,6 +57,20 @@ _add_opts() COMPREPLY+=($(compgen -W "${_list[*]}" -- "$cur")) }
+_kver() { + local _ver + local -a _ret + + for _ver in /usr/lib/modules/*; do + _ver=${_ver#/usr/lib/modules/} + _ver=${_ver%%/*} + _ret+=("$_ver") + done + + # shellcheck disable=SC2207 + COMPREPLY+=($(compgen -W "${_ret[*]}" -- "$cur")) +} + __kdumpctl() { # shellcheck disable=SC2034 local cur prev words cword @@ -85,6 +99,10 @@ __kdumpctl() { COMPREPLY+=($(compgen -W "ALL DEFAULT" -- "$cur")) return ;; + --kver) + _kver + return + ;; esac
_cmd=global
Currently the --kernel option relies on grubby to resolve the paths to the kernel image. This is somewhat problematic as grubby is not installed all the time. Furthermore it only works with installed kernels. So developer cannot use a self compiled kernel directly but has to install it first. Solve this by emulating the behaviour of grubbys --kernel option in kdumpctl.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 186 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 166 insertions(+), 20 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 15b4544..6d90b21 100755 --- a/kdumpctl +++ b/kdumpctl @@ -3,6 +3,7 @@ KEXEC=/sbin/kexec
KDUMP_KERNELVER="" KDUMP_KERNEL="" +declare -A KDUMP_KERNEL_LIST KDUMP_COMMANDLINE="" KEXEC_ARGS="" KDUMP_LOG_PATH="/var/log" @@ -1724,11 +1725,6 @@ _update_kernel_cmdline() fi }
-_valid_grubby_kernel_path() -{ - [[ -n "$1" ]] && grubby --info="$1" > /dev/null 2>&1 -} - # return all the kernel paths given a grubby kernel-path # # $1: kernel path accepted by grubby, e.g. DEFAULT, ALL, @@ -1747,7 +1743,7 @@ _get_all_kernels_from_grubby()
reset_crashkernel() { - local _kernel _kernels _dump_mode + local _kernel _dump_mode local _has_changed _needs_reboot local _old_ck _new_ck local _old_fadump _new_fadump @@ -1772,20 +1768,7 @@ reset_crashkernel() return fi
- # If kernel-path not specified, either - # - use KDUMP_KERNELVER if it's defined - # - use current running kernel - if [[ -z ${OPT[kernel]} ]]; then - [[ -n $KDUMP_KERNELVER ]] || KDUMP_KERNELVER=$(uname -r) - if ! _kernels=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then - derror "no kernel for version $KDUMP_KERNELVER found" - exit 1 - fi - else - _kernels=$(_get_all_kernels_from_grubby "${OPT[kernel]}") - fi - - for _kernel in $_kernels; do + for _kernel in "${KDUMP_KERNEL_LIST[@]}"; do _has_changed="" if [[ $(uname -m) == ppc64le ]]; then if [[ -n "${OPT[fadump]}" ]]; then @@ -1914,11 +1897,167 @@ reset_crashkernel_for_installed_kernel() _update_crashkernel "$_installed_kernel" }
+_init_systeminfo() +{ + local line + + while read -r line; do + case ${line%%:*} in + KERNEL_INSTALL_BOOT_ROOT) + [[ -n $KDUMP_BOOTDIR ]] && continue + KDUMP_BOOTDIR=${line##*: } + ;; + KERNEL_INSTALL_ENTRY_TOKEN) + BLS_ENTRY_TOKEN=${line##*: } + ;; + esac + done < <(kernel-install inspect) + BLS_CONFDIR=$KDUMP_BOOTDIR/loader/entries + BLS_UKIDIR=$KDUMP_BOOTDIR/efi/EFI/Linux +} + _is_valid_kver() { [[ -f /usr/lib/modules/$1/modules.dep ]] }
+_is_rescue_kernel() +{ + [[ "$1" == *rescue* ]] +} + +_parse_kver() +{ + local _img _kver + + [[ -z "$1" ]] && return + + _img=$1 + + # Fedora standard installation, i.e. $BOOT/vmlinuz-<version> + _kver=${_img##*/vmlinuz-} + _kver=${_kver%"$KDUMP_IMG_EXT"} + if _is_valid_kver "$_kver"; then + echo "$_kver" + return + fi + + # BLS recommended image names, i.e. $BOOT/<token>/<version>/linux + _kver=${_img##*/"$BLS_ENTRY_TOKEN"/} + _kver=${_kver%%/*} + if _is_valid_kver "$_kver"; then + echo "$_kver" + return + fi + + # Fedora UKI installation, i.e. $BOOT/efi/EFI/Linux/<token>-<version>.efi + _kver=${_img##*/"$BLS_ENTRY_TOKEN"-} + _kver=${_kver%.efi} + if _is_valid_kver "$_kver"; then + echo "$_kver" + return + fi + + ddebug "Could not parse version from $_img" + echo "UNKNOWN" +} + +# _get_installed_kernel +# Returns a line with <path to kernel img> <kver> for each installed kernel +# The function assumes that every installed kernel has a valid BLS entry +_get_installed_kernel() +{ + local _img + + while read -r _img; do + # On s390 the paths in the bls type 1 configs are absolute, not + # relative to the boot partition + [[ -f "$_img" ]] || _img="$KDUMP_BOOTDIR/${_img#/}" + [[ -f "$_img" ]] || continue + _is_rescue_kernel "$_img" && continue + + echo "$_img $(_parse_kver "$_img") " + done < <(sed -n -e "s/^linux (.*)$/\1/p" \ + "$BLS_CONFDIR"/"$BLS_ENTRY_TOKEN"-*.conf \ + "$BLS_CONFDIR"/ostree-*.conf \ + 2> /dev/null) + + for _img in "$BLS_UKIDIR"/"$BLS_ENTRY_TOKEN"-*.efi; do + [[ -f $_img ]] || continue + _is_rescue_kernel "$_img" && continue + + echo "$_img $(_parse_kver "$_img")" + done +} + +_setup_kernel_list() +{ + local _opt_kernel _opt_kver _img _kver _uname + + _opt_kernel="$1" + _opt_kver="$2" + + KDUMP_KERNEL_LIST=() + + # case: --kernel=<path to image> + if [[ -f "$_opt_kernel" ]]; then + _img="$1" + _kver=$(_parse_kver "$_img") + KDUMP_KERNEL_LIST[$_kver]=$_img + + # case: no --kernel= given. + # Also use this case for CoreOS to prevent the special cases DEFAULT + # and ALL. DEFAULT requires grubby which isn't available on CoreOS. + # ALL doesn't make sense as with CoreOS' A/B update scheme you should + # always only make changes to the current OS image. + elif [[ -z "$_opt_kernel" ]] || is_ostree; then + _uname=${_opt_kver:-"$(uname -r)"} + while read -r _img _kver; do + [[ "$_uname" == "$_kver" ]] || continue + KDUMP_KERNEL_LIST[$_kver]=$_img + break + done < <(_get_installed_kernel) + + # case: --kernel=ALL + elif [[ "$_opt_kernel" == ALL ]]; then + while read -r _img _kver; do + if [[ "$_kver" == UNKNOWN ]]; then + derror "Could not parse kernel version from '$_img'" + continue + fi + KDUMP_KERNEL_LIST[$_kver]=$_img + done < <(_get_installed_kernel) + return + + # case: --kernel=DEFAULT + elif [[ "$_opt_kernel" == DEFAULT ]]; then + if ! has_command grubby; then + derror "Special keyword DEFAULT requires grubby" + exit 1 + fi + _img=$(grubby --info=DEFAULT) + _img=${_img##*kernel="} + _img=${_img%%"*} + _kver=$(_parse_kver "$_img") + KDUMP_KERNEL_LIST[$_kver]=$_img + + else + derror "Invalid kernel image $2 provided" + exit 1 + fi + + if [[ -n "${KDUMP_KERNEL_LIST[UNKNOWN]}" ]]; then + if [[ -z "$_opt_kver" ]]; then + derror "Could not parse kernel version from ${KDUMP_KERNEL_LIST[UNKNOWN]}" + derror "Please provide it using the --kver option" + exit 1 + else + KDUMP_KERNEL_LIST[$_opt_kver]="${KDUMP_KERNEL_LIST[UNKNOWN]}" + unset 'KDUMP_KERNEL_LIST[UNKNOWN]' + fi + fi +} + main() { local func args @@ -1938,6 +2077,13 @@ main() _usage "Invalid kernel version $KDUMP_KERNELVER provided" fi
+ _init_systeminfo + _setup_kernel_list "${OPT[kernel]}" "$KDUMP_KERNELVER" + if [[ ${#KDUMP_KERNEL_LIST[@]} -eq 0 ]]; then + derror "Could not find any installed kernel" + exit 1 + fi + # Determine if the dump mode is kdump or fadump determine_dump_mode $func "$@"
With the new, global --kernel and --kver options both cases can be simplified to a point where they are almost identical. Merge them into a single pkg_upgrade_hook to simplify the CLI and have a more recognizable function naming.
While at it move the special handling for CoreOS from the spec file to the new hook so there is a single place with all exceptions.
Signed-off-by: Philipp Rudo prudo@redhat.com --- 92-crashkernel.install | 2 +- kdumpctl | 83 ++++++++---------------------------------- kexec-tools.spec | 11 +++--- 3 files changed, 22 insertions(+), 74 deletions(-)
diff --git a/92-crashkernel.install b/92-crashkernel.install index 19bd078..bd7c27e 100755 --- a/92-crashkernel.install +++ b/92-crashkernel.install @@ -7,7 +7,7 @@ KERNEL_IMAGE="$4"
case "$COMMAND" in add) - kdumpctl _reset-crashkernel-for-installed_kernel "$KERNEL_VERSION" + kdumpctl _pkg-upgrade-hook kernel --kver "$KERNEL_VERSION" exit 0 ;; esac diff --git a/kdumpctl b/kdumpctl index 6d90b21..603c8e9 100755 --- a/kdumpctl +++ b/kdumpctl @@ -39,9 +39,8 @@ cmds_global[showmem]=show_reserved_mem cmds_global[estimate]=do_estimate cmds_global[get-default-crashkernel]=get_default_crashkernel cmds_global[reset-crashkernel]=reset_crashkernel -cmds_global[_reset-crashkernel-after-update]=reset_crashkernel_after_update # shellcheck disable=SC2034 -cmds_global[_reset-crashkernel-for-installed_kernel]=reset_crashkernel_for_installed_kernel +cmds_global[_pkg-upgrade-hook]=pkg_upgrade_hook
# shellcheck disable=SC2034 opts_short_global="h" @@ -1670,24 +1669,6 @@ get_dump_mode_by_kernel() fi }
-_filter_grubby_kernel_str() -{ - local _grubby_kernel_str=$1 - echo -n "$_grubby_kernel_str" | sed -n -e 's/^kernel="(.*)"/\1/p' -} - -_find_kernel_path_by_release() -{ - local _release="$1" _grubby_kernel_str _kernel_path - _grubby_kernel_str=$(grubby --info ALL | grep -E "^kernel=.*$_release(/\w+)?"$") - _kernel_path=$(_filter_grubby_kernel_str "$_grubby_kernel_str") - if [[ -z $_kernel_path ]]; then - ddebug "kernel $_release doesn't exist" - return 1 - fi - echo -n "$_kernel_path" -} - _update_kernel_cmdline() { local _kernel _param _old _new @@ -1725,22 +1706,6 @@ _update_kernel_cmdline() fi }
-# return all the kernel paths given a grubby kernel-path -# -# $1: kernel path accepted by grubby, e.g. DEFAULT, ALL, -# /boot/vmlinuz-`uname -r` -# return: kernel paths separated by space -_get_all_kernels_from_grubby() -{ - local _kernels _line _kernel_path _grubby_kernel_path=$1 - - for _line in $(grubby --info "$_grubby_kernel_path" | grep "^kernel="); do - _kernel_path=$(_filter_grubby_kernel_str "$_line") - _kernels="$_kernels $_kernel_path" - done - echo -n "$_kernels" -} - reset_crashkernel() { local _kernel _dump_mode @@ -1833,23 +1798,6 @@ _update_crashkernel() fi }
-reset_crashkernel_after_update() -{ - local _kernel - - if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then - return - fi - - if ! _is_bootloader_installed; then - return - fi - - for _kernel in $(_get_all_kernels_from_grubby ALL); do - _update_crashkernel "$_kernel" - done -} - # read the value of an environ variable from given environ file path # # The environment variable entries in /proc/[pid]/environ are separated @@ -1869,32 +1817,33 @@ _is_osbuild() [[ $(read_proc_environ_var container "$_OSBUILD_ENVIRON_PATH") == bwrap-osbuild ]] }
-reset_crashkernel_for_installed_kernel() +pkg_upgrade_hook() { - local _installed_kernel + local _pkg
if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then return fi
- # During package install, only try to reset crashkernel for osbuild - # thus to avoid calling grubby when installing os via anaconda - if ! _is_bootloader_installed && ! _is_osbuild; then - return - fi - - if ! _installed_kernel=$(_find_kernel_path_by_release "$1"); then - exit 1 - fi + _pkg=$1
- if _is_osbuild; then + if [[ $_pkg == kernel ]] && _is_osbuild; then if ! grep -qs crashkernel= /etc/kernel/cmdline; then - reset_crashkernel "--kernel=$_installed_kernel" + reset_crashkernel "--kernel=${KDUMP_KERNEL_LIST["$KDUMP_KERNELVER"]}" fi return + elif [[ $_pkg == kexec-tools ]]; then + # automatic handling of crashkernel is not supported on CoreOS + is_ostree && exit 0 fi
- _update_crashkernel "$_installed_kernel" + _is_bootloader_installed || return + + for KDUMP_KERNELVER in "${!KDUMP_KERNEL_LIST[@]}"; do + KDUMP_KERNEL="${KDUMP_KERNEL_LIST["$KDUMP_KERNELVER"]}" + _update_crashkernel "$KDUMP_KERNEL" + done + }
_init_systeminfo() diff --git a/kexec-tools.spec b/kexec-tools.spec index 277b5fc..1375089 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -332,12 +332,11 @@ done # Try to reset kernel crashkernel value to new default value or set up # crasherkernel value for new install # -# Note -# 1. Skip ostree systems as they are not supported. -# 2. For Fedora 36 and RHEL9, "[ $1 == 1 ]" in posttrans scriptlet means both install and upgrade; -# For Fedora > 36, "[ $1 == 1 ]" only means install and "[ $1 == 2 ]" means upgrade -if [ ! -f /run/ostree-booted ] && [ $1 == 1 -o $1 == 2 ]; then - kdumpctl _reset-crashkernel-after-update +# Note: For Fedora < 36 and RHEL9, "[ $1 == 1 ]" in posttrans scriptlet means +# both install and upgrade; For Fedora > 36, "[ $1 == 1 ]" only means +# install and "[ $1 == 2 ]" means upgrade +if [ $1 == 1 -o $1 == 2 ]; then + kdumpctl _pkg-upgrade-hook kexec-tools --kernel=ALL : fi
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 6 ++++++ kdumpctl | 25 ++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index c95cc94..8112dff 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -502,6 +502,12 @@ prepare_kdump_kernel() { local kdump_kernelver=$1 local dir img boot_dirlist boot_imglist kdump_kernel machine_id + + if [[ -n "$KDUMP_KERNEL" ]]; then + echo "$KDUMP_KERNEL" + return + fi + read -r machine_id < /etc/machine-id
boot_dirlist=${KDUMP_BOOTDIR:-"/boot /boot/efi /efi /"} diff --git a/kdumpctl b/kdumpctl index 603c8e9..07b4382 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1298,6 +1298,14 @@ __start()
_start() { + if [[ ${#KDUMP_KERNEL_LIST[@]} -ne 1 ]]; then + derror "Cannot use --kernel=ALL with command 'start'." + exit 1 + fi + + KDUMP_KERNELVER=${!KDUMP_KERNEL_LIST[0]} + KDUMP_KERNEL=${KDUMP_KERNEL_LIST[0]} + if ! __start; then derror "Starting kdump: [FAILED]" exit 1 @@ -1428,12 +1436,23 @@ _restart()
_rebuild() { + local _ret + parse_config || return 1 check_and_wait_network_ready || return 1
- setup_initrd || return 1 - - rebuild_initrd + for KDUMP_KERNELVER in "${!KDUMP_KERNEL_LIST[@]}"; do + KDUMP_KERNEL=${KDUMP_KERNEL_LIST[$KDUMP_KERNELVER]} + if ! setup_initrd; then + derror "Failed to setup initrd for $KDUMP_KERNEL" + _ret=1 + continue + fi + rebuild_initrd + _ret=$? + [[ $_ret -eq 0 ]] || break + done + return $_ret }
check_vmlinux()
Handy way to change KDUMP_STDLOGLVL without the need to edit the sysconfig.
While at it redirect the error message printed when dinit fails to stderr.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 07b4382..b4a5253 100755 --- a/kdumpctl +++ b/kdumpctl @@ -43,12 +43,14 @@ cmds_global[reset-crashkernel]=reset_crashkernel cmds_global[_pkg-upgrade-hook]=pkg_upgrade_hook
# shellcheck disable=SC2034 -opts_short_global="h" +opts_short_global="hqv" # shellcheck disable=SC2034 opts_long_global=( --long "help" --long "kernel:" --long "kver:" + --long "quiet" + --long "verbose" )
parse_args_global() @@ -66,6 +68,16 @@ parse_args_global() KDUMP_KERNELVER="$2" return 2 ;; + -q|--quiet) + KDUMP_STDLOGLVL=0 + return 1 + ;; + -v|--verbose) + if [[ $KDUMP_STDLOGLVL -le 4 ]]; then + (( KDUMP_STDLOGLVL += 1 )) + fi + return 1 + ;; esac }
@@ -116,12 +128,6 @@ fi # shellcheck source=./kdump-logger.sh . $KDUMP_LIB_PATH/kdump-logger.sh
-#initiate the kdump logger -if ! dlog_init; then - echo "failed to initiate the kdump logger." - exit 1 -fi - KDUMP_TMPDIR=$(mktemp -d kdump.XXXX) trap ' ret=$?; @@ -158,6 +164,8 @@ Options: --kver <kernel version> The kernel version to use. This option is only required if the version cannot be determined from the kernel image. + -q, --quiet Supress any output. Equivalent to KDUMP_STDLOGLVL=0. + -v, --verbose Increase the log level. Can be given multiple times.
Options for reset-crashkernel: --fadump [on|off|nocma] @@ -348,7 +356,7 @@ single_instance_lock() fi
if ! exec 9> $lockfile; then - derror "Create file lock failed" + echo "Create file lock failed" >&2 exit 1 fi
@@ -356,7 +364,7 @@ single_instance_lock() rc=$?
while [[ $rc -ne 0 ]]; do - dinfo "Another app is currently holding the kdump lock; waiting for it to exit..." + echo "Another app is currently holding the kdump lock; waiting for it to exit..." flock -w $timeout 9 rc=$? done @@ -2033,6 +2041,12 @@ main() parse_args global func args "$@" eval set -- "$args"
+ #initiate the kdump logger + if ! dlog_init; then + echo "failed to initiate the kdump logger." >&2 + exit 1 + fi + if [[ -z "$func" ]]; then if [[ -z "$1" ]]; then _usage "No command provided." @@ -2062,7 +2076,7 @@ if [[ ${__SOURCED__:+x} ]]; then fi
if [[ ! -f $KDUMP_CONFIG_FILE ]]; then - derror "Error: No kdump config file found!" + echo "Error: No kdump config file found!" >&2 exit 1 fi
'status' currently only translates the content of /sys/kernel/kexec_crash_loaded (or /sys/kernel/fadump_registered) to a string which is only mildly helpful. Thus extend the information printed with some more status information like the dump mode and size of the reserved crashkernel memory. Furthermore, add warnings for typical problems that occur regularly.
This patch makes 'showmem' obsolete and removes it.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 52 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 18 deletions(-)
diff --git a/kdumpctl b/kdumpctl index b4a5253..0ed0379 100755 --- a/kdumpctl +++ b/kdumpctl @@ -35,7 +35,6 @@ cmds_global[reload]=_reload cmds_global[restart]=_restart cmds_global[rebuild]=_rebuild cmds_global[propagate]=propagate_ssh_key -cmds_global[showmem]=show_reserved_mem cmds_global[estimate]=do_estimate cmds_global[get-default-crashkernel]=get_default_crashkernel cmds_global[reset-crashkernel]=reset_crashkernel @@ -1092,17 +1091,6 @@ propagate_ssh_key() fi }
-show_reserved_mem() -{ - local mem - local mem_mb - - mem=$(< /sys/kernel/kexec_crash_size) - mem_mb=$((mem / 1024 / 1024)) - - dinfo "Reserved ${mem_mb}MB memory for crash kernel" -} - save_raw() { local raw_target @@ -1322,19 +1310,47 @@ _start()
_status() { - EXIT_CODE=0 + local _ret _i + local _ck_loaded _ck_mem + + local -a _units=( "Bytes" "KB" "MB" "GB" ) + + _ret=0 + is_kernel_loaded "$DEFAULT_DUMP_MODE" case "$?" in 0) - dinfo "Kdump is operational" - EXIT_CODE=0 + _ck_loaded=true + _ret=0 ;; 1) - dinfo "Kdump is not operational" - EXIT_CODE=3 + _ck_loaded=false + _ret=3 ;; esac - exit $EXIT_CODE + + _ck_mem=$(< /sys/kernel/kexec_crash_size) + if [[ $_ck_mem -gt 0 ]]; then + while [[ $(( _ck_mem / 1024 )) -gt 0 ]]; do + (( _ck_mem /= 1024 )) + (( _i += 1 )) + done + _ck_mem+=" ${_units[_i]}" + else + _ck_mem="None" + fi + + cat << EOF | dinfo +Dump mode: $DEFAULT_DUMP_MODE +Crashkernel loaded: $_ck_loaded +Memory reserved: $_ck_mem +EOF + + parse_config || exit $_ret + if ! check_and_wait_network_ready &> /dev/null; then + dwarn "Warning: Remote host not reachable. Please verify your network connection and check if you have propagated your SSH key." + fi + exit $_ret }
_reload()
The function is pretty simple and only used in the upgrade hook. So move it in there.
While at it make sure that the message printed is consistent with the one in reset_crashkernel. Furthermore, only ask for reboot once and only when the currently running kernel was updated.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 62 ++++++++++++++++++++++++-------------------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 0ed0379..348dae1 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1714,7 +1714,7 @@ get_dump_mode_by_kernel()
_update_kernel_cmdline() { - local _kernel _param _old _new + local _kernel _param _old _new _has_changed
_kernel="$1" _param="$2" @@ -1727,22 +1727,29 @@ _update_kernel_cmdline() if is_ostree; then if [[ -z "$_new" ]]; then rpm-ostree kargs --delete="$_param=$_old" + _has_changed="Added $_param=$_new" elif [[ -n "$_old" ]]; then rpm-ostree kargs --replace="$_param=$_new" + _has_changed="Updated $_param=$_new" else rpm-ostree kargs --append="$_param=$_new" + _has_changed="Removed $_param=$_new" fi elif has_command grubby; then if [[ -n "$_new" ]]; then grubby --update-kernel "$_kernel" --args "$_param=$_new" + _has_changed="Updated $_param=$_new" else grubby --update-kernel "$_kernel" --remove-args "$_param" + _has_changed="Removed $_param=$_new" fi else derror "Unable to update kernel command line" exit 1 fi
+ dwarn "$_has_changed for kernel=$_kernel." + # Don't use "[[ CONDITION ]] && COMMAND" which returns 1 under false CONDITION if [[ -f /etc/zipl.conf ]]; then zipl > /dev/null @@ -1751,8 +1758,7 @@ _update_kernel_cmdline()
reset_crashkernel() { - local _kernel _dump_mode - local _has_changed _needs_reboot + local _kernel _needs_reboot _dump_mode local _old_ck _new_ck local _old_fadump _new_fadump local func args @@ -1777,7 +1783,6 @@ reset_crashkernel() fi
for _kernel in "${KDUMP_KERNEL_LIST[@]}"; do - _has_changed="" if [[ $(uname -m) == ppc64le ]]; then if [[ -n "${OPT[fadump]}" ]]; then _dump_mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") @@ -1786,11 +1791,7 @@ reset_crashkernel() _new_fadump="${OPT[fadump]}" [[ "$_new_fadump" == off ]] && _new_fadump="" if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then - if [[ -n "$_new_fadump" ]]; then - _has_changed="Updated fadump=$_new_fadump" - else - _has_changed="Removed fadump" - fi + [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi else _dump_mode="$(get_dump_mode_by_kernel "$_kernel")" @@ -1802,16 +1803,16 @@ reset_crashkernel()
_old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then - _has_changed="Updated crashkernel=$_new_ck" - fi - if [[ -n "$_has_changed" ]]; then - _needs_reboot=1 - dwarn "$_has_changed for kernel=$_kernel. Please reboot the system for the change to take effect." + [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi done
- if [[ ${OPT[reboot]} == yes && $_needs_reboot == 1 ]]; then - systemctl reboot + if [[ $_needs_reboot == 1 ]]; then + if [[ ${OPT[reboot]} == yes ]]; then + systemctl reboot + else + dwarn "Please reboot the system for the change to take effect." + fi fi }
@@ -1825,22 +1826,6 @@ _is_bootloader_installed() fi }
-_update_crashkernel() -{ - local _kernel _dump_mode _msg - local _old_ck _new_ck - - _kernel=$1 - _dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then - _msg="For kernel=$_kernel, crashkernel=$_new_ck now. Please reboot the system for the change to take effect." - _msg+=" Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." - dinfo "$_msg" - fi -} - # read the value of an environ variable from given environ file path # # The environment variable entries in /proc/[pid]/environ are separated @@ -1862,7 +1847,7 @@ _is_osbuild()
pkg_upgrade_hook() { - local _pkg + local _pkg _old_ck _new_ck _needs_reboot
if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then return @@ -1884,9 +1869,16 @@ pkg_upgrade_hook()
for KDUMP_KERNELVER in "${!KDUMP_KERNEL_LIST[@]}"; do KDUMP_KERNEL="${KDUMP_KERNEL_LIST["$KDUMP_KERNELVER"]}" - _update_crashkernel "$KDUMP_KERNEL" + _old_ck=$(get_grub_kernel_boot_parameter "$KDUMP_KERNEL" crashkernel) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$(get_dump_mode_by_kernel "$KDUMP_KERNEL")") + if _update_kernel_cmdline "$KDUMP_KERNEL" crashkernel "$_old_ck" "$_new_ck"; then + [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 + fi done - + if [[ $_needs_reboot == 1 ]]; then + dwarn "Please reboot the system for the change to take effect." + fi + dinfo "Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." }
_init_systeminfo()
The only reason why there is still the need to special handle CoreOS in reset_crashkernel is parsing the crashkernel= parameter from the command line. Thus rename get_grub_kernel_boot_parameter to _get_kernel_cmdline_parameter and make it work for CoreOS as well. With this the special handling can be removed.
With the new _get_kernel_cmdline_parameter there is now a runtime check whether grubby is installed before every call to grubby. So we can also remove the weak dependency on grubby in the spec file.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 69 ++++++++++++++++++++++++++---------------------- kexec-tools.spec | 1 - 2 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 348dae1..14f522d 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1667,17 +1667,6 @@ get_default_crashkernel() kdump_get_arch_recommend_crashkernel "$_dump_mode" }
-# Read kernel cmdline parameter for a specific kernel -# $1: kernel path, DEFAULT or kernel path, ALL not accepted -# $2: kernel cmldine parameter -get_grub_kernel_boot_parameter() -{ - local _kernel_path=$1 _para=$2 - - [[ $_kernel_path == ALL ]] && derror "kernel_path=ALL invalid for get_grub_kernel_boot_parameter" && return 1 - grubby --info="$_kernel_path" | sed -En -e "/^args=.*$/{s/^.*(\s|")${_para}=(\S*).*"$/\2/p;q}" -} - # get dump mode by fadump value # return # - fadump, if fadump=on or fadump=nocma @@ -1703,7 +1692,7 @@ get_dump_mode_by_kernel() { local _kernel_path=$1 _fadump_val _dump_mode
- _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel_path" fadump) + _fadump_val=$(_get_kernel_cmdline_parameter "$_kernel_path" fadump) if _dump_mode=$(get_dump_mode_by_fadump_val "$_fadump_val"); then echo -n "$_dump_mode" else @@ -1712,6 +1701,36 @@ get_dump_mode_by_kernel() fi }
+_get_kernel_cmdline_parameter() +{ + local _kernel _param _cmdline _ret + + _kernel="$1" + _param="$2" + + if is_ostree; then + _cmdline="$(rpm-ostree kargs)" + elif has_command grubby; then + _cmdline="$(grubby --info="$_kernel")" + _cmdline="${_cmdline##*args="}" + _cmdline="${_cmdline%%"*}" + else + derror "Unable to get kernel command line" + fi + + ddebug "command line for kernel $_kernel is '$_cmdline'" + + # make sure there is a space before/after the parameter + _cmdline=" $_cmdline " + if [[ $_cmdline == *" $_param="* ]]; then + _ret=${_cmdline##*"$_param"=} + _ret=${_ret%% *} + echo "$_ret" + elif [[ $_cmdline == *" $_param "* ]];then + echo "$_param" + fi +} + _update_kernel_cmdline() { local _kernel _param _old _new _has_changed @@ -1767,27 +1786,12 @@ reset_crashkernel() _usage "Additional parameter '$1' provided." fi
- # 1. OSTree systems use "rpm-ostree kargs" instead of grubby to manage kernel command - # line. --kernel=ALL doesn't make sense for OStree. - # 2. We don't have any OSTree POWER systems so the dump mode is always kdump. - # 3. "rpm-ostree kargs" would prompt the user to reboot the system after - # modifying the kernel command line so there is no need for kexec-tools - # to repeat it. - if is_ostree; then - _old_ck=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') - _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) - if _update_kernel_cmdline "" crashkernel "$_old_ck" "$_new_ck"; then - [[ ${OPT[reboot]} == yes ]] && systemctl reboot - fi - return - fi - for _kernel in "${KDUMP_KERNEL_LIST[@]}"; do if [[ $(uname -m) == ppc64le ]]; then if [[ -n "${OPT[fadump]}" ]]; then _dump_mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + _old_fadump=$(_get_kernel_cmdline_parameter "$_kernel" fadump) _new_fadump="${OPT[fadump]}" [[ "$_new_fadump" == off ]] && _new_fadump="" if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then @@ -1801,7 +1805,7 @@ reset_crashkernel() _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) fi
- _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _old_ck=$(_get_kernel_cmdline_parameter "$_kernel" crashkernel) if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi @@ -1810,7 +1814,10 @@ reset_crashkernel() if [[ $_needs_reboot == 1 ]]; then if [[ ${OPT[reboot]} == yes ]]; then systemctl reboot - else + + # "rpm-ostree kargs" already asked the user to reboot after + # modifying the kernel command line. No need to repeat it. + elif ! is_ostree; then dwarn "Please reboot the system for the change to take effect." fi fi @@ -1869,7 +1876,7 @@ pkg_upgrade_hook()
for KDUMP_KERNELVER in "${!KDUMP_KERNEL_LIST[@]}"; do KDUMP_KERNEL="${KDUMP_KERNEL_LIST["$KDUMP_KERNELVER"]}" - _old_ck=$(get_grub_kernel_boot_parameter "$KDUMP_KERNEL" crashkernel) + _old_ck=$(_get_kernel_cmdline_parameter "$KDUMP_KERNEL" crashkernel) _new_ck=$(kdump_get_arch_recommend_crashkernel "$(get_dump_mode_by_kernel "$KDUMP_KERNEL")") if _update_kernel_cmdline "$KDUMP_KERNEL" crashkernel "$_old_ck" "$_new_ck"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 diff --git a/kexec-tools.spec b/kexec-tools.spec index 1375089..a8b0b9b 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -68,7 +68,6 @@ Requires: dracut-squash >= 058 Requires: ethtool Requires: util-linux Requires: binutils -Recommends: grubby Recommends: hostname BuildRequires: make BuildRequires: zlib-devel elfutils-devel glib2-devel bzip2-devel ncurses-devel bison flex lzo-devel snappy-devel libzstd-devel
All callers of _update_kernel_cmdline determine the "old" value right before the call via _get_kernel_cmdline_parameter. All the information needed to do this call are available in _update_kernel_cmdline. So move the call there to reduce some redundant code.
While at it drop the "new" from the variable names in the callers. Just to make the name a tad shorter.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 14f522d..26cc524 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1737,8 +1737,9 @@ _update_kernel_cmdline()
_kernel="$1" _param="$2" - _old="$3" - _new="$4" + _new="$3" + + _old="$(_get_kernel_cmdline_parameter "$_kernel" "$_param")"
# _new = "" removes a _param, so _new = _old = "" is fine [[ -n "$_new" ]] && [[ "$_old" == "$_new" ]] && return 1 @@ -1777,10 +1778,8 @@ _update_kernel_cmdline()
reset_crashkernel() { - local _kernel _needs_reboot _dump_mode - local _old_ck _new_ck - local _old_fadump _new_fadump - local func args + local _opts _kernel _needs_reboot + local _mode _ck _fadump
if [[ -n "$1" ]]; then _usage "Additional parameter '$1' provided." @@ -1789,24 +1788,21 @@ reset_crashkernel() for _kernel in "${KDUMP_KERNEL_LIST[@]}"; do if [[ $(uname -m) == ppc64le ]]; then if [[ -n "${OPT[fadump]}" ]]; then - _dump_mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - _old_fadump=$(_get_kernel_cmdline_parameter "$_kernel" fadump) - _new_fadump="${OPT[fadump]}" - [[ "$_new_fadump" == off ]] && _new_fadump="" - if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then + _mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") + _fadump="${OPT[fadump]}" + [[ "$_fadump" == off ]] && _fadump="" + if _update_kernel_cmdline "$_kernel" fadump "$_fadump"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi else - _dump_mode="$(get_dump_mode_by_kernel "$_kernel")" - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") + _mode="$(get_dump_mode_by_kernel "$_kernel")" fi else - _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) + _mode=kdump fi
- _old_ck=$(_get_kernel_cmdline_parameter "$_kernel" crashkernel) - if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then + _ck=$(kdump_get_arch_recommend_crashkernel "$_mode") + if _update_kernel_cmdline "$_kernel" crashkernel "$_ck"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi done @@ -1854,7 +1850,7 @@ _is_osbuild()
pkg_upgrade_hook() { - local _pkg _old_ck _new_ck _needs_reboot + local _pkg _ck _needs_reboot
if [[ $(kdump_get_conf_val auto_reset_crashkernel) != yes ]]; then return @@ -1876,9 +1872,8 @@ pkg_upgrade_hook()
for KDUMP_KERNELVER in "${!KDUMP_KERNEL_LIST[@]}"; do KDUMP_KERNEL="${KDUMP_KERNEL_LIST["$KDUMP_KERNELVER"]}" - _old_ck=$(_get_kernel_cmdline_parameter "$KDUMP_KERNEL" crashkernel) - _new_ck=$(kdump_get_arch_recommend_crashkernel "$(get_dump_mode_by_kernel "$KDUMP_KERNEL")") - if _update_kernel_cmdline "$KDUMP_KERNEL" crashkernel "$_old_ck" "$_new_ck"; then + _ck=$(kdump_get_arch_recommend_crashkernel "$(get_dump_mode_by_kernel "$KDUMP_KERNEL")") + if _update_kernel_cmdline "$KDUMP_KERNEL" crashkernel "$_ck"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi done
Currently there are two subcommands to manage the crashkernel parameter, reset-crashkernel and get-default-crashkernel. Merge the two together into a single crashkernel subcommand.
The synopsis is the following:
kdumpctl crashkernel -> show the currently set value kdumpctl crashkernel --default -> show the default value kdumpctl crashkernel --set <value> -> set the value to <value> kdumpctl crashkernel --set --default -> set the value to the default
Of course these options can be combined with the already existing --kernel and --fadump options.
Benefit of having this new subcommand is that it is much simpler and (imho) more intuitive compared to the old subcommands. Furthermore it is much more flexible. For example with the new implementations it is much easier to introduce, e.g. a --add option to increase crashkernel value by a certain size or a --remove option to remove it all together.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 88 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 24 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 26cc524..955558c 100755 --- a/kdumpctl +++ b/kdumpctl @@ -36,8 +36,7 @@ cmds_global[restart]=_restart cmds_global[rebuild]=_rebuild cmds_global[propagate]=propagate_ssh_key cmds_global[estimate]=do_estimate -cmds_global[get-default-crashkernel]=get_default_crashkernel -cmds_global[reset-crashkernel]=reset_crashkernel +cmds_global[crashkernel]=_crashkernel # shellcheck disable=SC2034 cmds_global[_pkg-upgrade-hook]=pkg_upgrade_hook
@@ -81,16 +80,22 @@ parse_args_global() }
# shellcheck disable=SC2034 -opts_short_reset_crashkernel="" +opts_short_crashkernel="" # shellcheck disable=SC2034 -opts_long_reset_crashkernel=( +opts_long_crashkernel=( + --long "default" --long "fadump:" --long "reboot" + --long "set" )
-parse_args_reset_crashkernel() +parse_args_crashkernel() { case $1 in + --default) + OPT[default]=yes + return 1 + ;; --fadump) if [[ $(uname -a) != ppc64le ]]; then derror "Option --fadump only valid on PPC" @@ -103,6 +108,10 @@ parse_args_reset_crashkernel() OPT[reboot]=yes return 1 ;; + --set) + OPT[set]=yes + return 1 + ;; esac }
@@ -141,8 +150,22 @@ has_function()
_usage() { + __usage "$PROGNAME COMMAND [OPTIONS]" "$@" +} + +_usage_crashkernel() +{ + __usage "$PROGNAME crashkernel [OPTIONS] [<value>]" "$@" +} + +__usage() +{ + local _cmd + + _cmd="$1"; shift + [[ -n "$*" ]] && dinfo "$*" - dinfo "Usage: $PROGNAME [OPTIONS] COMMAND" + dinfo "Usage: $_cmd" dinfo "See '$PROGNAME --help' or 'man $PROGNAME' for more details"
exit 1 @@ -166,10 +189,12 @@ Options: -q, --quiet Supress any output. Equivalent to KDUMP_STDLOGLVL=0. -v, --verbose Increase the log level. Can be given multiple times.
-Options for reset-crashkernel: +Options for crashkernel: + --default Use the recommended default value --fadump [on|off|nocma] The mode fadump should run in (only valid for PPC). --reboot Automatically reboot if the kernel command line changed. + --set Set the parameter instead of just showing showing the value
Common commands: status Print current status of the service @@ -178,8 +203,7 @@ Common commands: restart Equivalent to stop; start rebuild Force rebuild the kdump initramfs propagate Setup key authentication for ssh dump targets - reset-crashkernel - Reset the crashkernel parameter to its default value + crashkernel Manage the crashkernel parameter on the kernel command line
See 'man $PROGNAME' for more details and the full list of commands. EOF @@ -1660,13 +1684,6 @@ do_estimate() fi }
-get_default_crashkernel() -{ - local _dump_mode=$1 - - kdump_get_arch_recommend_crashkernel "$_dump_mode" -} - # get dump mode by fadump value # return # - fadump, if fadump=on or fadump=nocma @@ -1776,10 +1793,16 @@ _update_kernel_cmdline() fi }
-reset_crashkernel() +_crashkernel() { local _opts _kernel _needs_reboot - local _mode _ck _fadump + local _mode _ck _opt_ck _fadump + + _opt_ck="$1" + + if [[ "${OPT[set]}" == yes ]] && [[ "${OPT[default]}" != yes ]] && [[ -z "$_opt_ck" ]];then + _usage_crashkernel "No value to set the crashkernel parameter to provided" + fi
if [[ -n "$1" ]]; then _usage "Additional parameter '$1' provided." @@ -1789,11 +1812,6 @@ reset_crashkernel() if [[ $(uname -m) == ppc64le ]]; then if [[ -n "${OPT[fadump]}" ]]; then _mode=$(get_dump_mode_by_fadump_val "${OPT[fadump]}") - _fadump="${OPT[fadump]}" - [[ "$_fadump" == off ]] && _fadump="" - if _update_kernel_cmdline "$_kernel" fadump "$_fadump"; then - [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 - fi else _mode="$(get_dump_mode_by_kernel "$_kernel")" fi @@ -1801,10 +1819,32 @@ reset_crashkernel() _mode=kdump fi
- _ck=$(kdump_get_arch_recommend_crashkernel "$_mode") + if [[ "${OPT[set]}" != yes ]]; then + if [[ "${OPT[default]}" == yes ]]; then + kdump_get_arch_recommend_crashkernel "$_mode" + else + _get_kernel_cmdline_parameter "$_kernel" crashkernel + fi + continue + fi + + if [[ -n "$_opt_ck" ]]; then + _ck="$_opt_ck" + else + _ck=$(kdump_get_arch_recommend_crashkernel "$_mode") + fi if _update_kernel_cmdline "$_kernel" crashkernel "$_ck"; then [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 fi + + + if [[ -n "${OPT[fadump]}" ]]; then + _fadump="${OPT[fadump]}" + [[ "$_fadump" == off ]] && _fadump="" + if _update_kernel_cmdline "$_kernel" fadump "$_fadump"; then + [[ $(uname -r) == "$KDUMP_KERNELVER" ]] && _needs_reboot=1 + fi + fi done
if [[ $_needs_reboot == 1 ]]; then
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
diff --git a/kdumpctl b/kdumpctl index 955558c..b76f8b3 100755 --- a/kdumpctl +++ b/kdumpctl @@ -79,6 +79,11 @@ parse_args_global() esac }
+declare -A cmds_crashkernel +cmds_crashkernel[test]=_crashkernel_test +# shellcheck disable=SC2034 +cmds_crashkernel[set]=_crashkernel_set + # shellcheck disable=SC2034 opts_short_crashkernel="" # shellcheck disable=SC2034 @@ -115,6 +120,23 @@ parse_args_crashkernel() esac }
+# shellcheck disable=SC2034 +opts_short_crashkernel_set="f:" +# shellcheck disable=SC2034 +opts_long_crashkernel_set=( + --long "foo:" +) + +parse_args_crashkernel_set() +{ + case $1 in + -f|--foo) + OPT[foo]="$2" + return 2 + ;; + esac +} + # Only do the bare minimum when sourced during bash completion [[ ${COMP_LINE:+x} ]] && return
@@ -1859,6 +1881,23 @@ _crashkernel() fi }
+_crashkernel_test() +{ + echo "Hello, I'm test function _crashkernel_test." + echo "You know me better as 'kdumpctl crashkernel test'." + echo "Nice to meet you" +} + +_crashkernel_set() +{ + echo "Hello, I'm test function _crashkernel_set." + echo "You know me better as 'kdumpctl crashkernel set'." + echo "Nice to meet you" + if [[ -n "${OPT[foo]}" ]]; then + echo "Thanks for ${OPT[foo]}" + fi +} + GRUB_CFG=/boot/grub2/grub.cfg _is_bootloader_installed() {
FYI, to make review a little easier I also pushed the series to my fork https://src.fedoraproject.org/fork/prudo/rpms/kexec-tools/tree/cli_rework
Thanks Philipp
On Sat, 17 Jun 2023 11:27:52 +0200 Philipp Rudo prudo@redhat.com wrote:
Hi everybody,
The series far from being complete. But it finally passes the first simple tests so I think it's time to get the first feedback.
The series started out with the request to be able to rebuild the initramfs for all installed kernels. As 'reset-crashkernel' already had a --kernel=ALL option I didn't want to introduce a second way to do the same thing. My problem was the way --kernel=ALL was implemented, especially its hard dependency on grubby which might not be installed on all systems. This lead to basically a rewrite of the crashkernel handling with now (hopefully) simpler, better to understand code.
An other problem I had with the former implementation of --kernel=ALL was that the command line parser was self-written. This made the UI much more rigid than it had to be. So I changed it to use getopt instead. In order to prevent unnecessary duplicate code I also wrote a framework around it (which might be a little bit over engineered for what we need but once I started I refused to give up plus it gives bash-completion almost for free, which is nice ;-)). While at it I also added some basic options most users expect, like --help.
Finally I've merged multiple commands to make the UI (hopefully) a little bit cleaner and more intuitive. For example I've never understood why 'status' only retuned a simple yes/no but not other important information about the current state like 'git status' does.
As stated in the beginning there is still a lot to do. In particular:
- Test cases still need to be adjusted (and new ones added).
- The documentation needs to be reviewed and adjusted.
- The patches don't consider Pingfans 64k work, yet. So they will need some adjustments (they are currently based on 29fe563 ("kdump.conf: redirect unknown architecture warning to stderr"))
- I've noticed a bug in the bash completion where subsubcommands aren't completed correctly.
- And of course the series need a lot more testing. For the moment I've only made some very simple sanity checks. So I would be surprised if the code isn't still riddled with bugs...
Anyway, this is quite a big change and opinions on what a good UI is are very subjective. That's why I want to get feedback early before I waste too much time on something which isn't wanted.
Happy to hear your opinion (good and bad).
Thanks Philipp
Philipp Rudo (26): kdumpctl: Fix various shellcheck findings kdump-lib: make is_zstd_command_available more generic kdump-lib: simplify _get_kdump_kernel_version kdumpctl: drop _get_current_running_kernel_path kdumpctl: drop condrestart subcommand kdumpctl: make all subcommands execute exactly one function kdumpctl: Restructure subcommand handling and and usage kdumpctl: simplify _update_kernel_cmdline kdumpctl: Prevent option --fadump on non-PPC in reset_crashkernel kdumpctl: Stop updating grub config in reset_crashkernel kdumpctl: Simplify fadump handling in reset_crashkernel kdumpctl: Use getopt for option parsing in reset_crashkernel kdumpctl: Add simple bash-completion for kdumpctl kdumpctl: Make --kernel a global option kdumpctl: Add global -h/--help option kdumpctl: Add --kver option kdumpctl: Reduce dependencies from grubby for --kernel kdumpctl: merge reset_crashkernel_{after_update,for_installed_kernel} kdumpctl: Allow --kernel with 'start' and 'rebuild' kdumpctl: Add --quiet/--verbose option kdumpctl: Rework the 'status' subcommand kdumpctl: Merge _upgrade_crashkernel into the upgrade hook kdumpctl: Drop special handling for CoreOS in reset_crashkernel kdumpctl: Determine "old" value within _update_kernel_cmdline kdumpctl: Introduce new crashkernel subcommand REMOVEME: add test subsubcommands
92-crashkernel.install | 2 +- kdump-lib.sh | 27 +- kdumpctl | 1094 ++++++++++++++++++++++++-------------- kdumpctl.bash-completion | 125 +++++ kexec-tools.spec | 16 +- mkdumprd | 2 +- mkfadumprd | 2 +- 7 files changed, 865 insertions(+), 403 deletions(-) create mode 100755 kdumpctl.bash-completion
-
Philipp Rudo -
Pingfan Liu