9:16 p.m.
Hi Philipp,
Thanks for reviewing the patch!
On Thu, Dec 09, 2021 at 06:58:23PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Wed, 8 Dec 2021 10:25:34 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Rewrite kdumpctl reset-crashkernel KERNEL_PATH as
> kdumpctl reset-crashkernel [--fadump=[on|off|nocma]] [--kernel=path_to_kernel]
[--reboot]
>
> This interface would reset a specific kernel to the default crasherknel
> value given the kernel path. And it also supports grubby's syntax so
> there are the following special cases,
> - if --kernel not specified, current running kernel, i.e. `uname -r` is chosen
> - if --kernel=DEFAULT, the default boot kernel is chosen
> - if --kernel=ALL, all kernels would have its crashkernel reset to the default
value
>
> --fadump=[on|off|nocma] would be specified optionally to toggle fadump
> on/off. If it's not specified, the dump mode would be determined based
> on the fadump kernel cmdline parameter read from grubby and for --kernel=ALL,
> all installed kernels would be iterated to tell its dump mode kernel by
> kernel.
Just for my understanding. You cannot combine option --fadump together with
--kernel=ALL. Meaning you can only turn fadump on/off for specific
kernels but not for all. Is that correct? Is that intended?
Sorry for not clear enough. We can combine option --fadump with
--kernel=ALL and this feature comes at no cost thanks to
"grubby --update-kernel=ALL". But if the user doesn't specify the fadump
parameter, we can't make use of "grubby --update-kernel=ALL" because
some kernels could have fadump enabled while some not. In this case, we
should reset crashkernel kernel by kernel.
> This interface will also be called by kexec-tools RPM scriptlets [1]
> to reset crashkernel.
>
> Note the support of crashkenrel.default is dropped.
>
> [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/
>
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> kdumpctl | 113 +++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 89 insertions(+), 24 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 0b8dc0f..26358d2 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1381,41 +1381,105 @@ _get_current_running_kernel_path()
> fi
> }
>
> -reset_crashkernel()
> +_update_grub()
> {
> - local kernel=$1 entry crashkernel_default
> - local grub_etc_default="/etc/default/grub"
> -
> - [[ -z $kernel ]] && kernel=$(uname -r)
> - crashkernel_default=$(cat "/usr/lib/modules/$kernel/crashkernel.default"
2> /dev/null)
> -
> - if [[ -z $crashkernel_default ]]; then
> - derror "$kernel doesn't have a crashkernel.default"
> - exit 1
> - fi
> + local _grubby_kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4
>
> 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"
> - fi
> -
> [[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"
> - grubby --args "$crashkernel_default" --update-kernel "$entry"
$zipl_arg
> + grubby --args "crashkernel=$_crashkernel" --update-kernel
"$_grubby_kernel_path" $zipl_arg
> [[ $zipl_arg ]] && zipl > /dev/null
you need to be careful here. On s390 any change made by grubby won't
take effect until zipl gets called. So ...
Yes, I'm aware of this situation.
> + if [[ $_dump_mode == kdump ]]; then
> + grubby --remove-args="fadump" --update-kernel
"$_grubby_kernel_path"
> + else
> + grubby --args="fadump=$_fadump_val" --update-kernel
"$_grubby_kernel_path"
> + fi
... these changes would be ignored on the next boot (unless the user
calls zipl manually). This doesn't make a difference here as s390
doesn't support fadump. Nevertheless I think the call of zipl should be
moved to to the end.
But moving the call of zipl to the end could make the code more robust
in case someone may modify this part of code in the future. Will fix it
in next version. Thanks!
> fi
> }
>
> +_valid_grubby_kernel_path()
> +{
> + grubby --info="$1" > /dev/null 2>&1
Just noticed that this also returns true for $1 being empty. So this
should be
[[ -n "$1" ]] && grubby ...
Indeed grubby doesn't complain about this case. Thanks for catching this
issue.
> +}
> +
> +_get_all_kernels_from_grubby()
> +{
> + local _kernels _line _kernel_path
> + for _line in $(grubby --info ALL | grep "^kernel="); do
> + _kernel_path=$(_filter_grubby_kernel_str "$_line")
> + _kernels="$_kernels $_kernel_path"
> + done
> + echo -n "$_kernels"
> +}
> +
> +reset_crashkernel()
> +{
> + local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path
_kernel_crashkernel _kernel
> +
> + for _opt in "$@"; do
> + case "$_opt" in
> + --fadump=*)
> + _val=${_opt#*=}
> + if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then
> + _fadump_val=$_val
> + else
> + derror "failed to determine dump mode"
> + exit
> + fi
> + ;;
> + --kernel=*)
> + _val=${_opt#*=}
> + if ! _valid_grubby_kernel_path $_val; then
> + derror "Invalid $_opt, please specify a valid kernel path, ALL or
DEFAULT"
> + exit
> + fi
> + _grubby_kernel_path=$_val
> + ;;
> + --reboot)
> + _reboot=yes
> + ;;
> + *)
> + derror "$_opt not recognized"
> + exit 1
> + ;;
> + esac
> + done
> +
> + if [[ -z $_grubby_kernel_path ]] &&
> + ! _grubby_kernel_path=$(_get_current_running_kernel_path); then
> + derror "no running kernel found"
> + exit 1
> + fi
I see multiple problems with the rest of the function.
> + if [[ -z $_dump_mode ]]; then
> + if [[ $_grubby_kernel_path == ALL ]]; then
> + for _kernel in $(_get_all_kernels_from_grubby); do
> + _dump_mode=$(get_dump_mode_by_kernel "$_kernel")
> + _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
> + _update_grub "$_grubby_kernel_path" "$_kernel_crashkernel"
"$_dump_mode" "$_fadump_val"
* missing indentation
* $_kernel_crashkernel is used uninitialized.
Thanks for catching these two issues. Interestedly, shellcheck don't
catch the latter case.
> + done
> + [[ $_reboot == yes ]] && reboot
> + dwarn "For kernel=$_kernel, crashkernel=$_kernel_crashkernel now. Please
reboot the system to take effect."
* the warning is only printed for the last kernel
* the warning is printed even when nothing changed e.g. if kernel
already had the recommended crashkernel
> + return
> + else
> + _dump_mode=$(get_dump_mode_by_kernel "$_kernel")
> + _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
> + fi
> + fi
> +
> + _kernel_crashkernel=$(kdump_get_arch_recommend_crashkernel
"$_dump_mode")
> + _update_grub "$_grubby_kernel_path" "$_kernel_crashkernel"
"$_dump_mode" "$_fadump_val"
> +
> + [[ $_reboot == yes ]] && reboot
> + dwarn "For kernel=$_grubby_kernel_path, crashkernel=$_kernel_crashkernel now.
Please reboot the system to take effect."
* same like above
Thanks for spotting the issues.
* all in all I don't understand why you don't simply use a single loop
As explained in the beginning, I want to make use of
"grubby --update-kernel= ALL". But when --fadump is not specified, this
needs to be treated as a special case. I'll see if there is a more
elegant solution for this special case.
In my opinion the function should look like this (not tested)
_grubby_get_kernel()
{
# use sed to modify all lines in case $1=ALL
grubby --info="$1" | sed -n -e
's/^kernel="\(.*\)"/\1/p'
}
reset_crashkernel()
{
local _opt _reboot _grubby_kernel_path _kernel
local _dump_mode __dump_mode _fadump_val __fadump_val
local _new_crashkernel _old_crashkernel
_grubby_kernel_path=$(_find_kernel_path_by_release "$(uname -r)")
for _opt in "$@"; do
case "$_opt" in
--fadump=*)
_fadump_val=${_opt#*=}
if ! _dump_mode=$(get_dump_mode_by_fadump_val
$_fadump_val); then
derror "failed to determine dump mode"
exit
fi
;;
--kernel=*)
_grubby_kernel_path=${_opt#*=}
;;
--reboot)
_reboot=yes
;;
*)
derror "$_opt not recognized"
exit 1
;;
esac
done
if ! _valid_grubby_kernel_path $_grubby_kernel_path; then
derror "Invalid --kernel=$_grubby_kernel_path provided."
derror "Please specify a valid kernel path, ALL or DEFAULT"
exit
fi
# in case the two options cannot be combined.
# Error handling is just an example.
if [[ $_grubby_kernel_path == ALL ]] && [[ -n $_dump_mode ]]; then
dwarn "Cannot combine --kernel=ALL with option --fadump"
_dump_mode=""
fi
while read _kernel; do
if [[ -z $_dump_mode ]]; then
__dump_mode=$(get_dump_mode_by_kernel "$_kernel")
__fadump_val=$(get_grub_kernel_boot_parameter "$_kernel"
fadump)
else
__dump_mode=$_dump_mode
__fadump_val=$_fadump_val
fi
_new_crashkernel=$(kdump_get_arch_recommend_crashkernel
"$__dump_mode")
_old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel"
"crashkernel")
[[ $_new_crashkernel == $_old_crashkernel ]] && continue
ret=$(_update_grub "$_grubby_kernel_path"
"$_kernel_crashkernel" "$__dump_mode" "$__fadump_val")
if [[ $ret ]]; then
dwarn "Updated crashkernel=$_kernel_crashkernel for
kernel=$_grubby_kernel_path."
_kernel_updated=1
fi
done < <(_grubby_get_kernel $_grubby_kernel_path)
[[ $_reboot == yes ]] && reboot
[[ $_kernel_updated -eq 1 ]] && dwarn " Please reboot the system for
the updates to take effect."
}
Thanks
Philipp
> +}
> +
> if [[ ! -f $KDUMP_CONFIG_FILE ]]; then
> derror "Error: No kdump config file found!"
> exit 1
> @@ -1478,7 +1542,8 @@ main()
> get_default_crashkernel "$2"
> ;;
> reset-crashkernel)
> - reset_crashkernel "$2"
> + shift
> + reset_crashkernel "$@"
> ;;
> *)
> dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
--
Best regards,
Coiby