Re: [PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

Hi Philipp, I have some concerns about this patch. On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo <prudo(a)redhat.com&gt; wrote: > > 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(a)redhat.com&gt; > --- > kdumpctl | 71 ++++++++++++++++++++++++-------------------------------- > 1 file changed, 30 insertions(+), 41 deletions(-) > > diff --git a/kdumpctl b/kdumpctl > index a311238..ebb634b 100755 > --- a/kdumpctl > +++ b/kdumpctl > @@ -204,10 +204,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) ;; Personally I prefer the original way, which checks ssh specific configs in check_ssh_config(), and check_config() do the generic checking. > > net | options | link_delay | disk_timeout | debug_mem_level | blacklist) > derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives." > @@ -241,6 +258,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 > } > @@ -664,29 +682,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)" If you want to emit the extra /etc/kdump.conf parsing, how about using kdump_get_conf_val() to get values of sshkey, path, ssh, just like how check_failure_action_config() does, so we can keep the ssh specific checking in this function?

> + [[ -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 > @@ -709,6 +706,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) > @@ -747,16 +746,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 > + Yes, just like what the commit message says, it will change the behaviour of propagate, if any non-ssh configuration error will fail ssh key propagate. It seems not a good idea to me...

Thanks, Tao Liu > + 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 > @@ -1039,11 +1035,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 > @@ -1159,12 +1153,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 > > -- > 2.34.1 > _______________________________________________ > kexec mailing list -- kexec(a)lists.fedoraproject.org > To unsubscribe send an email to kexec-leave(a)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 on the list, report it: https://pagure.io/fedora-infrastructure