With the deprecation of the 'default' option in kdump.conf check_failure_action_config needed to track which option was used (default or failure_action). This made the function quite complex.Thus make option 'default' a true alias of 'failure_action' when parsing kdump.conf and simplify check_failure_action_config.

Do the same simplifications for check_final_action_config as both functions are basically identical.

Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 55 +++++++++++++++++-------------------------------------- 1 file changed, 17 insertions(+), 38 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 3cad3dd..8a06c2e 100755 --- a/kdumpctl +++ b/kdumpctl @@ -255,7 +255,12 @@ parse_config() config_val=$DEFAULT_SSHKEY fi ;; - path | core_collector | kdump_post | kdump_pre | extra_bins | extra_modules | failure_action | default | final_action | force_rebuild | force_no_rebuild | fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;; + default) + dwarn "WARNING: Option 'default' was renamed 'failure_action' and will be removed in the future." + dwarn "Please update $KDUMP_CONFIG_FILE to use option 'failure_action' instead." + _set_config failure_action "$config_val" || return 1 + ;; + path | core_collector | kdump_post | kdump_pre | extra_bins | extra_modules | failure_action | final_action | force_rebuild | force_no_rebuild | fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;;

net | options | link_delay | disk_timeout | debug_mem_level | blacklist) derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives." @@ -990,31 +995,12 @@ start_dump()

check_failure_action_config() { - local default_option - local failure_action - local option="failure_action" - - default_option=${OPT[default]} - failure_action=${OPT[failure_action]} - - if [[ -z $failure_action ]] && [[ -z $default_option ]]; then - return 0 - elif [[ -n $failure_action ]] && [[ -n $default_option ]]; then - derror "Cannot specify 'failure_action' and 'default' option together" - return 1 - fi - - if [[ -n $default_option ]]; then - option="default" - failure_action="$default_option" - fi - - case "$failure_action" in - reboot | halt | poweroff | shell | dump_to_rootfs) + case "${OPT[failure_action]}" in + "" | reboot | halt | poweroff | shell | dump_to_rootfs) return 0 ;; *) - dinfo $"Usage kdump.conf: $option {reboot|halt|poweroff|shell|dump_to_rootfs}" + dinfo $"Usage kdump.conf: failure_action {reboot|halt|poweroff|shell|dump_to_rootfs}" return 1 ;; esac @@ -1022,22 +1008,15 @@ check_failure_action_config()

check_final_action_config() { - local final_action - - final_action=${OPT[final_action]} - if [[ -z $final_action ]]; then + case "${OPT[final_action]}" in + "" | reboot | halt | poweroff) return 0 - else - case "$final_action" in - reboot | halt | poweroff) - return 0 - ;; - *) - dinfo $"Usage kdump.conf: final_action {reboot|halt|poweroff}" - return 1 - ;; - esac - fi + ;; + *) + dinfo $"Usage kdump.conf: final_action {reboot|halt|poweroff}" + return 1 + ;; + esac }

start()

Currently dracut-early-kdump.sh claims to be POSIX compliant but it sources kdump-lib.sh which uses bash-only syntax. Thus require bash for dracut-early-kdump.sh as well.

Signed-off-by: Philipp Rudo prudo@redhat.com --- .editorconfig | 2 +- dracut-early-kdump.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/.editorconfig b/.editorconfig index 87c4f98..b343f27 100644 --- a/.editorconfig +++ b/.editorconfig @@ -22,7 +22,7 @@ space_redirects = true shell_variant = bash

# Use dracut code style for *-module-setup.sh -[*-module-setup.sh] +[*-module-setup.sh,dracut-early-kdump.sh] shell_variant = bash indent_style = space indent_size = 4 diff --git a/dracut-early-kdump.sh b/dracut-early-kdump.sh index 9e55258..2954915 100755 --- a/dracut-early-kdump.sh +++ b/dracut-early-kdump.sh @@ -1,4 +1,4 @@ -#! /bin/sh +#! /bin/bash

KEXEC=/sbin/kexec standard_kexec_args="-p"

mkfadumprd doesn't call prepare_kdump_bootinfo from kdump-lib.sh. Thus both KDUMP_KERNELVER and DEFAULT_INITRD are always empty. Simply remove them from the debug print.

Signed-off-by: Philipp Rudo prudo@redhat.com --- mkfadumprd | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) mode change 100644 => 100755 mkfadumprd

diff --git a/mkfadumprd b/mkfadumprd old mode 100644 new mode 100755 index 2fc396c..719e150 --- a/mkfadumprd +++ b/mkfadumprd @@ -37,7 +37,7 @@ FADUMP_INITRD="$MKFADUMPRD_TMPDIR/fadump.img" ### First build an initramfs with dump capture capability # this file tells the initrd is fadump enabled touch "$MKFADUMPRD_TMPDIR/fadump.initramfs" -ddebug "rebuild fadump initrd: $FADUMP_INITRD $DEFAULT_INITRD $KDUMP_KERNELVER" +ddebug "rebuild fadump initrd: $FADUMP_INITRD" # Don't use squash for capture image or default image as it negatively impacts # compression ratio and increases the size of the initramfs image. # Don't compress the capture image as uncompressed image is needed immediately.

prepare_cmdline is totally broken. For example if the remove list ($2) is empty it removes all white spaces or if a parameter has a quoted value containing a white space it only removes the first part of the parameter up to the first space. Thus add a test case that shows what the function should do in order to fix it in subsequent patches.

Signed-off-by: Philipp Rudo prudo@redhat.com --- spec/kdump-lib_spec.sh | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)

diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 8c91480..3d10006 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -48,4 +48,36 @@ Describe 'kdump-lib' End End

+ Describe 'prepare_cmdline()' + get_bootcpu_apicid() { + echo 1 + } + + get_watchdog_drvs() { + echo foo + } + + add="disable_cpu_apicid=1 foo.pretimeout=0" + + Parameters + #test cmdline remove add result + "#1" "a b c" "" "" "a b c" + "#2" "a b c" "b" "" "a c" + "#3" "a b=x c" "b" "" "a c" + "#4" "a b='x y' c" "b" "" "a c" + "#5" "a b='x y' c" "b=x" "" "a c" + "#6" "a b='x y' c" "b='x y'" "" "a c" + "#7" "a b c" "" "x" "a b c x" + "#8" "a b c" "" "x=1" "a b c x=1" + "#9" "a b c" "" "x='1 2'" "a b c x='1 2'" + "#10" "a b c" "a" "x='1 2'" "b c x='1 2'" + "#11" "a b c" "x" "x='1 2'" "a b c x='1 2'" + End + + It "Test $1: should generate the correct kernel command line" + When call prepare_cmdline "$2" "$3" "$4" + The output should equal "$5 $add" + End + End + End

A recently added unit test found that prepare_cmdline has several problems. For example an empty remove list will remove all spaces or when the cmdline contains a parameter with quoted values containing spaces will only remove the beginning up to the first space. Furthermore the old design requires lots of subshells and pipes.

This patch rewrites prepare_cmdline in a way that makes the unit test happy and tries to use as many bash built-ins as possible.

Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 128 ++++++++++++++++++++++++++------------------------- 1 file changed, 66 insertions(+), 62 deletions(-)

diff --git a/kdump-lib.sh b/kdump-lib.sh index bea74b2..6b0a83d 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -526,24 +526,6 @@ is_kernel_loaded() [[ $(< $_sysfs) -eq 1 ]] }

-# remove_cmdline_param <kernel cmdline> <param1> [<param2>] ... [<paramN>] -# Remove a list of kernel parameters from a given kernel cmdline and print the result. -# For each "arg" in the removing params list, "arg" and "arg=xxx" will be removed if exists. -remove_cmdline_param() -{ - local cmdline=$1 - shift - - for arg in "$@"; do - cmdline=$(echo "$cmdline" | - sed -e "s/\b$arg=[^ ]*//g" \ - -e "s/^$arg\b//g" \ - -e "s/[[:space:]]$arg\b//g" \ - -e "s/\s+/ /g") - done - echo "$cmdline" -} - # # This function returns the "apicid" of the boot # cpu (cpu 0) if present. @@ -558,23 +540,6 @@ get_bootcpu_apicid() /proc/cpuinfo }

-# -# append_cmdline <kernel cmdline> <parameter name> <parameter value> -# This function appends argument "$2=$3" to string ($1) if not already present. -# -append_cmdline() -{ - local cmdline=$1 - local newstr=${cmdline/$2/""} - - # unchanged str implies argument wasn't there - if [[ $cmdline == "$newstr" ]]; then - cmdline="${cmdline} ${2}=${3}" - fi - - echo "$cmdline" -} - # This function check iomem and determines if we have more than # 4GB of ram available. Returns 1 if we do, 0 if we dont need_64bit_headers() @@ -792,26 +757,46 @@ get_watchdog_drvs() echo "$_wdtdrvs" }

+_cmdline_parse() +{ + local opt val + + while read -r opt; do + if [[ $opt =~ = ]]; then + val=${opt#*=} + opt=${opt%%=*} + # ignore options like 'foo=' + [[ -z $val ]] && continue + # xargs removes quotes, add them again + [[ $val =~ [[:space:]] ]] && val=""$val"" + else + val="" + fi + + echo "$opt $val" + done <<< "$(echo "$1" | xargs -n 1 echo)" +} + # # prepare_cmdline <commandline> <commandline remove> <commandline append> # This function performs a series of edits on the command line. # Store the final result in global $KDUMP_COMMANDLINE. prepare_cmdline() { - local cmdline id arg + local in out append opt val id drv + local -A remove + + in=${1:-$(< /proc/cmdline)} + while read -r opt val; do + [[ -n "$opt" ]] || continue + remove[$opt]=1 + done <<< "$(_cmdline_parse "$2")" + append=$3

- if [[ -z $1 ]]; then - cmdline=$(< /proc/cmdline) - else - cmdline="$1" - fi

# These params should always be removed - cmdline=$(remove_cmdline_param "$cmdline" crashkernel panic_on_warn) - # These params can be removed configurably - while read -r arg; do - cmdline=$(remove_cmdline_param "$cmdline" "$arg") - done <<< "$(echo "$2" | xargs -n 1 echo)" + remove[crashkernel]=1 + remove[panic_on_warn]=1

# Always remove "root=X", as we now explicitly generate all kinds # of dump target mount information including root fs. @@ -819,43 +804,62 @@ prepare_cmdline() # We do this before KDUMP_COMMANDLINE_APPEND, if one really cares # about it(e.g. for debug purpose), then can pass "root=X" using # KDUMP_COMMANDLINE_APPEND. - cmdline=$(remove_cmdline_param "$cmdline" root) + remove[root]=1

# With the help of "--hostonly-cmdline", we can avoid some interitage. - cmdline=$(remove_cmdline_param "$cmdline" rd.lvm.lv rd.luks.uuid rd.dm.uuid rd.md.uuid fcoe) + remove[rd.lvm.lv]=1 + remove[rd.luks.uuid]=1 + remove[rd.dm.uuid]=1 + remove[rd.md.uuid]=1 + remove[fcoe]=1

# Remove netroot, rd.iscsi.initiator and iscsi_initiator since # we get duplicate entries for the same in case iscsi code adds # it as well. - cmdline=$(remove_cmdline_param "$cmdline" netroot rd.iscsi.initiator iscsi_initiator) + remove[netroot]=1 + remove[rd.iscsi.initiator]=1 + remove[iscsi_initiator]=1 + + while read -r opt val; do + [[ -n "$opt" ]] || continue + [[ -n "${remove[$opt]}" ]] && continue + + if [[ -n "$val" ]]; then + out+="$opt=$val " + else + out+="$opt " + fi + done <<< "$(_cmdline_parse "$in")"

- cmdline="${cmdline} $3" + out+="$append "

id=$(get_bootcpu_apicid) - if [[ -n ${id} ]]; then - cmdline=$(append_cmdline "$cmdline" disable_cpu_apicid "$id") + if [[ -n "${id}" ]]; then + out+="disable_cpu_apicid=$id " fi

# If any watchdog is used, set it's pretimeout to 0. pretimeout let # watchdog panic the kernel first, and reset the system after the # panic. If the system is already in kdump, panic is not helpful # and only increase the chance of watchdog failure. - for i in $(get_watchdog_drvs); do - cmdline+=" $i.pretimeout=0" - - if [[ $i == hpwdt ]]; then - # hpwdt have a special parameter kdumptimeout, is's only suppose - # to be set to non-zero in first kernel. In kdump, non-zero - # value could prevent the watchdog from resetting the system. - cmdline+=" $i.kdumptimeout=0" + for drv in $(get_watchdog_drvs); do + out+="$drv.pretimeout=0 " + + if [[ $drv == hpwdt ]]; then + # hpwdt have a special parameter kdumptimeout, it is + # only supposed to be set to non-zero in first kernel. + # In kdump, non-zero value could prevent the watchdog + # from resetting the system. + out+="$drv.kdumptimeout=0 " fi done

# This is a workaround on AWS platform. Always remove irqpoll since it # may cause the hot-remove of some pci hotplug device. - is_aws_aarch64 && cmdline=$(remove_cmdline_param "${cmdline}" irqpoll) + is_aws_aarch64 && out=$(echo "$out" | sed -e "/<irqpoll>//")

- echo "$cmdline" + # Trim unnecessary whitespaces + echo "$out" | sed -e "s/^ *//g" -e "s/ *$//g" -e "s/ +/ /g" }

PROC_IOMEM=/proc/iomem

Like for 'start' move the printing of the error message to the calling function. This not only makes the code more consistent to 'start' but also prevents 'kdumpctl restart' to call 'start' in case 'stop' has failed. This doesn't impact the case when 'kdumpctl restart' is run without any crash kernel being loaded as kexec will still return success in that case.

Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 77ed584..3d3b9d4 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1090,15 +1090,9 @@ reload_fadump() stop() { if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then - stop_fadump + stop_fadump || return else - stop_kdump - fi - - # shellcheck disable=SC2181 - if [[ $? != 0 ]]; then - derror "Stopping kdump: [FAILED]" - return 1 + stop_kdump || return fi

dinfo "Stopping kdump: [OK]" @@ -1684,7 +1678,10 @@ main() fi ;; stop) - stop + if ! stop; then + derror "Stopping kdump: [FAILED]" + exit 1 + fi ;; status) EXIT_CODE=0 @@ -1705,7 +1702,10 @@ main() reload ;; restart) - stop + if ! stop; then + derror "Stopping kdump: [FAILED]" + exit 1 + fi if ! start; then derror "Starting kdump: [FAILED]" exit 1

At the beginning of do_estimate it currently checks whether the TARGET_INITRD exists and if not fails with an error message. This not only requires the user to manually trigger the build of the initrd but also ignores all cases where the TARGET_INITRD exists but need to be rebuild. For example when there were changes to kdump.conf or when the system switches from kdump to fadump. All these changes will impact the outcome of do_estimate. Thus properly check whether the initrd needs to be rebuild and if it does trigger the rebuild automatically.

To do so move the check whether the TARGET_INITRD has fadump enabled to is_system_modified and call this function. With this force_(no_)rebuild options in kdump.conf are ignored to avoid unnecessary rebuilds.

While at it cleanup check_system_modified and rename it to is_system_modified. Furthermore move printing the info that the initrd gets rebuild to rebuild_initrd to avoid every caller has the same line.

Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 62 ++++++++++++++++++++++++-------------------------------- 1 file changed, 27 insertions(+), 35 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 5954527..d5bf408 100755 --- a/kdumpctl +++ b/kdumpctl @@ -117,6 +117,8 @@ rebuild_kdump_initrd()

rebuild_initrd() { + dinfo "Rebuilding $TARGET_INITRD" + if [[ ! -w $(dirname "$TARGET_INITRD") ]]; then derror "$(dirname "$TARGET_INITRD") does not have write permission. Cannot rebuild $TARGET_INITRD" return 1 @@ -532,32 +534,27 @@ check_fs_modified()

# returns 0 if system is not modified # returns 1 if system is modified -# returns 2 if system modification is invalid -check_system_modified() +# returns 2 if an error occurred +is_system_modified() { local ret

[[ -f $TARGET_INITRD ]] || return 1

- check_files_modified - ret=$? - if [[ $ret -ne 0 ]]; then - return $ret + # in case of fadump mode, check whether the default/target initrd is + # already built with dump capture capability + if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then + if ! lsinitrd -f $DRACUT_MODULES_FILE "$TARGET_INITRD" | grep -q -e ^kdumpbase$ -e ^zz-fadumpinit$; then + dinfo "Rebuild $TARGET_INITRD with dump capture support" + return 1 + fi fi

- check_fs_modified - ret=$? - if [[ $ret -ne 0 ]]; then - return $ret - fi + image_time=$(stat -c "%Y" "$TARGET_INITRD" 2> /dev/null)

+ check_files_modified || return + check_fs_modified || return check_drivers_modified - ret=$? - if [[ $ret -ne 0 ]]; then - return $ret - fi - - return 0 }

# need_initrd_rebuild - check whether the initrd needs to be rebuild @@ -600,18 +597,7 @@ need_initrd_rebuild() return 1 fi

- # in case of fadump mode, check whether the default/target initrd is - # already built with dump capture capability - if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then - if ! lsinitrd -f $DRACUT_MODULES_FILE "$TARGET_INITRD" | grep -q -e ^kdumpbase$ -e ^zz-fadumpinit$; then - dinfo "Rebuild $TARGET_INITRD with dump capture support" - return 1 - fi - fi - - image_time=$(stat -c "%Y" "$TARGET_INITRD" 2> /dev/null) - check_system_modified - + is_system_modified }

# On ppc64le LPARs, the keys trusted by firmware do not end up in @@ -994,7 +980,6 @@ start() # Nothing to do ;; 1) - dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd || return ;; *) @@ -1105,7 +1090,6 @@ rebuild()

setup_initrd || return 1

- dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd }

@@ -1118,10 +1102,18 @@ do_estimate() local size_mb=$((1024 * 1024))

setup_initrd - if [[ ! -f $TARGET_INITRD ]]; then - derror "kdumpctl estimate: kdump initramfs is not built yet." - exit 1 - fi + is_system_modified + case "$?" in + 0) + # Nothing to do + ;; + 1) + rebuild_initrd || return + ;; + *) + return + ;; + esac

kdump_mods="$(lsinitrd "$TARGET_INITRD" -f /usr/lib/dracut/hostonly-kernel-modules.txt | tr '\n' ' ')" baseline=$(kdump_get_arch_recommend_size)