Resolves: bz1449801
"cat $KDUMP_CONFIG_FILE|grep -v "^#"|while read ..." use pipes to invoke subshells, as a result we met "the dreaded inaccessible variables within a subshell problem" as described in the book "Advanced Bash-Scripting Guide".
It cause regressions, so revert it. --- kdumpctl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 46b65d2..644cf96 100755 --- a/kdumpctl +++ b/kdumpctl @@ -365,11 +365,11 @@ check_config() return 1 }
- cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do + while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in - "") + #* | "") ;; raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes) [ -z "$config_val" ] && { @@ -386,7 +386,7 @@ check_config() return 1; ;; esac - done + done < $KDUMP_CONFIG_FILE
check_default_config || return 1
@@ -799,7 +799,7 @@ load_kdump()
check_ssh_config() { - cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do + while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in @@ -820,7 +820,7 @@ check_ssh_config() *) ;; esac - done + done < $KDUMP_CONFIG_FILE
#make sure they've configured kdump.conf for ssh dumps local SSH_TARGET=`echo -n $DUMP_TARGET | sed -n '/.*@/p'`
The "time kdumpctl start" command shows that strip_comments() consumes lots of cpu time. By only calling it when necessary, it saves us nearly half second.
Tested on my Fedora kvm machine. Before this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.849s user 0m1.497s sys 0m0.462s
After this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.344s user 0m1.195s sys 0m0.195s
Signed-off-by: Xunlei Pang xlpang@redhat.com --- kdumpctl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 644cf96..4d3f053 100755 --- a/kdumpctl +++ b/kdumpctl @@ -366,12 +366,12 @@ check_config() }
while read config_opt config_val; do - # remove inline comments after the end of a directive. - config_val=$(strip_comments $config_val) case "$config_opt" in #* | "") ;; raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes) + # remove inline comments after the end of a directive. + config_val=$(strip_comments $config_val) [ -z "$config_val" ] && { echo "Invalid kdump config value for option $config_opt." return 1; @@ -800,10 +800,10 @@ load_kdump() check_ssh_config() { while read config_opt config_val; do - # remove inline comments after the end of a directive. - config_val=$(strip_comments $config_val) case "$config_opt" in sshkey) + # remove inline comments after the end of a directive. + config_val=$(strip_comments $config_val) if [ -f "$config_val" ]; then # canonicalize the path SSH_KEY_LOCATION=$(/usr/bin/readlink -m $config_val) @@ -812,9 +812,11 @@ check_ssh_config() fi ;; path) + config_val=$(strip_comments $config_val) SAVE_PATH=$config_val ;; ssh) + config_val=$(strip_comments $config_val) DUMP_TARGET=$config_val ;; *)
On 05/11/17 at 01:44pm, Xunlei Pang wrote:
The "time kdumpctl start" command shows that strip_comments() consumes lots of cpu time. By only calling it when necessary, it saves us nearly half second.
Tested on my Fedora kvm machine. Before this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.849s user 0m1.497s sys 0m0.462s
After this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.344s user 0m1.195s sys 0m0.195s
Xunlei, do you have test results with comparing with previous one before the revert?
Signed-off-by: Xunlei Pang xlpang@redhat.com
kdumpctl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 644cf96..4d3f053 100755 --- a/kdumpctl +++ b/kdumpctl @@ -366,12 +366,12 @@ check_config() }
while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in #* | "") ;; raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) [ -z "$config_val" ] && { echo "Invalid kdump config value for option $config_opt." return 1;
@@ -800,10 +800,10 @@ load_kdump() check_ssh_config() { while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in sshkey)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) if [ -f "$config_val" ]; then # canonicalize the path SSH_KEY_LOCATION=$(/usr/bin/readlink -m $config_val)
@@ -812,9 +812,11 @@ check_ssh_config() fi ;; path)
ssh)config_val=$(strip_comments $config_val) SAVE_PATH=$config_val ;;
*)config_val=$(strip_comments $config_val) DUMP_TARGET=$config_val ;;
-- 1.8.3.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
On 05/12/2017 at 09:23 AM, Dave Young wrote:
On 05/11/17 at 01:44pm, Xunlei Pang wrote:
The "time kdumpctl start" command shows that strip_comments() consumes lots of cpu time. By only calling it when necessary, it saves us nearly half second.
Tested on my Fedora kvm machine. Before this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.849s user 0m1.497s sys 0m0.462s
After this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.344s user 0m1.195s sys 0m0.195s
Xunlei, do you have test results with comparing with previous one before the revert?
It's similar: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.352s user 0m1.215s sys 0m0.193s
Regards, Xunlei
Signed-off-by: Xunlei Pang xlpang@redhat.com
kdumpctl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 644cf96..4d3f053 100755 --- a/kdumpctl +++ b/kdumpctl @@ -366,12 +366,12 @@ check_config() }
while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in #* | "") ;; raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) [ -z "$config_val" ] && { echo "Invalid kdump config value for option $config_opt." return 1;
@@ -800,10 +800,10 @@ load_kdump() check_ssh_config() { while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in sshkey)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) if [ -f "$config_val" ]; then # canonicalize the path SSH_KEY_LOCATION=$(/usr/bin/readlink -m $config_val)
@@ -812,9 +812,11 @@ check_ssh_config() fi ;; path)
ssh)config_val=$(strip_comments $config_val) SAVE_PATH=$config_val ;;
*)config_val=$(strip_comments $config_val) DUMP_TARGET=$config_val ;;
-- 1.8.3.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
On 05/12/17 at 09:40am, Xunlei Pang wrote:
On 05/12/2017 at 09:23 AM, Dave Young wrote:
On 05/11/17 at 01:44pm, Xunlei Pang wrote:
The "time kdumpctl start" command shows that strip_comments() consumes lots of cpu time. By only calling it when necessary, it saves us nearly half second.
Tested on my Fedora kvm machine. Before this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.849s user 0m1.497s sys 0m0.462s
After this patch: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.344s user 0m1.195s sys 0m0.195s
Xunlei, do you have test results with comparing with previous one before the revert?
It's similar: $ time kdumpctl start kexec: loaded kdump kernel Starting kdump: [OK]
real 0m1.352s user 0m1.215s sys 0m0.193s
Thanks for the data, it looks good, per discussion in irc. I think the two patches are fine.
Acked-by: Dave Young dyoung@redhat.com
Thanks Dave
Regards, Xunlei
Signed-off-by: Xunlei Pang xlpang@redhat.com
kdumpctl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 644cf96..4d3f053 100755 --- a/kdumpctl +++ b/kdumpctl @@ -366,12 +366,12 @@ check_config() }
while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in #* | "") ;; raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) [ -z "$config_val" ] && { echo "Invalid kdump config value for option $config_opt." return 1;
@@ -800,10 +800,10 @@ load_kdump() check_ssh_config() { while read config_opt config_val; do
# remove inline comments after the end of a directive.
case "$config_opt" in sshkey)config_val=$(strip_comments $config_val)
# remove inline comments after the end of a directive.
config_val=$(strip_comments $config_val) if [ -f "$config_val" ]; then # canonicalize the path SSH_KEY_LOCATION=$(/usr/bin/readlink -m $config_val)
@@ -812,9 +812,11 @@ check_ssh_config() fi ;; path)
ssh)config_val=$(strip_comments $config_val) SAVE_PATH=$config_val ;;
*)config_val=$(strip_comments $config_val) DUMP_TARGET=$config_val ;;
-- 1.8.3.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
On 05/11/17 at 01:44pm, Xunlei Pang wrote:
Resolves: bz1449801
"cat $KDUMP_CONFIG_FILE|grep -v "^#"|while read ..." use pipes to invoke subshells, as a result we met "the dreaded inaccessible variables within a subshell problem" as described in the book "Advanced Bash-Scripting Guide".
Is it worth to add a tmp file which dropped comment and blank lines? Seems in mkdumprd there is also a while loop to parse kdump.conf
It cause regressions, so revert it.
kdumpctl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 46b65d2..644cf96 100755 --- a/kdumpctl +++ b/kdumpctl @@ -365,11 +365,11 @@ check_config() return 1 }
- cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do
- while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in
"")
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes) [ -z "$config_val" ] && {\#* | "") ;;
@@ -386,7 +386,7 @@ check_config() return 1; ;; esac
- done
done < $KDUMP_CONFIG_FILE
check_default_config || return 1
@@ -799,7 +799,7 @@ load_kdump()
check_ssh_config() {
- cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do
- while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in
@@ -820,7 +820,7 @@ check_ssh_config() *) ;; esac
- done
done < $KDUMP_CONFIG_FILE
#make sure they've configured kdump.conf for ssh dumps local SSH_TARGET=`echo -n $DUMP_TARGET | sed -n '/.*@/p'`
-- 1.8.3.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
On 05/12/17 at 09:27am, Dave Young wrote:
On 05/11/17 at 01:44pm, Xunlei Pang wrote:
Resolves: bz1449801
"cat $KDUMP_CONFIG_FILE|grep -v "^#"|while read ..." use pipes to invoke subshells, as a result we met "the dreaded inaccessible variables within a subshell problem" as described in the book "Advanced Bash-Scripting Guide".
Is it worth to add a tmp file which dropped comment and blank lines? Seems in mkdumprd there is also a while loop to parse kdump.conf
found some scripts below can strip blank lines and comments (include the comments after valid options in a line): grep -o '^[^#]*' a.conf|grep -v '^\s*(#|$)'
But not sure how to make these two greps in one grep.
It cause regressions, so revert it.
kdumpctl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 46b65d2..644cf96 100755 --- a/kdumpctl +++ b/kdumpctl @@ -365,11 +365,11 @@ check_config() return 1 }
- cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do
- while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in
"")
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes) [ -z "$config_val" ] && {\#* | "") ;;
@@ -386,7 +386,7 @@ check_config() return 1; ;; esac
- done
done < $KDUMP_CONFIG_FILE
check_default_config || return 1
@@ -799,7 +799,7 @@ load_kdump()
check_ssh_config() {
- cat $KDUMP_CONFIG_FILE | grep -v "^#" | while read config_opt config_val; do
- while read config_opt config_val; do # remove inline comments after the end of a directive. config_val=$(strip_comments $config_val) case "$config_opt" in
@@ -820,7 +820,7 @@ check_ssh_config() *) ;; esac
- done
done < $KDUMP_CONFIG_FILE
#make sure they've configured kdump.conf for ssh dumps local SSH_TARGET=`echo -n $DUMP_TARGET | sed -n '/.*@/p'`
-- 1.8.3.1 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org