Hi Tao,
On Thu, 28 Sep 2023 21:20:28 +0800
Tao Liu <ltao(a)redhat.com> wrote:
The kdumpctl test involves the following procedures:
1) Output the current kdump config to console, including crash kernel size
and effective fields of kdump.conf.
2) A countdown function, to wait for the crash & dump confirmation from user.
3) Generating a timestamp as the id for the current kdump test. The id will
be in kexec-dmesg.log for kdump or vmcore-dmesg.txt for fadump, and also
in KDUMP_TEST_ENV file. kdumpctl test-check will get the test id from
KDUMP_TEST_ENV file, and check if the test id exists in kexec-dmesg.log
or vmcore-dmesg.txt.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
kdumpctl | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 140 insertions(+), 1 deletion(-)
diff --git a/kdumpctl b/kdumpctl
index 6e4685f..b54b5ae 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -16,6 +16,8 @@ KDUMP_INITRD=""
TARGET_INITRD=""
#kdump shall be the default dump mode
DEFAULT_DUMP_MODE="kdump"
+KDUMP_TEST_ENV="/var/kdump/.kdump_test"
+KDUMP_TEST_PREFIX="kdump_test_"
standard_kexec_args="-d -p"
@@ -1073,6 +1075,10 @@ reload()
dwarn "Kdump was not running: [WARNING]"
fi
+ if [[ -f "$KDUMP_TEST_ENV" ]]; then
+ . "$KDUMP_TEST_ENV"
+ fi
Sorry, but I'm strongly against this. You are executing unconditionally
a file with well known name. Throw in a (accidental) problem with file
permissions and you have a vulnerability that allows execution of
arbitrary code as root!
+
if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
reload_fadump
return
@@ -1665,6 +1671,136 @@ reset_crashkernel_for_installed_kernel()
_update_crashkernel "$_installed_kernel"
}
+countdown()
+{
+ local _sec=$1 _msg=$2
+
+ while [[ $_sec -gt 0 ]]; do
+ printf "\r$_msg %ds " "$_sec"
+ sleep 1
+ _sec=$((_sec - 1))
+ done
+}
+
+kill_countdown()
+{
+ local _pid=$1
+
+ kill "$_pid" 2>/dev/null
+ if [[ -n "$2" ]]; then
+ echo -e "$2"
+ fi
+ return 0
+}
+
+output_conf()
+{
+ local -n conf=$1
+ local _width _max_width=0 _line_width _elem _key
+
+ for _elem in "${!conf[@]}"; do
+ _width=${#_elem}
+ if (( _width > _max_width )); then
+ _max_width=$_width
+ fi
+ done
+ _line_width=$_max_width
+ _max_width=0
+ for _elem in "${conf[@]}"; do
+ _width=${#_elem}
+ if (( _width > _max_width )); then
+ _max_width=$_width
+ fi
+ done
+ _line_width=$((_line_width + _max_width))
+
+ echo -e "\nCurrent kdump configuration:"
+ printf -- '-%.0s' $(seq -s " " $((_line_width + 1))); echo
+ for _key in "${!conf[@]}"; do
+ printf "%-$(( _line_width - _max_width ))s %s\n" "$_key"
"${conf[$_key]}"
+ done
+ printf -- '-%.0s' $(seq -s " " $((_line_width + 1))); echo
+}
+
+get_all_kdump_conf()
+{
+ local _max_width=0 _width _elem _line_width _key _func=$1
Looks like most of the variables were move to output_conf. You should
clean them up.
+ declare -A kdump_conf
kdump_conf should be local as well.
+ parse_config || return
+
+ for _elem in "${!OPT[@]}"; do
+ kdump_conf["$_elem"]="${OPT[$_elem]}"
+ done
+
+ if [[ "$DEFAULT_DUMP_MODE" == "kdump" ||
"$DEFAULT_DUMP_MODE" == "fadump" ]]; then
+ kdump_conf["crashkernel_size"]="$(($(get_reserved_mem_size) / 1024 /
1024))M"
+ kdump_conf["dump_mode"]="$DEFAULT_DUMP_MODE"
+ else
+ return
+ fi
+
+ if [ -n "$_func" ]; then
+ $_func kdump_conf
+ fi
What's the point of this function when no _func is provided? The way I
see it then turns in a very complicated call to parse_config. But that
should probably be done explicitly at the beginning of kdump_do_test as
you will access $OPT later anyways.
+}
+
+kdump_do_test()
+{
+ local _input _countdown_pid
+ local _TIMEOUT=10 _TEST_ID="$KDUMP_TEST_PREFIX""$(date
+%Y-%m-%d-%T)"
+
+ is_kernel_loaded "$DEFAULT_DUMP_MODE"
+ case "$?" in
+ 0)
+ get_all_kdump_conf output_conf
+ countdown "$_TIMEOUT" "Will test kdump based on the config, a crash
dump & reboot will be performed, proceed? (yes/[no]):" &
+ _countdown_pid="$!"
+
+ read -t "$_TIMEOUT" _input
+
+ if [[ -z "$_input" ]]; then
+ kill_countdown "$_countdown_pid" "\nKdump test aborted."
&& exit 0
+ elif [[ "$_input" == "no" ]]; then
+ kill_countdown "$_countdown_pid" "Kdump test aborted." &&
exit 0
+ elif [[ "$_input" == "yes" ]]; then
+ kill_countdown "$_countdown_pid"
+ countdown 2 "After machine restart, input \"kdumpctl test-check\" to
get the test result."
+ else
+ kill_countdown "$_countdown_pid" "Please input yes or no."
&& exit 0
+ fi
+ wait
Is it really worth having this countdown? It adds quite a bit of
overhead. In fact I don't understand fully why there is a timeout in
the first place. I simply don't see any benefit it provides.
On a side note, I would really love that there is a more consistent way
in handling user input. In kdump.conf for example
auto_reset_crashkernel also uses yes|no while force_rebuild uses 1|0.
That's not really a problem with your code but still something we
should keep in mind.
+ ;;
+ 1)
+ dinfo "Kdump should be enabled before testing"
+ exit 1
+ ;;
+ esac
The switch case is identical to
if !is_kernel_loaded "$DEFAULT_DUMP_MODE"; then
dinfo "Kdump should be enabled before testing"
exit 1
fi
get_all_kdump_conf output_conf
[...]
with the latter being much better readable.
+
+ [[ ! -d $(dirname "$KDUMP_TEST_ENV") ]] && mkdir -p $(dirname
"$KDUMP_TEST_ENV")
+ echo "KDUMP_COMMANDLINE_APPEND+=\" $_TEST_ID \"" >
"$KDUMP_TEST_ENV"
+ if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
+ echo "$_TEST_ID" > /dev/kmsg
+ fi
+ sync
+
check_and_wait_network_ready
+ setup_initrd || return 1
+ need_initrd_rebuild
+ case "$?" in
+ 0)
+ # Nothing to do
+ ;;
+ 1)
+ rebuild_initrd || return
+ ;;
+ *)
+ return
+ ;;
+ esac
+ reload
Thanks
Philipp
+ echo c > /proc/sysrq-trigger
+}
+
main()
{
# Determine if the dump mode is kdump or fadump
@@ -1740,8 +1876,11 @@ main()
reset_crashkernel_for_installed_kernel "$2"
fi
;;
+ test)
+ kdump_do_test
+ ;;
*)
- dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
+ dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem|test|test-check}"
exit 1
;;
esac