Hi,
while looking into transforming mkdumprd and mkfadumprd into library functions I noticed various nits in kdumpctl. This series addresses them.
The series is made up of two parts:
Patches 1-8 are small independent cleanups and fixes.
Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It achieves this by only parsing kdump.conf once in check_config and storing the parsed values in an array. Later accesses can then simply use the value stored in the array instead of calling kdump_get_conf_val.
Thanks Philipp
v2: * Drop patch 2 as caused problems when value were edited in /etc/sysconfig/kdump * Fix false '!' in patch 13
Philipp Rudo (14): kdump-capture.service: switch to journal for stdout kdump-lib: fix typo in variable name kdumpctl: remove unnecessary uses of $? kdump-lib-initramfs: merge definitions for default ssh key kdumpctl: fix comment in check_and_wait_network_ready kdumpctl: forbid aliases from ssh config kdumpctl: simplify propagate_ssh_key kdumpctl: merge check_ssh_config into check_config kdumpctl: reduce file operations on kdump.conf kdumpctl: drop SAVE_PATH variable kdumpctl: drop SSH_KEY_LOCATION variable kdumpctl: drop DUMP_TARGET variable kdumpctl: remove kdump_get_conf_val in save_raw kdumpctl: simplify local_fs_dump_target
dracut-kdump-capture.service | 4 +- dracut-kdump.sh | 2 +- kdump-lib-initramfs.sh | 1 + kdump-lib.sh | 14 +-- kdumpctl | 227 +++++++++++++++-------------------- mkdumprd | 2 +- 6 files changed, 112 insertions(+), 138 deletions(-)
Using syslog for StandardOutput in a service file was deprecated in systemd v246 with commit f3dc6af20f ("core: automatically update StandardOuput=syslog to =journal (and similar for StandardError=)"). Thus the following warnings are printed in the crash kernel when creating a dump.
systemd[1]: /usr/lib/systemd/system/kdump-capture.service:23: Standard output type syslog is obsolete, automatically updating to journal. Please update your unit file, and consider removing the setting altogether. systemd[1]: /usr/lib/systemd/system/kdump-capture.service:24: Standard output type syslog+console is obsolete, automatically updating to journal+console. Please update your unit file, and consider removing the setting altogether.
Fix this by redirecting the stdout and stderr to the journal.
Signed-off-by: Philipp Rudo prudo@redhat.com --- dracut-kdump-capture.service | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dracut-kdump-capture.service b/dracut-kdump-capture.service index 3f20aba..7109169 100644 --- a/dracut-kdump-capture.service +++ b/dracut-kdump-capture.service @@ -20,8 +20,8 @@ Environment=NEWROOT=/sysroot Type=oneshot ExecStart=/bin/kdump.sh StandardInput=null -StandardOutput=syslog -StandardError=syslog+console +StandardOutput=journal +StandardError=journal+console KillMode=process RemainAfterExit=yes
in prepare_kdump_bootinfo s/defaut/default/.
While at it declare it with the other local variables as local.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index 4ed5035..5b1656e 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -640,7 +640,7 @@ prepare_kexec_args() prepare_kdump_bootinfo() { local boot_img boot_imglist boot_dirlist boot_initrdlist - local machine_id + local machine_id dir img default_initrd_base var_target_initrd_dir
if [[ -z $KDUMP_KERNELVER ]]; then KDUMP_KERNELVER="$(uname -r)" @@ -677,8 +677,8 @@ prepare_kdump_bootinfo() boot_initrdlist="initramfs-$KDUMP_KERNELVER.img initrd" for initrd in $boot_initrdlist; do if [[ -f "$KDUMP_BOOTDIR/$initrd" ]]; then - defaut_initrd_base="$initrd" - DEFAULT_INITRD="$KDUMP_BOOTDIR/$defaut_initrd_base" + default_initrd_base="$initrd" + DEFAULT_INITRD="$KDUMP_BOOTDIR/$default_initrd_base" break fi done @@ -686,12 +686,12 @@ prepare_kdump_bootinfo() # Create kdump initrd basename from default initrd basename # initramfs-5.7.9-200.fc32.x86_64.img => initramfs-5.7.9-200.fc32.x86_64kdump.img # initrd => initrdkdump - if [[ -z $defaut_initrd_base ]]; then + if [[ -z $default_initrd_base ]]; then kdump_initrd_base=initramfs-${KDUMP_KERNELVER}kdump.img - elif [[ $defaut_initrd_base == *.* ]]; then - kdump_initrd_base=${defaut_initrd_base%.*}kdump.${DEFAULT_INITRD##*.} + elif [[ $default_initrd_base == *.* ]]; then + kdump_initrd_base=${default_initrd_base%.*}kdump.${DEFAULT_INITRD##*.} else - kdump_initrd_base=${defaut_initrd_base}kdump + kdump_initrd_base=${default_initrd_base}kdump fi
# Place kdump initrd in $(/var/lib/kdump) if $(KDUMP_BOOTDIR) not writable
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 1869753..2ba0951 100755 --- a/kdumpctl +++ b/kdumpctl @@ -86,7 +86,6 @@ rebuild_fadump_initrd() check_earlykdump_is_enabled() { grep -q -w "rd.earlykdump" /proc/cmdline - return $? }
rebuild_kdump_initrd() @@ -117,8 +116,6 @@ rebuild_initrd() else rebuild_kdump_initrd fi - - return $? }
#$1: the files to be checked with IFS=' ' @@ -584,7 +581,6 @@ check_rebuild()
dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd - return $? }
# On ppc64le LPARs, the keys trusted by firmware do not end up in @@ -815,8 +811,6 @@ check_current_status() else check_current_kdump_status fi - - return $? }
save_raw() @@ -945,7 +939,6 @@ check_dump_feasibility() fi
check_kdump_feasibility - return $? }
start_fadump() @@ -967,8 +960,6 @@ start_dump() else load_kdump fi - - return $? }
check_failure_action_config() @@ -1077,7 +1068,7 @@ reload()
if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then reload_fadump - return $? + return else if ! stop_kdump; then derror "Stopping kdump: [FAILED]" @@ -1141,7 +1132,7 @@ reload_fadump() # to handle such scenario. if stop_fadump; then start_fadump - return $? + return fi fi
@@ -1180,7 +1171,6 @@ rebuild()
dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd - return $? }
do_estimate() @@ -1787,5 +1777,3 @@ single_instance_lock exec 9<&- main "$@" ) - -exit $?
There are currently three identical definitions for the default ssh key. Combine them into one in kdump-lib-initramfs.sh.
Signed-off-by: Philipp Rudo prudo@redhat.com --- dracut-kdump.sh | 2 +- kdump-lib-initramfs.sh | 1 + kdumpctl | 2 +- mkdumprd | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/dracut-kdump.sh b/dracut-kdump.sh index b69bc98..b17455a 100755 --- a/dracut-kdump.sh +++ b/dracut-kdump.sh @@ -22,7 +22,7 @@ FAILURE_ACTION="systemctl reboot -f" DATEDIR=$(date +%Y-%m-%d-%T) HOST_IP='127.0.0.1' DUMP_INSTRUCTION="" -SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa" +SSH_KEY_LOCATION=$DEFAULT_SSHKEY DD_BLKSIZE=512 FINAL_ACTION="systemctl reboot -f" KDUMP_PRE="" diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh index 9be0fe9..84e6bf7 100755 --- a/kdump-lib-initramfs.sh +++ b/kdump-lib-initramfs.sh @@ -4,6 +4,7 @@ # not be the default shell. Any code added must be POSIX compliant.
DEFAULT_PATH="/var/crash/" +DEFAULT_SSHKEY="/root/.ssh/kdump_id_rsa" KDUMP_CONFIG_FILE="/etc/kdump.conf" FENCE_KDUMP_CONFIG_FILE="/etc/sysconfig/fence_kdump" FENCE_KDUMP_SEND="/usr/libexec/fence_kdump_send" diff --git a/kdumpctl b/kdumpctl index 2ba0951..8e057e4 100755 --- a/kdumpctl +++ b/kdumpctl @@ -10,7 +10,7 @@ MKDUMPRD="/sbin/mkdumprd -f" MKFADUMPRD="/sbin/mkfadumprd" DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt" SAVE_PATH=/var/crash -SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa" +SSH_KEY_LOCATION=$DEFAULT_SSHKEY INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum" DUMP_TARGET="" DEFAULT_INITRD="" diff --git a/mkdumprd b/mkdumprd index 5996d50..3e250e0 100644 --- a/mkdumprd +++ b/mkdumprd @@ -22,7 +22,7 @@ if ! dlog_init; then exit 1 fi
-SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa" +SSH_KEY_LOCATION=$DEFAULT_SSHKEY SAVE_PATH=$(get_save_path) OVERRIDE_RESETTABLE=0
The time out was increased to 180 seconds in 680c0d3 ("kdumpctl: distinguish the failed reason of ssh"). Update the comment to reflect that change.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kdumpctl b/kdumpctl index 8e057e4..adc2dab 100755 --- a/kdumpctl +++ b/kdumpctl @@ -734,7 +734,7 @@ check_and_wait_network_ready()
cur=$(date +%s) diff=$((cur - start_time)) - # 60s time out + # time out after 180s if [[ $diff -gt 180 ]]; then break fi
For ssh targets kdumpctl only verifies that the config value has the correct <user>@<host> format itself. For all other tests, e.g. if the destination can be reached, it relies on ssh. This allows users to provide a <host> that isn't the proper hostname but an alias defined in the ssh_config without failing the tests. If this is done dracut-module-setup.sh:kdump_get_remote_ip will fail to obtain the targets ip address. This failure is not detected and thus will not fail the initramfs creation. The resulting initramfs however doesn't have the necessary information for setting up the network and thus will fail to boot.
Prevent the use of alias hostnames by verifying that the given hostname is the same one ssh would use after parsing the ssh_config.
Note: Don't use getent ahosts to verify that the given host can be resolved as this requires the network to be up which cannot be guaranteed when the kdump.conf is parsed.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/kdumpctl b/kdumpctl index adc2dab..db625f4 100755 --- a/kdumpctl +++ b/kdumpctl @@ -663,7 +663,7 @@ load_kdump()
check_ssh_config() { - local SSH_TARGET + local target
while read -r config_opt config_val; do case "$config_opt" in @@ -687,11 +687,14 @@ check_ssh_config() esac done <<< "$(kdump_read_conf)"
- #make sure they've configured kdump.conf for ssh dumps - SSH_TARGET=$(echo -n "$DUMP_TARGET" | sed -n '/.*@/p') - if [[ -z $SSH_TARGET ]]; then + [[ -n $DUMP_TARGET ]] || return 1 + [[ $DUMP_TARGET =~ .*@.* ]] || return 1 + target=$(ssh -G "$DUMP_TARGET" | sed -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p") + if [[ ${DUMP_TARGET#*@} != "$target" ]]; then + derror "Invalid ssh destination $DUMP_TARGET provided." return 1 fi + return 0 }
The function has multiple problems:
1) SSH_{USER,SERVER} aren't defined local 2) Weird use of cut and sed to parse the DUMP_TARGET for the user and host although check_ssh_config guarantees that it has the format <user>@<host>. 3) Unnecessary use of a variable for the return value 4) Weird behavior to first unpack the DUMP_TARGET to SSH_USER and SSH_SERVER and then putting it back together again 5) Definition of variable errmsg that is only used once but breaks grep-ability of error message. 6) Wrong order when redirecting output of ssh-keygen, see SC2069 [1]
Fix them now.
While at it also improve the error messages in the function.
[1] https://www.shellcheck.net/wiki/SC2069
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/kdumpctl b/kdumpctl index db625f4..eb3a3d2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -755,35 +755,32 @@ check_ssh_target()
propagate_ssh_key() { + local SSH_USER SSH_SERVER + if ! check_ssh_config; then - derror "No ssh config specified in $KDUMP_CONFIG_FILE. Can't propagate" + derror "No ssh destination defined in $KDUMP_CONFIG_FILE." + derror "Please verify that $KDUMP_CONFIG_FILE contains 'ssh <user>@<host>' and that it is properly formatted." exit 1 fi
local KEYFILE=$SSH_KEY_LOCATION - local errmsg="Failed to propagate ssh key"
#Check to see if we already created key, if not, create it. if [[ -f $KEYFILE ]]; then dinfo "Using existing keys..." else dinfo "Generating new ssh keys... " - /usr/bin/ssh-keygen -t rsa -f "$KEYFILE" -N "" 2>&1 > /dev/null + /usr/bin/ssh-keygen -t rsa -f "$KEYFILE" -N "" &> /dev/null dinfo "done." fi
- #now find the target ssh user and server to contact. - SSH_USER=$(echo "$DUMP_TARGET" | cut -d@ -f1) - SSH_SERVER=$(echo "$DUMP_TARGET" | sed -e's/(.*@)(.*$)/\2/') - - #now send the found key to the found server - ssh-copy-id -i "$KEYFILE" "$SSH_USER@$SSH_SERVER" - RET=$? - if [[ $RET == 0 ]]; then + SSH_USER=${DUMP_TARGET%@*} + SSH_SERVER=${DUMP_TARGET#*@} + if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER" return 0 else - derror "$errmsg, $KEYFILE failed in transfer to $SSH_SERVER" + derror "Failed to propagate ssh key, could not transfer $KEYFILE to $SSH_SERVER" exit 1 fi }
check_config and check_ssh_config both parse /etc/kdump.conf and are usually used together. The difference between both is that check_ssh_config does some extra checks on the format of the provided ssh destination but ignores invalid or deprecated options in the config. Thus merge check_ssh_config into check_config. Leave the additional checks on the ssh destination in check_ssh_config but treat it like the checks done for e.g. the failure_action.
This slightly changes the behavior of 'kdumpctl propagate', which now fails if kdump.conf contains an invalid value unrelated to ssh. This change in behavior isn't problematic because 'kdumpctl propagate' always needs to be followed by a 'kdumpctl start' to have a working kdump environment. For the situations where 'propagate' fails now the 'start' would have failed in the past. So the failure only moved one step ahead in the sequence.
While at it drop check_ssh_target and call check_and_wait_network_ready directly.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 71 ++++++++++++++++++++++++-------------------------------- 1 file changed, 30 insertions(+), 41 deletions(-)
diff --git a/kdumpctl b/kdumpctl index eb3a3d2..c11084e 100755 --- a/kdumpctl +++ b/kdumpctl @@ -205,10 +205,27 @@ check_config() fi config_opt=_target ;; - ext[234] | minix | btrfs | xfs | nfs | ssh) + ext[234] | minix | btrfs | xfs | nfs ) config_opt=_target ;; - sshkey | 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) ;; + ssh) + config_opt=_target + DUMP_TARGET=$config_val + ;; + sshkey) + if [[ -z $config_val ]]; then + derror "Invalid kdump config value for option '$config_opt'" + return 1 + elif [[ -f $config_val ]]; then + SSH_KEY_LOCATION=$(/usr/bin/readlink -m "$config_val") + else + dwarn "WARNING: '$config_val' doesn't exist, using default value '$SSH_KEY_LOCATION'" + fi + ;; + path) + SAVE_PATH=$config_val + ;; + 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) ;;
net | options | link_delay | disk_timeout | debug_mem_level | blacklist) derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives." @@ -242,6 +259,7 @@ check_config() check_failure_action_config || return 1 check_final_action_config || return 1 check_fence_kdump_config || return 1 + check_ssh_config || return 1
return 0 } @@ -665,29 +683,8 @@ check_ssh_config() { local target
- while read -r config_opt config_val; do - case "$config_opt" in - sshkey) - # remove inline comments after the end of a directive. - if [[ -f $config_val ]]; then - # canonicalize the path - SSH_KEY_LOCATION=$(/usr/bin/readlink -m "$config_val") - else - dwarn "WARNING: '$config_val' doesn't exist, using default value '$SSH_KEY_LOCATION'" - fi - ;; - path) - SAVE_PATH=$config_val - ;; - ssh) - DUMP_TARGET=$config_val - ;; - *) ;; - - esac - done <<< "$(kdump_read_conf)" + [[ -n $DUMP_TARGET ]] || return 0
- [[ -n $DUMP_TARGET ]] || return 1 [[ $DUMP_TARGET =~ .*@.* ]] || return 1 target=$(ssh -G "$DUMP_TARGET" | sed -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p") if [[ ${DUMP_TARGET#*@} != "$target" ]]; then @@ -710,6 +707,8 @@ check_and_wait_network_ready() local retval local errmsg
+ [[ -n $DUMP_TARGET ]] || return 0 + start_time=$(date +%s) while true; do errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "$SAVE_PATH" 2>&1) @@ -748,16 +747,13 @@ check_and_wait_network_ready() return 1 }
-check_ssh_target() -{ - check_and_wait_network_ready -} - propagate_ssh_key() { local SSH_USER SSH_SERVER
- if ! check_ssh_config; then + check_config || return 1 + + if [[ -z $DUMP_TARGET ]] ; then derror "No ssh destination defined in $KDUMP_CONFIG_FILE." derror "Please verify that $KDUMP_CONFIG_FILE contains 'ssh <user>@<host>' and that it is properly formatted." exit 1 @@ -1040,11 +1036,9 @@ start() return 0 fi
- if check_ssh_config; then - if ! check_ssh_target; then - derror "Starting kdump: [FAILED]" - return 1 - fi + if ! check_and_wait_network_ready; then + derror "Starting kdump: [FAILED]" + return 1 fi
if ! check_rebuild; then @@ -1160,12 +1154,7 @@ stop() rebuild() { check_config || return 1 - - if check_ssh_config; then - if ! check_ssh_target; then - return 1 - fi - fi + check_and_wait_network_ready || return 1
setup_initrd || return 1
Every call to kdump_get_conf_val parses kdump.conf although the file has already been parsed in check_config. Thus store the values parsed in check_config in an array and use them later instead of re-parsing the file over and over again.
While at it rename check_config to parse_config.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 73 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 32 deletions(-)
diff --git a/kdumpctl b/kdumpctl index c11084e..7fada01 100755 --- a/kdumpctl +++ b/kdumpctl @@ -27,6 +27,8 @@ standard_kexec_args="-d -p" # Some default values in case /etc/sysconfig/kdump doesn't include KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"
+declare -A OPT + if [[ -f /etc/sysconfig/kdump ]]; then . /etc/sysconfig/kdump fi @@ -185,9 +187,29 @@ restore_default_initrd() fi }
-check_config() +_set_config() +{ + local opt=$1 + local val=$2 + + if [[ -z $val ]]; then + derror "Invalid kdump config value for option '$opt'" + return 1 + fi + + if [[ -n ${OPT[$opt]} ]]; then + if [[ $opt == _target ]]; then + derror "More than one dump targets specified" + else + derror "Duplicated kdump config value of option $opt" + fi + return 1 + fi + OPT[$opt]="$val" +} + +parse_config() { - local -A _opt_rec while read -r config_opt config_val; do case "$config_opt" in dracut_args) @@ -196,7 +218,7 @@ check_config() derror 'Multiple mount targets specified in one "dracut_args".' return 1 fi - config_opt=_target + _set_config _target "$(get_dracut_args_target "$config_val")" || return 1 fi ;; raw) @@ -240,20 +262,7 @@ check_config() ;; esac
- if [[ -z $config_val ]]; then - derror "Invalid kdump config value for option '$config_opt'" - return 1 - fi - - if [[ -n ${_opt_rec[$config_opt]} ]]; then - if [[ $config_opt == _target ]]; then - derror "More than one dump targets specified" - else - derror "Duplicated kdump config value of option $config_opt" - fi - return 1 - fi - _opt_rec[$config_opt]="$config_val" + _set_config "$config_opt" "$config_val" || return 1 done <<< "$(kdump_read_conf)"
check_failure_action_config || return 1 @@ -321,8 +330,8 @@ check_files_modified() #also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled. modified_files=$(get_pcs_cluster_modified_files)
- EXTRA_BINS=$(kdump_get_conf_val kdump_post) - CHECK_FILES=$(kdump_get_conf_val kdump_pre) + EXTRA_BINS=${OPT[kdump_post]} + CHECK_FILES=${OPT[kdump_pre]} HOOKS="/etc/kdump/post.d/ /etc/kdump/pre.d/" if [[ -d /etc/kdump/post.d ]]; then for file in /etc/kdump/post.d/*; do @@ -339,17 +348,17 @@ check_files_modified() done fi HOOKS="$HOOKS $POST_FILES $PRE_FILES" - CORE_COLLECTOR=$(kdump_get_conf_val core_collector | awk '{print $1}') + CORE_COLLECTOR=$(echo "${OPT[core_collector]}" | awk '{print $1}') CORE_COLLECTOR=$(type -P "$CORE_COLLECTOR") # POST_FILES and PRE_FILES are already checked against executable, need not to check again. EXTRA_BINS="$EXTRA_BINS $CHECK_FILES" - CHECK_FILES=$(kdump_get_conf_val extra_bins) + CHECK_FILES=${OPT[extra_bins]} EXTRA_BINS="$EXTRA_BINS $CHECK_FILES" files="$KDUMP_CONFIG_FILE $KDUMP_KERNEL $EXTRA_BINS $CORE_COLLECTOR" [[ -e /etc/fstab ]] && files="$files /etc/fstab"
# Check for any updated extra module - EXTRA_MODULES="$(kdump_get_conf_val extra_modules)" + EXTRA_MODULES="${OPT[extra_modules]}" if [[ -n $EXTRA_MODULES ]]; then if [[ -e /lib/modules/$KDUMP_KERNELVER/modules.dep ]]; then files="$files /lib/modules/$KDUMP_KERNELVER/modules.dep" @@ -541,14 +550,14 @@ check_rebuild()
setup_initrd || return 1
- force_no_rebuild=$(kdump_get_conf_val force_no_rebuild) + force_no_rebuild=${OPT[force_no_rebuild]} force_no_rebuild=${force_no_rebuild:-0} if [[ $force_no_rebuild != "0" ]] && [[ $force_no_rebuild != "1" ]]; then derror "Error: force_no_rebuild value is invalid" return 1 fi
- force_rebuild=$(kdump_get_conf_val force_rebuild) + force_rebuild=${OPT[force_rebuild]} force_rebuild=${force_rebuild:-0} if [[ $force_rebuild != "0" ]] && [[ $force_rebuild != "1" ]]; then derror "Error: force_rebuild value is invalid" @@ -751,7 +760,7 @@ propagate_ssh_key() { local SSH_USER SSH_SERVER
- check_config || return 1 + parse_config || return 1
if [[ -z $DUMP_TARGET ]] ; then derror "No ssh destination defined in $KDUMP_CONFIG_FILE." @@ -825,7 +834,7 @@ save_raw() dwarn "Warning: Detected '$check_fs' signature on $raw_target, data loss is expected." return 0 fi - kdump_dir=$(kdump_get_conf_val path) + kdump_dir=${OPT[path]} if [[ -z ${kdump_dir} ]]; then coredir="/var/crash/$(date +"%Y-%m-%d-%H:%M")" else @@ -911,7 +920,7 @@ check_fence_kdump_config()
hostname=$(hostname) ipaddrs=$(hostname -I) - nodes=$(kdump_get_conf_val "fence_kdump_nodes") + nodes=${OPT[fence_kdump_nodes]}
for node in $nodes; do if [[ $node == "$hostname" ]]; then @@ -964,8 +973,8 @@ check_failure_action_config() local failure_action local option="failure_action"
- default_option=$(kdump_get_conf_val default) - failure_action=$(kdump_get_conf_val failure_action) + default_option=${OPT[default]} + failure_action=${OPT[failure_action]}
if [[ -z $failure_action ]] && [[ -z $default_option ]]; then return 0 @@ -994,7 +1003,7 @@ check_final_action_config() { local final_action
- final_action=$(kdump_get_conf_val final_action) + final_action=${OPT[final_action]} if [[ -z $final_action ]]; then return 0 else @@ -1017,7 +1026,7 @@ start() return 1 fi
- if ! check_config; then + if ! parse_config; then derror "Starting kdump: [FAILED]" return 1 fi @@ -1153,7 +1162,7 @@ stop()
rebuild() { - check_config || return 1 + parse_config || return 1 check_and_wait_network_ready || return 1
setup_initrd || return 1
The variable is only used for ssh dump targets. Furthermore it is identical to the value stored in ${OPT[path]}. Thus drop SAVE_PATH and use ${OPT[path]} instead.
Also make sure that ${OPT[path]} is always set to the default value when no entry in kdump.conf is found.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 7fada01..a35c55b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -9,7 +9,6 @@ KDUMP_LOG_PATH="/var/log" MKDUMPRD="/sbin/mkdumprd -f" MKFADUMPRD="/sbin/mkfadumprd" DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt" -SAVE_PATH=/var/crash SSH_KEY_LOCATION=$DEFAULT_SSHKEY INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum" DUMP_TARGET="" @@ -244,10 +243,7 @@ parse_config() dwarn "WARNING: '$config_val' doesn't exist, using default value '$SSH_KEY_LOCATION'" fi ;; - path) - SAVE_PATH=$config_val - ;; - 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) ;; + 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) ;;
net | options | link_delay | disk_timeout | debug_mem_level | blacklist) derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives." @@ -265,6 +261,8 @@ parse_config() _set_config "$config_opt" "$config_val" || return 1 done <<< "$(kdump_read_conf)"
+ OPT[path]=${OPT[path]:-$DEFAULT_PATH} + check_failure_action_config || return 1 check_final_action_config || return 1 check_fence_kdump_config || return 1 @@ -720,21 +718,21 @@ check_and_wait_network_ready()
start_time=$(date +%s) while true; do - errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "$SAVE_PATH" 2>&1) + errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1) retval=$?
# ssh exits with the exit status of the remote command or with 255 if an error occurred if [[ $retval -eq 0 ]]; then return 0 elif [[ $retval -ne 255 ]]; then - derror "Could not create $DUMP_TARGET:$SAVE_PATH, you should check the privilege on server side" + derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side" return 1 fi
# if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa ddebug "$errmsg" if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then - derror "Could not create $DUMP_TARGET:$SAVE_PATH, you probably need to run "kdumpctl propagate"" + derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run "kdumpctl propagate"" return 1 fi
@@ -752,7 +750,7 @@ check_and_wait_network_ready() sleep 1 done
- dinfo "Could not create $DUMP_TARGET:$SAVE_PATH, ipaddr is not ready yet. You should check network connection" + dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection" return 1 }
@@ -820,7 +818,6 @@ check_current_status()
save_raw() { - local kdump_dir local raw_target
raw_target=$(kdump_get_conf_val raw) @@ -834,13 +831,8 @@ save_raw() dwarn "Warning: Detected '$check_fs' signature on $raw_target, data loss is expected." return 0 fi - kdump_dir=${OPT[path]} - if [[ -z ${kdump_dir} ]]; then - coredir="/var/crash/$(date +"%Y-%m-%d-%H:%M")" - else - coredir="${kdump_dir}/$(date +"%Y-%m-%d-%H:%M")" - fi
+ coredir="${OPT[path]}/$(date +"%Y-%m-%d-%H:%M")" mkdir -p "$coredir" [[ -d $coredir ]] || { derror "failed to create $coredir"
The variable is only used for ssh dump targets. Furthermore it is identical to the value stored in ${OPT[sshkey]}. Thus drop SSH_KEY_LOCATION and use ${OPT[sshkey]} instead.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index a35c55b..97d97f5 100755 --- a/kdumpctl +++ b/kdumpctl @@ -9,7 +9,6 @@ KDUMP_LOG_PATH="/var/log" MKDUMPRD="/sbin/mkdumprd -f" MKFADUMPRD="/sbin/mkfadumprd" DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt" -SSH_KEY_LOCATION=$DEFAULT_SSHKEY INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum" DUMP_TARGET="" DEFAULT_INITRD="" @@ -238,9 +237,10 @@ parse_config() derror "Invalid kdump config value for option '$config_opt'" return 1 elif [[ -f $config_val ]]; then - SSH_KEY_LOCATION=$(/usr/bin/readlink -m "$config_val") + config_val=$(/usr/bin/readlink -m "$config_val") else - dwarn "WARNING: '$config_val' doesn't exist, using default value '$SSH_KEY_LOCATION'" + dwarn "WARNING: '$config_val' doesn't exist, using default value '$DEFAULT_SSHKEY'" + 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) ;; @@ -262,6 +262,7 @@ parse_config() done <<< "$(kdump_read_conf)"
OPT[path]=${OPT[path]:-$DEFAULT_PATH} + OPT[sshkey]=${OPT[sshkey]:-$DEFAULT_SSHKEY}
check_failure_action_config || return 1 check_final_action_config || return 1 @@ -718,7 +719,7 @@ check_and_wait_network_ready()
start_time=$(date +%s) while true; do - errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1) + errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1) retval=$?
# ssh exits with the exit status of the remote command or with 255 if an error occurred @@ -766,7 +767,7 @@ propagate_ssh_key() exit 1 fi
- local KEYFILE=$SSH_KEY_LOCATION + local KEYFILE=${OPT[sshkey]}
#Check to see if we already created key, if not, create it. if [[ -f $KEYFILE ]]; then
The variable is only used for ssh dump targets. Furthermore it is identical to the value stored in ${OPT[_target]}. Thus drop DUMP_TARGET and use ${OPT[_target]} instead.
In order to be able to distinguish between the different target types introduce the internal ${OPT[_fstype]}.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 97d97f5..a26f0bd 100755 --- a/kdumpctl +++ b/kdumpctl @@ -10,7 +10,6 @@ MKDUMPRD="/sbin/mkdumprd -f" MKFADUMPRD="/sbin/mkfadumprd" DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt" INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum" -DUMP_TARGET="" DEFAULT_INITRD="" DEFAULT_INITRD_BAK="" KDUMP_INITRD="" @@ -196,7 +195,7 @@ _set_config() fi
if [[ -n ${OPT[$opt]} ]]; then - if [[ $opt == _target ]]; then + if [[ $opt == _target ]] || [[ $opt == _fstype ]]; then derror "More than one dump targets specified" else derror "Duplicated kdump config value of option $opt" @@ -216,6 +215,7 @@ parse_config() derror 'Multiple mount targets specified in one "dracut_args".' return 1 fi + _set_config _fstype "$(get_dracut_args_fstype "$config_val")" || return 1 _set_config _target "$(get_dracut_args_target "$config_val")" || return 1 fi ;; @@ -223,15 +223,13 @@ parse_config() if [[ -d "/proc/device-tree/ibm,opal/dump" ]]; then dwarn "WARNING: Won't capture opalcore when 'raw' dump target is used." fi + _set_config _fstype "$config_opt" || return 1 config_opt=_target ;; - ext[234] | minix | btrfs | xfs | nfs ) + ext[234] | minix | btrfs | xfs | nfs | ssh) + _set_config _fstype "$config_opt" || return 1 config_opt=_target ;; - ssh) - config_opt=_target - DUMP_TARGET=$config_val - ;; sshkey) if [[ -z $config_val ]]; then derror "Invalid kdump config value for option '$config_opt'" @@ -691,12 +689,12 @@ check_ssh_config() { local target
- [[ -n $DUMP_TARGET ]] || return 0 + [[ "${OPT[_fstype]}" == ssh ]] || return 0
- [[ $DUMP_TARGET =~ .*@.* ]] || return 1 - target=$(ssh -G "$DUMP_TARGET" | sed -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p") - if [[ ${DUMP_TARGET#*@} != "$target" ]]; then - derror "Invalid ssh destination $DUMP_TARGET provided." + target=$(ssh -G "${OPT[_target]}" | sed -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p") + [[ ${OPT[_target]} =~ .*@.* ]] || return 1 + if [[ ${OPT[_target]#*@} != "$target" ]]; then + derror "Invalid ssh destination ${OPT[_target]} provided." return 1 fi
@@ -715,25 +713,25 @@ check_and_wait_network_ready() local retval local errmsg
- [[ -n $DUMP_TARGET ]] || return 0 + [[ "${OPT[_fstype]}" == ssh ]] || return 0
start_time=$(date +%s) while true; do - errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1) + errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "${OPT[_target]}" mkdir -p "${OPT[path]}" 2>&1) retval=$?
# ssh exits with the exit status of the remote command or with 255 if an error occurred if [[ $retval -eq 0 ]]; then return 0 elif [[ $retval -ne 255 ]]; then - derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side" + derror "Could not create ${OPT[_target]}:${OPT[path]}, you should check the privilege on server side" return 1 fi
# if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa ddebug "$errmsg" if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then - derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run "kdumpctl propagate"" + derror "Could not create ${OPT[_target]}:${OPT[path]}, you probably need to run "kdumpctl propagate"" return 1 fi
@@ -751,7 +749,7 @@ check_and_wait_network_ready() sleep 1 done
- dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection" + dinfo "Could not create ${OPT[_target]}:${OPT[path]}, ipaddr is not ready yet. You should check network connection" return 1 }
@@ -761,7 +759,7 @@ propagate_ssh_key()
parse_config || return 1
- if [[ -z $DUMP_TARGET ]] ; then + if [[ ${OPT[_fstype]} != ssh ]] ; then derror "No ssh destination defined in $KDUMP_CONFIG_FILE." derror "Please verify that $KDUMP_CONFIG_FILE contains 'ssh <user>@<host>' and that it is properly formatted." exit 1 @@ -778,9 +776,9 @@ propagate_ssh_key() dinfo "done." fi
- SSH_USER=${DUMP_TARGET%@*} - SSH_SERVER=${DUMP_TARGET#*@} - if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then + SSH_USER=${OPT[_target]%@*} + SSH_SERVER=${OPT[_target]#*@} + if ssh-copy-id -i "$KEYFILE" "${OPT[_target]}"; then dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER" return 0 else
With the introduction of ${OPT[fstype]} this call to kdump_get_conf_val can be removed now as well.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index a26f0bd..d7dcf1a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -819,8 +819,9 @@ save_raw() { local raw_target
- raw_target=$(kdump_get_conf_val raw) - [[ -z $raw_target ]] && return 0 + [[ ${OPT[_fstype]} == raw ]] || return 0 + + raw_target=${OPT[_target]} [[ -b $raw_target ]] || { derror "raw partition $raw_target not found" return 1
Make use of the new ${OPT[]} array and simplify local_fs_dump_target to remove one more file operations.
While at it rename the local_fs_dump_target to is_local_target
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/kdumpctl b/kdumpctl index d7dcf1a..499e93b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -850,27 +850,22 @@ save_raw() return 0 }
-local_fs_dump_target() +is_local_target() { - local _target - - if _target=$(grep -E "^ext[234]|^xfs|^btrfs|^minix" /etc/kdump.conf); then - echo "$_target" | awk '{print $2}' - fi + [[ ${OPT[_fstype]} =~ ^ext[234]|^xfs|^btrfs|^minix ]] }
path_to_be_relabeled() { - local _path _target _mnt="/" _rmnt + local _path _mnt="/" _rmnt
if is_user_configured_dump_target; then if is_mount_in_dracut_args; then return fi
- _target=$(local_fs_dump_target) - if [[ -n $_target ]]; then - _mnt=$(get_mntpoint_from_target "$_target") + if is_local_target; then + _mnt=$(get_mntpoint_from_target "${OPT[_target]}") if ! is_mounted "$_mnt"; then return fi
Hi Philipp,
Thanks for the patch set! For the whole patch set,
Reviewed-by: Coiby Xu coxu@redhat.com
And also thanks to Tao for reviewing the patches!
On Fri, Mar 25, 2022 at 03:46:57PM +0100, Philipp Rudo wrote:
Hi,
while looking into transforming mkdumprd and mkfadumprd into library functions I noticed various nits in kdumpctl. This series addresses them.
The series is made up of two parts:
Patches 1-8 are small independent cleanups and fixes.
Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It achieves this by only parsing kdump.conf once in check_config and storing the parsed values in an array. Later accesses can then simply use the value stored in the array instead of calling kdump_get_conf_val.
Thanks Philipp
v2:
- Drop patch 2 as caused problems when value were edited in /etc/sysconfig/kdump
- Fix false '!' in patch 13
Philipp Rudo (14): kdump-capture.service: switch to journal for stdout kdump-lib: fix typo in variable name kdumpctl: remove unnecessary uses of $? kdump-lib-initramfs: merge definitions for default ssh key kdumpctl: fix comment in check_and_wait_network_ready kdumpctl: forbid aliases from ssh config kdumpctl: simplify propagate_ssh_key kdumpctl: merge check_ssh_config into check_config kdumpctl: reduce file operations on kdump.conf kdumpctl: drop SAVE_PATH variable kdumpctl: drop SSH_KEY_LOCATION variable kdumpctl: drop DUMP_TARGET variable kdumpctl: remove kdump_get_conf_val in save_raw kdumpctl: simplify local_fs_dump_target
dracut-kdump-capture.service | 4 +- dracut-kdump.sh | 2 +- kdump-lib-initramfs.sh | 1 + kdump-lib.sh | 14 +-- kdumpctl | 227 +++++++++++++++-------------------- mkdumprd | 2 +- 6 files changed, 112 insertions(+), 138 deletions(-)
-- 2.35.1
-
Coiby Xu -
Philipp Rudo