Hi Coiby,
On Fri, 19 Nov 2021 11:23:04 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
kumpctl reset-crashkernel [[CRASHKERNEL], [KERNELPATH[,
CRASHKERNEL]]
With no CRASHKERNEL specified, kexec-tools would use the value from
kdump.conf. With valid CRASHKERNEL specified, kexec-tools would also
write the value to kdump.conf.
With no KERNELPATH specified, crashkernel would be set for current
running kernel, i.e. `uname -r`. If KERNELRELEASE=ALL, the same
s/KERNELRELEASE/KERNELPATH/ ?
crashkernel values would be set for all kernels.
This interface will also be called by kernel installation hook
92-crashkernel.install to set crashkernel for a newly installed
kernel.
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdumpctl | 122 ++++++++++++++++++++++++++++++++++++++++++++---------
kdumpctl.8 | 12 +++---
2 files changed, 107 insertions(+), 27 deletions(-)
diff --git a/kdumpctl b/kdumpctl
index 6f1afad..345e112 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1315,41 +1315,123 @@ get_grub_kernel_boot_parameter() {
grubby --info="$_kernel_path" | sed -n
's/^args="\(.*\)"$/\1/p' | sed -n
"s/^.*${_para}=\(\S*\).*/\1/p"
}
-reset_crashkernel()
+_update_crashkernel()
{
- local kernel=$1 entry crashkernel_default
+ local _entry="$1" _crashkernel="$2"
local grub_etc_default="/etc/default/grub"
- [[ -z $kernel ]] && kernel=$(uname -r)
- crashkernel_default=$(cat "/usr/lib/modules/$kernel/crashkernel.default"
2> /dev/null)
This silently removes the support for crashkernel.default again. Please
mention it in the commit message.
-
- if [[ -z $crashkernel_default ]]; then
- derror "$kernel doesn't have a crashkernel.default"
- exit 1
- fi
-
if is_atomic; then
if rpm-ostree kargs | grep -q "crashkernel="; then
- rpm-ostree --replace="crashkernel=$crashkernel_default"
+ rpm-ostree --replace="crashkernel=$_crashkernel"
else
- rpm-ostree --append="crashkernel=$crashkernel_default"
+ rpm-ostree --append="crashkernel=$_crashkernel"
fi
else
- entry=$(grubby --info ALL | grep "^kernel=.*$kernel")
- entry=${entry#kernel=}
- entry=${entry#\"}
- entry=${entry%\"}
-
if [[ -f $grub_etc_default ]]; then
- sed -i -e "s/^\(GRUB_CMDLINE_LINUX=.*\)crashkernel=[^\ \"]*\([\
\"].*\)$/\1$crashkernel_default\2/" "$grub_etc_default"
+ sed -i -e "s/^\(GRUB_CMDLINE_LINUX=.*\)crashkernel=[^\ \"]*\([\
\"].*\)$/\1$_crashkernel\2/" "$grub_etc_default"
Like in patch 5 this regex is frail to partial matches, i.e. it also
matches foo-crashkernel=.
fi
[[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"
- grubby --args "$crashkernel_default" --update-kernel "$entry"
$zipl_arg
+ grubby --args "crashkernel=$_crashkernel" --update-kernel
"$_entry" $zipl_arg
[[ $zipl_arg ]] && zipl > /dev/null
fi
}
+_find_grubby_kernel_path()
+{
+ local _kernel="$1" _kernel_path
+ if [[ $_kernel == ALL || $_kernel == DEFAULT ]]; then
+ _kernel_path=$_kernel
+ else
+ _kernel_path=$(grubby --info ALL | grep "^kernel=.*$_kernel")
+ _kernel_path=${_kernel_path#kernel=}
+ _kernel_path=${_kernel_path#\"}
+ _kernel_path=${_kernel_path%\"}
+ [[ -z "$_kernel_path" ]] && derror "kernel $_kernel doesn't
eixt" && return 1
typo
s/eixt/exist/
in general can you explain, which values $_kernel (which IIUC is
identical to the CRASHKERNEL parameter passed by the user) can have?
The way I understand grubby allows either ALL, DEFAULT,
vmlinuz-<version> or the path to the vmlinuz file. The last case (path
to vmlinuz file) isn't handled correctly here. In particular the path
could also be relative, e.g. ../../boot/vmlinuz-<version>, which would
break the grep. I guess the easiest solution is to add
[[ -e $_kernel ]] && _kernel=$(realpath $_kernel)
at the beginning of the else block. This would also allow symbolic
links to the vmlinuz file, something grubby doesn't support. But I
don't think that is a problem.
+ fi
+ echo -n "$_kernel_path"
+}
+
+# Besides valid kernel path, grubby also accept DEFAULT and ALL
+_valid_grubby_kernel_path()
+{
+ [[ -e $1 ]] || [[ $1 == ALL ]] || [[ $1 == DEFAULT ]]
-e only checks if the file exists but not if it is a valid kernel
grubby can work with. Why not just call grubby and see if it can work
with the provided kernel, i.e.
grubby --info="$1" > /dev/null 2>&1
+}
+
+
+get_dump_mode_of_kernel()
+{
+ local _kernel_path=$1 _fadump_val
+ _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel_path" fadump)
+ if [[ -z $_fadump_val ]] || [[ $_fadump_val == off ]]; then
+ echo -n fadump
+ elif [[ $_fadump_val == on ]] || [[ $_fadump_val == nocma ]]; then
+ echo -n kdump
When $_fadump_val is "on" the dump mode is kdump and when it's
"off"
it's "fadump"? Is that correct? From the naming I would expect the
opposite. If it is correct could you please add a comment with an
explanation. I find that very confusing.
+ else
+ derror "invalid fadump=$_fadump_val"
+ exit
+ fi
+}
+
+# determine which parameter is kernel_path and crashkernel
+# return kernel_path;crashkernel
+_determine_kernel_path_and_crashkernel()
+{
+ local _grub_kernel_path _crashkernel
+ if [[ -n "$1" ]] && [[ -n "$2" ]]; then
+ _grub_kernel_path=$(_find_grubby_kernel_path "$1") || return 1
+ if _valid_grubby_kernel_path "$1"; then
+ _grub_kernel_path=$1
+ else
+ derror "kernel $1 doesn't exist"
+ exit 1
+ fi
+ _crashkernel="$2"
+ elif [[ -n "$1" ]]; then
+ if _valid_grubby_kernel_path "$1"; then
+ _grub_kernel_path=$1
+ else
+ _crashkernel="$1"
+ fi
+ fi
+
+ [[ -z "$_grub_kernel_path" ]] &&
_grub_kernel_path=$(_find_grubby_kernel_path "$(uname -r)")
+
+ echo -n "${_grub_kernel_path};${_crashkernel}"
Introducing this special notation isn't really nice. You could simplify
it by simply
echo $_grub_kernel_path $_crashkernel
and then ...
+}
+
+reset_crashkernel()
+{
+ local _crashkernel _kernel_crashkernel _grub_kernel_path _tmp_str _dump_mode
+
+ if _tmp_str=$(_determine_kernel_path_and_crashkernel "$1" "$2");
then
+ _grub_kernel_path=$(cut -d ";" -f 1 <<< "$_tmp_str")
+ _crashkernel=$(cut -d ";" -f 2 <<< "$_tmp_str")
+ else
+ exit 1
+ fi
... change this to
read _grub_kernel_path _crashkernel < <(_determine_kernel_path_and_crashkernel
"$1" "$2")
This also removes the superfluous error check above. The way I see it
_determine_kernel_path_and_crashkernel will either exit directly or at
least return ";". So $tmp_str will always evaluate as true and the the
else block will never be reached.
+
+ if [[ -z "$_crashkernel" ]]; then
+ _crashkernel=$(kdump_get_conf_val crashkernel)
The -z _grub_kernel_path is handled in
_determine_kernel_path_and_crashkernel. Wouldn't it make sense to
handle both -z cases in the same function?
+ else
+ kdump_write_conf_val crashkernel "$_crashkernel"
+ fi
As said in an earlier mail I wouldn't silently update kdump.conf with
the new value. Imagine the case
1) a user runs the standard kernel, has kdump setup and everything
works fine
2) the user installs kernel-debug and wants to increase the crashkernel
memory for this kernel
3) with this kdumpctl updates kdump.conf so that every other standard
kernel will use the larger crashkernel memory for the debug kernel
unless the user manually resets the value back to the original value.
That's not the behavior I would expect.
Thanks
Philipp
+ if [[ $_grub_kernel_path == ALL ]]; then
+ _dump_mode="$DEFAULT_DUMP_MODE"
+ else
+ _dump_mode=$(get_dump_mode_of_kernel "$_grub_kernel_path")
+ fi
+
+ if [[ $_crashkernel == "auto" ]] ; then
+ _kernel_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")
+ else
+ _kernel_crashkernel="$_crashkernel"
+ fi
+
+ _update_crashkernel "$_grub_kernel_path" "$_kernel_crashkernel"
+}
+
if [[ ! -f $KDUMP_CONFIG_FILE ]]; then
derror "Error: No kdump config file found!"
exit 1
@@ -1412,7 +1494,7 @@ main()
get_default_crashkernel "$2"
;;
reset-crashkernel)
- reset_crashkernel "$2"
+ reset_crashkernel "$2" "$3"
;;
*)
dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
diff --git a/kdumpctl.8 b/kdumpctl.8
index 74be062..19734aa 100644
--- a/kdumpctl.8
+++ b/kdumpctl.8
@@ -50,13 +50,11 @@ Estimate a suitable crashkernel value for current machine. This is a
best-effort estimate. It will print a recommanded crashkernel value
based on current kdump setup, and list some details of memory usage.
.TP
-.I reset-crashkernel [KERNEL]
-Reset crashkernel value to default value. kdumpctl will try to read
-from /usr/lib/modules/<KERNEL>/crashkernel.default and reset specified
-kernel's crashkernel cmdline value. If no kernel is
-specified, will reset current running kernel's crashkernel value.
-If /usr/lib/modules/<KERNEL>/crashkernel.default doesn't exist, will
-simply exit return 1.
+.I reset-crashkernel [[CRASHKERNEL], [KERNELPATH[, CRASHKERNEL]]
+Reset crashkernel value to CRASHKERNEL or the value specified in
+/etc/kdump.conf. If no kernel is specified, will reset current running
+kernel's crashkernel value. If CRASHKERNEL is given, the value would be also
+writeen to /etc/kdump.conf.
.SH "SEE ALSO"