On Fri, 10 Dec 2021 11:16:32 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
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.
Sorry, I totally misinterpreted the code which made me think that the
two options cannot be combined. But after looking at it again I noticed
I was wrong. Sorry for the confusion.
Nevertheless the wording in the paragraph is a little bit confusing.
How about
"--fadump=[on|off|nocma] toggles fadump on/off for the kernel provided
in KERNEL_PATH. If --fadump is omitted the dump mode is determined by
parsing the kernel command line for the kernel to update."
>> 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()
[...]
>
>* 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.
Personally I still think the single loop as sketched below is more
elegant as it simplifies the code significantly. It requires a few more
calls to grubby though. But as the code isn't performance critical I
think that is not a big problem.
Anyway, it's just my personal preference.
Thanks
Philipp
>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}"
>