Hi Coiby,
On Fri, 3 Dec 2021 18:18:55 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
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:
>
[...]
>> -
>> - 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.
That's even better :)
>> 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!
np, thanks for taking care of the problem!
>
>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.
In my test on F35 it works...
# grubby --info=vmlinuz-$(uname -r)
index=0
kernel="/boot/vmlinuz-5.15.5-200.fc35.x86_64"
args="ro rd.lvm.lv=fedora_fedora/root console=ttyS0 crashkernel=256M"
root="/dev/mapper/fedora_fedora-root"
initrd="/boot/initramfs-5.15.5-200.fc35.x86_64.img"
title="Fedora Linux (5.15.5-200.fc35.x86_64) 35 (Server Edition)"
id="ed32f6182bc44595983ddbc6ed06bc92-5.15.5-200.fc35.x86_64"
# pwd && grubby --info=../boot/vmlinuz-$(uname -r)
/root
index=0
kernel="/boot/vmlinuz-5.15.5-200.fc35.x86_64"
args="ro rd.lvm.lv=fedora_fedora/root console=ttyS0 crashkernel=256M"
root="/dev/mapper/fedora_fedora-root"
initrd="/boot/initramfs-5.15.5-200.fc35.x86_64.img"
title="Fedora Linux (5.15.5-200.fc35.x86_64) 35 (Server Edition)"
id="ed32f6182bc44595983ddbc6ed06bc92-5.15.5-200.fc35.x86_64"
# ls -l kernel && grubby --info=kernel
lrwxrwxrwx. 1 root root 36 Dec 6 15:47 kernel -> /boot/vmlinuz-5.15.5-200.fc35.x86_64
The param kernel is incorrect
>> + 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,
yes, not being able to return two values reliably is a real pain in
bash. But you are right, my proposal wouldn't handle situations
properly, when _grub_kernel_path and/or _crashkernel are empty.
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.
I'm looking forward to see your v2.
>> +
>> + 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.
np, it's just a minor nit anyway.
>> + 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!
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"
>