Hi Coiby,

On Tue, 22 Feb 2022 14:44:51 +0800 Coiby Xu coxu@redhat.com wrote:

On Fri, Feb 04, 2022 at 06:34:16PM +0100, Philipp Rudo wrote:

...

Allow the use of default values defined in kdump-lib-initramfs.sh and kdump-lib.sh for global variables by moving their initialization before the definition of the variables.

Signed-off-by: Philipp Rudo prudo@redhat.com

kdumpctl | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 3ccfa97..ed772a0 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1,4 +1,20 @@ #!/bin/bash

+if [[ -f /etc/sysconfig/kdump ]]; then

• . /etc/sysconfig/kdump

+fi

+[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut +. $dracutbasedir/dracut-functions.sh +. /lib/kdump/kdump-lib.sh +. /lib/kdump/kdump-logger.sh

+#initiate the kdump logger +if ! dlog_init; then

• echo "failed to initiate the kdump logger."
• exit 1

+fi

KEXEC=/sbin/kexec

KDUMP_KERNELVER="" @@ -9,7 +25,7 @@ KDUMP_LOG_PATH="/var/log" MKDUMPRD="/sbin/mkdumprd -f" MKFADUMPRD="/sbin/mkfadumprd" DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt" -SAVE_PATH=/var/crash +SAVE_PATH=$DEFAULT_PATH SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa" INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum" DUMP_TARGET="" @@ -27,21 +43,6 @@ standard_kexec_args="-d -p" # Some default values in case /etc/sysconfig/kdump doesn't include KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"

One issue with sourcing /etc/sysconfig/kdump early is configurations like KDUMP_KERNELVER, KDUMP_COMMANDLINE_REMOVE and etc. could be overwritten. Maybe we simply move "SAVE_PATH=$DEFAULT_PATH" instead?

you are right. With this patch the values in /etc/sysconfig/kdump get overwritten again...

I took an other look and I think I will drop this patch. The only two variables that benefit from this change are SAVE_PATH and SSH_KEY_LOCATION (after patch 5). But both are dropped in later patches. So there is no real benefit in keeping this patch. Especially when it breaks stuff...

Thanks Philipp

...

-if [[ -f /etc/sysconfig/kdump ]]; then

• . /etc/sysconfig/kdump

-fi

-[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut -. $dracutbasedir/dracut-functions.sh -. /lib/kdump/kdump-lib.sh -. /lib/kdump/kdump-logger.sh

-#initiate the kdump logger -if ! dlog_init; then

• echo "failed to initiate the kdump logger."
• exit 1

-fi

single_instance_lock() { local rc timeout=5 -- 2.34.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 on the list, report it: https://pagure.io/fedora-infrastructure

Hi Coiby,

On Tue, 22 Feb 2022 15:07:12 +0800 Coiby Xu coxu@redhat.com wrote:

On Fri, Feb 04, 2022 at 06:34:18PM +0100, Philipp Rudo wrote:

...

Signed-off-by: Philipp Rudo prudo@redhat.com

kdumpctl | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/kdumpctl b/kdumpctl index ed772a0..d08b91c 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() @@ -116,8 +115,6 @@ rebuild_initrd() else rebuild_kdump_initrd fi

• return $?

}

#$1: the files to be checked with IFS=' ' @@ -583,7 +580,6 @@ check_rebuild()

dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd

• return $?

}

# On ppc64le LPARs, the keys trusted by firmware do not end up in @@ -814,8 +810,6 @@ check_current_status() else check_current_kdump_status fi

• return $?

}

save_raw() @@ -944,7 +938,6 @@ check_dump_feasibility() fi

check_kdump_feasibility

• return $?

}

start_fadump() @@ -966,8 +959,6 @@ start_dump() else load_kdump fi

• return $?

}

check_failure_action_config() @@ -1076,7 +1067,7 @@ reload()

if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then reload_fadump

• return $?
  

• return
  

  else if ! stop_kdump; then derror "Stopping kdump: [FAILED]"

@@ -1140,7 +1131,7 @@ reload_fadump() # to handle such scenario. if stop_fadump; then start_fadump

• 	return $?
  

• 	return  
  

I'm curious for start_fadump and reload_fadump both of which may return 1, why only return normal status?

There is no "normal status" you can return. When you omit the return value 'return' will automatically return the status of the last executed command. Thus 'return' and 'return $?' are identical.

Same for 'exit'

Thanks Philipp

...

fi

fi

@@ -1179,7 +1170,6 @@ rebuild()

dinfo "Rebuilding $TARGET_INITRD" rebuild_initrd

• return $?

}

do_estimate() @@ -1711,5 +1701,3 @@ single_instance_lock exec 9<&- main "$@" )

-exit $?

2.34.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 on the list, report it: https://pagure.io/fedora-infrastructure

Hi Coiby,

On Wed, 23 Feb 2022 08:45:15 +0800 Coiby Xu coxu@redhat.com wrote:

Hi Philipp,

On Tue, Feb 22, 2022 at 11:22:20AM +0100, Philipp Rudo wrote:

...

Hi Coiby,

On Tue, 22 Feb 2022 15:07:12 +0800 Coiby Xu coxu@redhat.com wrote:

[...]

...

...

...

start_fadump() @@ -966,8 +959,6 @@ start_dump() else load_kdump fi

• return $?

}

check_failure_action_config() @@ -1076,7 +1067,7 @@ reload()

if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then reload_fadump

• return $?
  

• return
  

  else if ! stop_kdump; then derror "Stopping kdump: [FAILED]"

@@ -1140,7 +1131,7 @@ reload_fadump() # to handle such scenario. if stop_fadump; then start_fadump

• 	return $?
  

• 	return  
  

I'm curious for start_fadump and reload_fadump both of which may return 1, why only return normal status?

There is no "normal status" you can return. When you omit the return value 'return' will automatically return the status of the last executed command. Thus 'return' and 'return $?' are identical.

Thanks for correcting my misunderstanding! But why not simply removing "return $?" instead?

I don't fully understand your question. Do you mean doing something like this?

diff --git a/kdumpctl b/kdumpctl index 630e56f..24c266f 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1121,11 +1121,10 @@ reload_fadump() # to handle such scenario. if stop_fadump; then start_fadump - return fi fi

- return 1 + return }

stop()

Unfortunately that won't work as expected. The problem is that in bash control statements like 'if' or 'for' do have a return value. This is usually the return value of the last command executed _or_ 0 if, e.g. no test condition is true for 'if' (for a full list see 'Compound Commands' section in the bash man page). Thus the above will change the return value in the case that stop_fadump fails.

You can test that behavior with this snippet

if false; then false fi && echo "return value is 0" || echo "return value is 1"

which will print "return value is 0". However, when you update 'if false;' to 'if true;' then it will print "return value is 1".

Thanks Philipp

Hi Coiby,

On Fri, 25 Feb 2022 14:45:27 +0800 Coiby Xu coxu@redhat.com wrote:

On Thu, Feb 24, 2022 at 05:49:33PM +0100, Philipp Rudo wrote:

...

Hi Coiby,

...

Thanks for correcting my misunderstanding! But why not simply removing "return $?" instead?

I don't fully understand your question. Do you mean doing something like this?

Sorry for not being clear enough.

...

diff --git a/kdumpctl b/kdumpctl index 630e56f..24c266f 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1121,11 +1121,10 @@ reload_fadump() # to handle such scenario. if stop_fadump; then start_fadump

•                   return  
  

Although I only suggested removing this return originally (which obvious is incorrect if I read the remaining code in reload_fadump),

...

           fi
   fi

•   return 1
  

•   return  
  

actually it works with this return also removed.

hmm... you are right that this 'return' is redundant as functions, like 'if' statements, automatically return the value of the last command executed when they reach the end. However, dropping it still changes the behavior compared to the original version.

...

}

stop()

Unfortunately that won't work as expected.

...

The problem is that in bash control statements like 'if' or 'for' do have a return value. This is usually the return value of the last command executed _or_ 0 if, e.g. no test condition is true for 'if' (for a full list see 'Compound Commands' section in the bash man page). Thus the above will change the return value in the case that stop_fadump fails.

You can test that behavior with this snippet

if false; then false fi && echo "return value is 0" || echo "return value is 1"

which will print "return value is 0". However, when you update 'if false;' to 'if true;' then it will print "return value is 1".

If we modify your example to the same if/else/fi structure as reload_fadump,

if false; then false else true fi && echo "return value is 0" || echo "return value is 1"

this will print "return value is 0". And changing the only true to false will print "return value is 1".

That's absolutely right. But you're still missing that the original code returned 'false' in that case. So dropping the return will change the return value of the function for that case.

Let's look at the full function. For the problematic case the original code looked like this

f() { if false; then echo "outer if" return else if false; then echo "inner if" return fi fi return 1 }

For this case the return value of the function will always be 1 (aka. false). If we changed the last line of the function to a simple 'return' (or drop the line altogether), then the return value will be determined by the outer 'if' statement. For that case it will be the return value of the last command executed in the 'else' block. But the 'else' block only has one command which is the inner 'if' statement. For the inner 'if' none of the conditions are true so it returns 0 (aka. true). So the return value of the function changed from 1 to 0.

Thanks Philipp

Hi Tao,

On Tue, 22 Feb 2022 17:37:31 +0800 Tao Liu ltao@redhat.com wrote:

Hi Philipp,

I have some concerns about this patch.

On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo prudo@redhat.com 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@redhat.com

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?

omitting the extra parsing is the intermediate goal of this patch. The ultimate goal is to introduce the OPT array in the next patch. This allows simplifying kdumpctl a lot. Especially it allows to get rid of most calls to kdump_get_conf_val in kdumpctl and with it quite a lot of file accesses.

Looking even further in the future the goal is to transform mkdumprd from a script to a library. It has a similar parser and thus could make use of the OPT array as well. I don't have it fully worked out yet, though.

...

•   [[ -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...

For me the over all simplification was worth the change in behavior. Especially failing when encountering unknown or deprecating I think would be very helpful. Al least I think it would have prevented this bug [1].

We could separate the parsing and verification steps into two functions. I.e. have one function that parses kdump.conf into the OPT array (only failing for unknown/deprecated options) and a second one that verifies the config options. Then propagate could only check the ssh options while the other function call the "check everything" function. Would that be ok with you?

Thanks Philipp

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2049153

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@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 on the list, report it: https://pagure.io/fedora-infrastructure

Hi Tao,

On Wed, 23 Feb 2022 18:12:45 +0800 Tao Liu ltao@redhat.com wrote:

Hi Philipp,

On Tue, Feb 22, 2022 at 03:17:39PM +0100, Philipp Rudo wrote:

...

Hi Tao,

On Tue, 22 Feb 2022 17:37:31 +0800 Tao Liu ltao@redhat.com wrote:

...

Hi Philipp,

I have some concerns about this patch.

On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo prudo@redhat.com 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@redhat.com

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?

omitting the extra parsing is the intermediate goal of this patch. The ultimate goal is to introduce the OPT array in the next patch. This allows simplifying kdumpctl a lot. Especially it allows to get rid of most calls to kdump_get_conf_val in kdumpctl and with it quite a lot of file accesses.

Looking even further in the future the goal is to transform mkdumprd from a script to a library. It has a similar parser and thus could make use of the OPT array as well. I don't have it fully worked out yet, though.

Sorry I didn't look through all your patches. BTW, great design for the OPT array, the config parser and setter, which making the code much cleaner.

np and thanks.

...

...

...

•   [[ -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...

For me the over all simplification was worth the change in behavior. Especially failing when encountering unknown or deprecating I think would be very helpful. Al least I think it would have prevented this bug [1].

I see...

...

We could separate the parsing and verification steps into two functions. I.e. have one function that parses kdump.conf into the OPT array (only failing for unknown/deprecated options) and a second one that verifies the config options. Then propagate could only check the ssh options while the other function call the "check everything" function. Would that be ok with you?

Yes, thanks for the explanation, I'm OK with your desgin.

BTW, for all the patches:

Reviewed-by: Tao Liu ltao@redhat.com

Thanks for reviewing! Philipp

Thanks, Tao Liu

...

Thanks Philipp

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2049153

...

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@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 on the list, report it: https://pagure.io/fedora-infrastructure

Hi Coiby,

finally there's time to go through my fedora mails. Please excuse the very late answer.

On Tue, 1 Mar 2022 12:33:32 +0800 Coiby Xu coxu@redhat.com wrote:

On Fri, Feb 04, 2022 at 06:34:27PM +0100, Philipp Rudo wrote:

...

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 e70b121..aaf753b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -28,7 +28,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="" @@ -195,7 +194,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"

@@ -215,6 +214,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
  ;;
  

@@ -222,15 +222,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'"

@@ -690,12 +688,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

@@ -714,25 +712,25 @@ check_and_wait_network_ready() local retval local errmsg

• [[ -n $DUMP_TARGET ]] || return 0

• [[ "${OPT[_fstype]}" != ssh ]] || return 0

                      ^^
                     s/!=/==

Thanks! That's definitely not what it should be...

As caught by the kdump tests, this error would make kdump.service fail to start when dump target is not ssh and thus all non-ssh tests fails,

$ RELEASE=34 make test-run ======== Test results ========

---

nfs-kdump: TEST FAILED

nfs-early-kdump: TEST FAILED

ssh-kdump: TEST PASSED

local-kdump: TEST FAILED

I should take a look at your series. These tests seem to be very handy ;-)

An unrelated question why no check_and_wait_network_ready for NFS dump target since it also requires the network to be ready. Pinfang, do you know the answer since you are author of commit c1a0634 ("kdumpctl: wait a while for network ready if dump target is ssh")?

Good question... The kdump.serve has a 'After=remote-fs.target' which should be enough. At least when the kdump.service is used and the dump target is auto mounted. Furthermore, if we wanted to use check_and_wait_network_ready for nfs we need to rewrite it as it...

...

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)
  

... directly depends on ssh and requires the ssh key to be propagated. That was a problem I ran into during this cleanup...

Thanks Philipp

...

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

@@ -750,7 +748,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

}

@@ -760,7 +758,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

@@ -777,9 +775,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

-- 2.34.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 on the list, report it: https://pagure.io/fedora-infrastructure