Hi Philipp,
On Wed, Dec 01, 2021 at 04:04:11PM +0100, Philipp Rudo wrote:
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/ ?
Thanks for spotting the typo.
> 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.
Sure, thanks for the reminder!
> -
> - 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=.
Taking the case of setting debug kernel's crashkernel value into
consideration, I think it's better to drop it. Another reason is
grubby would "update the GRUB_CMDLINE_LINUX variable in
/etc/default/grub when the --update-kernel option is used with
the ALL argument". So there is no need to fix the regex issue in v2.
> 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/
Thanks for pointing out the typo!
in general can you explain, which values $_kernel (which IIUC is
identical to the CRASHKERNEL parameter passed by the user) can have?
Sure, I'll add this part in the commit msg next time.
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.
It seems grubby don't accept vmlinuz-<version> or relative path ot the
vmlinux file,
$ sudo grubby --info=vmlinuz-`uname -r`
The param vmlinuz-5.14.14-200.fc34.x86_64 is incorrect
$ sudo grubby --info=./vmlinuz-5.14.14-200.fc34.x86_64
The param ./vmlinuz-5.14.14-200.fc34.x86_64 is incorrect
So we don't need to take care these two cases.
> + 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
Thanks for a better solution!
> +}
> +
> +
> +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.
I designed this by purpose since bash couldn't return two values (in
retrospect, an array seems to be a better idea). Because both
_grub_kernel_path and _crashkernel could be empty. So if
_grub_kernel_path is empty and _crashkernel is not, the following
way won't work,
read _grub_kernel_path _crashkernel < <(_determine_kernel_path_and_crashkernel
"$1" "$2")
But in next version, the interface would be implemented as,
kdumpctl reset-crashkernel [--fadump=[on|off|nocma]] [--kernel=path_to_kernel]
[--reboot]
and "kumpctl fadump" is not needed. So there is no need to abstract
_determine_kernel_path_and_crashkernel.
> +
> + 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?
Thanks for the suggestion. Unfortunately, this suggestion couldn't be
applied to v2 since the implementation has changed as mentioned before.
> + 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 for the suggestion! There is no updating kdump.conf in v2.
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"
--
Best regards,
Coiby