kdump.sh: Skip num-threads when -E option presents #125
Conversation
Summary of ChangesHello @pfliu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the kdump process where Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a failure in kdump.sh where --num-threads was incorrectly added to makedumpfile when the ELF format (-E) was specified. The change prevents this by checking for the -E option before adding --num-threads.
My review includes a suggestion to make the implementation POSIX-compliant, as required by the script's own documentation. The current change uses a bash-specific construct which could cause issues on some systems.
Additionally, I've noticed that the same potential issue exists in the dump_raw function (lines 389-391 in kdump.sh). It would be beneficial to apply a similar fix there to ensure raw dumps with -E specified also work correctly.
dracut/99kdumpbase/kdump.sh
Outdated
| CORE_COLLECTOR=$(echo "$CORE_COLLECTOR" | sed -e "s/-F//g") | ||
| THREADS=$(nproc) | ||
| if [ "$THREADS" -gt 1 ]; then | ||
| if [ "$THREADS" -gt 1 ] && [[ "$CORE_COLLECTOR" != *"-E"* ]]; then |
There was a problem hiding this comment.
The use of [[...]] is not POSIX-compliant, but this script is expected to be POSIX-compliant as stated in the file's header comment. Using [[...]] can lead to issues on systems where /bin/sh is not bash.
A more portable approach that is consistent with other parts of this file is to use grep to check for the presence of the -E option.
| if [ "$THREADS" -gt 1 ] && [[ "$CORE_COLLECTOR" != *"-E"* ]]; then | |
| if [ "$THREADS" -gt 1 ] && ! echo "$CORE_COLLECTOR" | grep -q -- '-E'; then |
|
Hi @pfliu Gemini also notices that "the same potential issue exists in the dump_raw function (lines 389-391 in kdump.sh)". I think this is a valid concern, right? |
pfliu
force-pushed
the
elf_fix
branch
3 times, most recently
from
8b918fb to
190bf7f
Compare
|
as mentioned in #117, I find the current handling of @pfliu could you do this cleanup as well while fixing the issue? I'd appreciate it. Thanks |
|
Hi @prudo1 Thanks for sharing your feedback! I also agree addressing num-thread early in |
@prudo1, I have pushed an update version, which make the code clean. Please help to review. Thanks |
dracut/99kdumpbase/kdump.sh
Outdated
| case $CORE_COLLECTOR in | ||
| *makedumpfile*) | ||
| THREADS=$(nproc) | ||
| if [ "$THREADS" -gt 1 ]; then | ||
| CORE_COLLECTOR="$CORE_COLLECTOR --num-threads=$THREADS" | ||
| fi | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Hi @pfliu,
thanks for the update! A few nits I found with your code
- The hunk should be moved to the end of the function. The way it is implemented right now the option is only added when
core_collectoris set inkdump.confbut not when the default value is used. - The flattened dump format also doesn't support
--num-threads=. So you have to prevent setting the option when-Fis set as well. - Ideally the
CORE_COLLECTORis no longer changed afterget_kdump_confs(). So...
dracut/99kdumpbase/kdump.sh
Outdated
| @@ -149,10 +157,6 @@ dump_fs() { | |||
| case $CORE_COLLECTOR in | |||
| *makedumpfile*) | |||
| CORE_COLLECTOR=$(echo "$CORE_COLLECTOR" | sed -e "s/-F//g") | |||
There was a problem hiding this comment.
... this line should be moved to get_kdump_confs() as well.
All in all, I would implement it so the code looks something like this in the end.
Note: The diff is meant as an example. I haven't done any testing on it.
diff --git a/dracut/99kdumpbase/kdump.sh b/dracut/99kdumpbase/kdump.sh
index 0ec1c3c..1c8ec85 100755
--- a/dracut/99kdumpbase/kdump.sh
+++ b/dracut/99kdumpbase/kdump.sh
@@ -22,8 +22,7 @@ KDUMP_LOG_DEST=""
KDUMP_LOG_OP=""
KDUMP_TEST_ID=""
KDUMP_TEST_STATUS=""
-CORE_COLLECTOR=""
-DEFAULT_CORE_COLLECTOR="makedumpfile -l --message-level 7 -d 31"
+CORE_COLLECTOR="makedumpfile -l --message-level 7 -d 31"
DMESG_COLLECTOR="/sbin/vmcore-dmesg"
FAILURE_ACTION="systemctl reboot -f"
DATEDIR=$(date +%Y-%m-%d-%T)
@@ -55,20 +54,6 @@ get_kdump_confs() {
;;
core_collector)
[ -n "$config_val" ] && CORE_COLLECTOR="$config_val"
- case $CORE_COLLECTOR in
- *makedumpfile*)
- THREADS=$(nproc)
- if [ "$THREADS" -gt 1 ]; then
- case "$CORE_COLLECTOR" in
- *-E*) ;;
-
- *)
- CORE_COLLECTOR="$CORE_COLLECTOR --num-threads=$THREADS"
- ;;
- esac
- fi
- ;;
- esac
;;
sshkey)
if [ -f "$config_val" ]; then
@@ -122,12 +107,37 @@ get_kdump_confs() {
esac
done < "$KDUMP_CONF_PARSED"
- if [ -z "$CORE_COLLECTOR" ]; then
- CORE_COLLECTOR="$DEFAULT_CORE_COLLECTOR"
- if is_ssh_dump_target || is_raw_dump_target; then
- CORE_COLLECTOR="$CORE_COLLECTOR -F"
- fi
- fi
+ case $CORE_COLLECTOR in
+ *makedumpfile*)
+ # make sure the flattened dump format is used for ssh and raw
+ # targets but not for fs targets.
+ if is_ssh_dump_target || is_raw_dump_target; then
+ case "$CORE_COLLECTOR" in
+ *-F*) ;;
+ *)
+ dwarn "When dumping to ssh/raw targets the flattened dump format must be used. Adding option -F to makedumpfile."
+ CORE_COLLECTOR="$CORE_COLLECTOR -F"
+ ;;
+ esac
+ else
+ CORE_COLLECTOR=$(echo "$CORE_COLLECTOR" | sed -e "s/-F//g")
+ fi
+
+ case "$CORE_COLLECTOR" in
+ *-F*|*-E*)
+ # --num-threads= is only supported by the kdump format
+ ;;
+ *)
+ THREADS=$(nproc)
+ if [ "$THREADS" -gt 1 ]; then
+ CORE_COLLECTOR="$CORE_COLLECTOR --num-threads=$THREADS"
+ fi
+ ;;
+ esac
+ ;;
+
+ *) ;;
+ esac
}
# store the kexec kernel log to a file.
@@ -159,13 +169,6 @@ dump_fs() {
fi
fi
- # Remove -F in makedumpfile case. We don't want a flat format dump here.
- case $CORE_COLLECTOR in
- *makedumpfile*)
- CORE_COLLECTOR=$(echo "$CORE_COLLECTOR" | sed -e "s/-F//g")
- ;;
- esac
-
if [ -z "$KDUMP_TEST_ID" ]; then
_dump_fs_path=$(echo "$1/$KDUMP_PATH/$HOST_IP-$DATEDIR/" | tr -s /)
else
diff --git a/mkdumprd b/mkdumprd
index caca83b..a8a5c8d 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -288,10 +288,13 @@ verify_core_collector()
fi
if is_ssh_dump_target || is_raw_dump_target; then
- if ! strstr "$_params" "-F"; then
- perror_exit 'The specified dump target needs makedumpfile "-F" option.'
+ # temporarily add -F option, if needed. It will be added for
+ # automatically in kdump.sh.
+ if [[ " $_params " != *" -F "* ]]; then
+ _params="$_params -F vmcore"
+ else
+ _params="$_params vmcore"
fi
- _params="$_params vmcore"
else
_params="$_params vmcore dumpfile"
fi
That should also allow to remove the error in mkdumprd when the -F option is missing for ssh/raw dump targets, which IMHO is a big improvement (I always forget to add -F when I try to dump via ssh...).
Thanks
Philipp
There was a problem hiding this comment.
Thanks for your suggestion. I have re-organize the patch into three
Signed-off-by: Pingfan Liu <piliu@redhat.com>
The handling of num-threads is duplicated in dump_fs() and dump_raw(). Centralize them into get_kdump_confs() Signed-off-by: Pingfan Liu <piliu@redhat.com>
Resolves: https://issues.redhat.com/browse/RHEL-75537 Configure "makedumpfile -E -d 31" in kdump.conf, and panic the system, kdump will fail with the error as: [ 41.891856] kdump.sh[724]: --num-threads cannot used with ELF format. [ 41.897104] kdump.sh[724]: Commandline parameter is invalid. [ 41.897290] kdump.sh[724]: Try `makedumpfile --help' for more information. [ 41.897435] kdump.sh[724]: makedumpfile Failed. [ 41.898804] kdump[726]: saving vmcore failed, exitcode:1 Skip --num-threads when -E option is given. As for the -F option, it is meaningless to sequentially access linear addresses with multiple threads Skip --num-threads too in that case. Signed-off-by: Pingfan Liu <piliu@redhat.com>
prudo1
left a comment
prudo1
left a comment
There was a problem hiding this comment.
Hi @pfliu,
personally, I'd really love the check for -F in mkdumprd to be gone. But that is only because I always forget to set it when I test a ssh target and it buggers me that I've forgot it, again...
Anyway, while getting rid of the check would be nice I'm afraid that's something that can wait. In general I think it would be good to reconsider how the config is handled so we don't have to parse it over and over again and make changes to it at different places. But that is a bigger task and way out of scope for this PR. Plus the PR is a big step in the right direction.
Having that said, looks good to me!
Thanks
Philipp
Thank you very much. Your suggestion makes the code cleaner and easy to maintain |
3 participants
Resolves: https://issues.redhat.com/browse/RHEL-75537
Configure "makedumpfile -E -d 31" in kdump.conf, and panic the system, kdump will fail with the error as:
[ 41.891856] kdump.sh[724]: --num-threads cannot used with ELF format.
[ 41.897104] kdump.sh[724]: Commandline parameter is invalid.
[ 41.897290] kdump.sh[724]: Try `makedumpfile --help' for more information.
[ 41.897435] kdump.sh[724]: makedumpfile Failed.
[ 41.898804] kdump[726]:
saving vmcore failed, exitcode:1
Skip --num-threads when -E option is given