Hi everybody,
this series got spit from the CLI rework series as they are independent and can be merged earlier to reduce the overall series size. While most of the patches were part of the rfc v1 of the CLI rework series patches 9-12 are new.
@Pingfan: I've dropped your Reviewed-by from patch 1 as I've added two more fixes to the patch. But I've kept it for patch 2&4 where I've updated the commit message and patch 3 where I've dropped the definition of _is_valid_kver as after the rebase an identical function already existed. Hope that is ok with you.
To simplify review I've also pushed the series to branch cleanup on https://src.fedoraproject.org/forks/prudo/rpms/kexec-tools.git.
Thanks Philipp
rfc -> v1: - Rebased to latest rawhide - Patch 1 added two more fixes. - Patch 2,4&8 improved the commit message - Patch 3 dropped definition of _is_valid_kver as it was already present on latest rawhide - Added new patches 9-12
Philipp Rudo (12): Fix various shellcheck findings kdump-lib: make is_zstd_command_available more generic kdump-lib: simplify _get_kdump_kernel_version kdumpctl: drop _get_current_running_kernel_path kdumpctl: drop condrestart subcommand kdumpctl: simplify _update_kernel_cmdline kdumpctl: Prevent option --fadump on non-PPC in reset_crashkernel kdumpctl: Stop updating grub config in reset_crashkernel kdump.conf: Remove option override_resettable spec: Clean up handling of dracut files spec: Silence unversioned Obsolete warning kdump-lib: Harden _crashkernel_add
gen-kdump-conf.sh | 6 -- kdump-lib.sh | 175 ++++++++++++++++++++---------- kdump.conf.5 | 8 -- kdumpctl | 236 +++++++++++++---------------------------- kexec-tools.spec | 50 ++++----- mkdumprd | 57 +--------- mkfadumprd | 2 +- spec/kdump-lib_spec.sh | 38 ++++--- 8 files changed, 235 insertions(+), 337 deletions(-)
This includes fixes for
SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns. SC2005 (style): Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. SC2162 (info): read without -r will mangle backslashes. SC2086 (info): Double quote to prevent globbing and word splitting. SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).
In addition add some source hints to prevent false positive findings.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 10 +++++----- kdumpctl | 8 ++++++-- 2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index 4290be3..f79c13a 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -197,7 +197,7 @@ get_bind_mount_source() local _fsroot _src_nofsroot
_mnt=$(df "$1" | tail -1 | awk '{print $NF}') - _path=${1#$_mnt} + _path=${1#"$_mnt"}
_src=$(get_mount_info SOURCE target "$_mnt" -f) _opt=$(get_mount_info OPTIONS target "$_mnt" -f) @@ -214,7 +214,7 @@ get_bind_mount_source() echo "$_mnt$_path" && return fi
- _fsroot=${_src#${_src_nofsroot}[} + _fsroot=${_src#"${_src_nofsroot}"[} _fsroot=${_fsroot%]} _mnt=$(get_mount_info TARGET source "$_src_nofsroot" -f)
@@ -223,7 +223,7 @@ get_bind_mount_source() local _subvol _subvol=${_opt#*subvol=} _subvol=${_subvol%,*} - _fsroot=${_fsroot#$_subvol} + _fsroot=${_fsroot#"$_subvol"} fi echo "$_mnt$_fsroot$_path" } @@ -270,7 +270,7 @@ kdump_get_persistent_dev() dev=$(blkid -L "${dev#LABEL=}") ;; esac - echo $(get_persistent_dev "$dev") + get_persistent_dev "$dev" }
is_ostree() @@ -810,7 +810,7 @@ get_recommend_size() while read -r -d , range; do # need to use non-default IFS as double spaces are used as a # single delimiter while commas aren't... - IFS=, read start start_unit end end_unit size <<< \ + IFS=, read -r start start_unit end end_unit size <<< \ "$(echo "$range" | sed -n "s/([0-9]+)([GT]?)-([0-9]*)([GT]?):([0-9]+[MG])/\1,\2,\3,\4,\5/p")"
# aka. 102400T diff --git a/kdumpctl b/kdumpctl index 7e561fd..78790c9 100755 --- a/kdumpctl +++ b/kdumpctl @@ -29,14 +29,17 @@ if [[ -f /etc/sysconfig/kdump ]]; then fi
[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut -. $dracutbasedir/dracut-functions.sh +# shellcheck source=/dev/null +. "$dracutbasedir"/dracut-functions.sh
if [[ ${__SOURCED__:+x} ]]; then KDUMP_LIB_PATH=. else KDUMP_LIB_PATH=/lib/kdump fi +# shellcheck source=./kdump-lib.sh . $KDUMP_LIB_PATH/kdump-lib.sh +# shellcheck source=./kdump-logger.sh . $KDUMP_LIB_PATH/kdump-logger.sh
#initiate the kdump logger @@ -247,7 +250,7 @@ restore_default_initrd() if [[ $default_checksum != "$backup_checksum" ]]; then dwarn "WARNING: checksum mismatch! Can't restore original initrd.." else - rm -f $INITRD_CHECKSUM_LOCATION + rm -f "$INITRD_CHECKSUM_LOCATION" if mv "$DEFAULT_INITRD_BAK" "$DEFAULT_INITRD"; then derror "Restoring original initrd as fadump mode is disabled." sync -f "$DEFAULT_INITRD" @@ -497,6 +500,7 @@ check_drivers_modified() # If it's dump target is on block device, detect the block driver _target=$(get_block_dump_target) if [[ -n $_target ]]; then + # shellcheck disable=SC2317 _record_block_drivers() { local _drivers
There is value to use the function in other places as well. For example it can be used to check whether optional dependencies, like grubby, are installed. Thus make it more generic so it can be reused in later commits.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdump-lib.sh | 4 ++-- mkdumprd | 2 +- mkfadumprd | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index f79c13a..42844c8 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -48,9 +48,9 @@ is_squash_available() done }
-is_zstd_command_available() +has_command() { - [[ -x "$(command -v zstd)" ]] + [[ -x $(command -v "$1") ]] }
dracut_have_option() diff --git a/mkdumprd b/mkdumprd index f93874d..4feac28 100644 --- a/mkdumprd +++ b/mkdumprd @@ -443,7 +443,7 @@ handle_default_dump_target if ! have_compression_in_dracut_args; then if is_squash_available && dracut_have_option "--squash-compressor"; then add_dracut_arg "--squash-compressor" "zstd" - elif is_zstd_command_available; then + elif has_command zstd; then add_dracut_arg "--compress" "zstd" fi fi diff --git a/mkfadumprd b/mkfadumprd index 719e150..dd6840f 100755 --- a/mkfadumprd +++ b/mkfadumprd @@ -64,7 +64,7 @@ _dracut_isolate_args=(
# Use zstd compression method, if available if ! have_compression_in_dracut_args; then - if is_zstd_command_available; then + if has_command zstd; then _dracut_isolate_args+=(--compress zstd) fi fi
Only check whether modules for a given kernel version are installed instead of searching for a kernel image. It's safer to assume that every kernel uses kernel modules compared to that it follows certain naming conventions. Furthermore it is much more lightweight and thus allows to determine the KDUMP_KERNELVER much earlier for every command in kdumpctl.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdump-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index 42844c8..5fad026 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -586,7 +586,7 @@ _get_kdump_kernel_version()
_version_nondebug=${_version%+debug} _version_nondebug=${_version_nondebug%-debug} - if [[ -f "$(prepare_kdump_kernel "$_version_nondebug")" ]]; then + if _is_valid_kver "$_version_nondebug"; then dinfo "Use of debug kernel detected. Trying to use $_version_nondebug" echo "$_version_nondebug" else
_get_current_running_kernel_path is identical to _find_kernel_path_by_release $(uname -r) so simply use this instead of defining a new function.
While at it simplify reset_crashkernel slightly. This changes the behavior of the function for the case when KDUMP_KERNELVER is defined but no kernel with this version is installed. Before, the missing kernel is silently ignored and the currently running kernel is used instead. Now, kdumpctl will exit with an error.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdumpctl | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 78790c9..be270db 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1422,18 +1422,6 @@ _find_kernel_path_by_release() echo -n "$_kernel_path" }
-_get_current_running_kernel_path() -{ - local _release _path - - _release=$(uname -r) - if _path=$(_find_kernel_path_by_release "$_release"); then - echo -n "$_path" - else - return 1 - fi -} - _update_kernel_cmdline() { local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4 @@ -1628,14 +1616,11 @@ reset_crashkernel() # - use KDUMP_KERNELVER if it's defined # - use current running kernel if [[ -z $_grubby_kernel_path ]]; then - if [[ -z $KDUMP_KERNELVER ]] || - ! _kernel_path=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then - if ! _kernel_path=$(_get_current_running_kernel_path); then - derror "no running kernel found" - exit 1 - fi + [[ -n $KDUMP_KERNELVER ]] || KDUMP_KERNELVER=$(uname -r) + if ! _kernels=$(_find_kernel_path_by_release "$KDUMP_KERNELVER"); then + derror "no kernel for version $KDUMP_KERNELVER found" + exit 1 fi - _kernels=$_kernel_path else _kernels=$(_get_all_kernels_from_grubby "$_grubby_kernel_path") fi
condrestart is a left over from the time of SysVinit that is no longer needed since the kexec-tools switched to systemd (10c91a1 ("Removing sysvinit files") plus the one before). What's especially intriguing is that from the beginning (0112f36 ("- Add a kdump sysconfig file and init script - Spec file additions for pre/post install/uninstall")) the sub-command never did any actual work (other than not returning an error). Thus simply remove the condrestart sub-command.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdumpctl | 2 -- 1 file changed, 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index be270db..fcb7a5e 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1795,8 +1795,6 @@ main() rebuild) rebuild ;; - condrestart) ;; - propagate) propagate_ssh_key ;;
_update_kernel_cmdline handles two cmdline parameters at once. This does not only make the function itself but also its callers more complicated than necessary. For example in _update_crashkernel the fadump gets "updated" to the value that has been read from grubby. Thus simplify _update_kernel_cmdline to only update one parameter at once.
While at it shorten some variable named in the callers.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdumpctl | 106 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 50 deletions(-)
diff --git a/kdumpctl b/kdumpctl index fcb7a5e..618b35e 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1424,21 +1424,33 @@ _find_kernel_path_by_release()
_update_kernel_cmdline() { - local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4 + local _kernel _param _old _new + + _kernel="$1" + _param="$2" + _old="$3" + _new="$4" + + # _new = "" removes a _param, so _new = _old = "" is fine + [[ -n "$_new" ]] && [[ "$_old" == "$_new" ]] && return 1
if is_ostree; then - if rpm-ostree kargs | grep -q "crashkernel="; then - rpm-ostree kargs --replace="crashkernel=$_crashkernel" + if [[ -z "$_new" ]]; then + rpm-ostree kargs --delete="$_param=$_old" + elif [[ -n "$_old" ]]; then + rpm-ostree kargs --replace="$_param=$_new" else - rpm-ostree kargs --append="crashkernel=$_crashkernel" + rpm-ostree kargs --append="$_param=$_new" fi - else - grubby --args "crashkernel=$_crashkernel" --update-kernel "$_kernel_path" - if [[ $_dump_mode == kdump ]]; then - grubby --remove-args="fadump" --update-kernel "$_kernel_path" + elif has_command grubby; then + if [[ -n "$_new" ]]; then + grubby --update-kernel "$_kernel" --args "$_param=$_new" else - grubby --args="fadump=$_fadump_val" --update-kernel "$_kernel_path" + grubby --update-kernel "$_kernel" --remove-args "$_param" fi + else + derror "Unable to update kernel command line" + exit 1 fi
# Don't use "[[ CONDITION ]] && COMMAND" which returns 1 under false CONDITION @@ -1530,9 +1542,11 @@ _read_kernel_arg_in_grub_etc_default()
reset_crashkernel() { - local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel _kernels - local _old_crashkernel _new_crashkernel _new_dump_mode _crashkernel_changed - local _new_fadump_val _old_fadump_val _what_is_updated + local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels + local _dump_mode _new_dump_mode + local _has_changed _needs_reboot + local _old_ck _new_ck + local _old_fadump _new_fadump
for _opt in "$@"; do case "$_opt" in @@ -1570,14 +1584,10 @@ reset_crashkernel() # modifying the kernel command line so there is no need for kexec-tools # to repeat it. if is_ostree; then - _old_crashkernel=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') - _new_dump_mode=kdump - _new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") - if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then - _update_kernel_cmdline "" "$_new_crashkernel" "$_new_dump_mode" "" - if [[ $_reboot == yes ]]; then - systemctl reboot - fi + _old_ck=$(rpm-ostree kargs | sed -n -E 's/.*(^|\s)crashkernel=(\S*).*/\2/p') + _new_ck=$(kdump_get_arch_recommend_crashkernel kdump) + if _update_kernel_cmdline "" crashkernel "$_old_ck" "$_new_ck"; then + [[ $_reboot == yes ]] && systemctl reboot fi return fi @@ -1626,35 +1636,33 @@ reset_crashkernel() fi
for _kernel in $_kernels; do + _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") - _new_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") + _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode - _new_crashkernel=$_crashkernel - _new_fadump_val=$_fadump_val + _new_ck=$_crashkernel + _new_fadump=$_fadump_val fi
- _old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) - _old_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - if [[ $_old_crashkernel != "$_new_crashkernel" || $_old_fadump_val != "$_new_fadump_val" ]]; then - _update_kernel_cmdline "$_kernel" "$_new_crashkernel" "$_new_dump_mode" "$_new_fadump_val" - if [[ $_reboot != yes ]]; then - if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then - _what_is_updated="Updated crashkernel=$_new_crashkernel" - else - # This case happens only when switching between fadump=on and fadump=nocma - _what_is_updated="Updated fadump=$_new_fadump_val" - fi - dwarn "$_what_is_updated for kernel=$_kernel. Please reboot the system for the change to take effect." - fi - _crashkernel_changed=yes + _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) + if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then + _has_changed="Updated fadump=$_new_fadump" + fi + if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then + _has_changed="Updated crashkernel=$_new_ck" + fi + if [[ -n "$_has_changed" ]]; then + _needs_reboot=1 + dwarn "$_has_changed for kernel=$_kernel. Please reboot the system for the change to take effect." fi done
- if [[ $_reboot == yes && $_crashkernel_changed == yes ]]; then - reboot + if [[ $_reboot == yes && $_needs_reboot == 1 ]]; then + systemctl reboot fi }
@@ -1670,21 +1678,19 @@ _is_bootloader_installed()
_update_crashkernel() { - local _kernel _kver _dump_mode _old_default_crashkernel _new_default_crashkernel _fadump_val _msg + local _kernel _kver _dump_mode _msg + local _old_ck _new_ck
_kernel=$1 _dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _old_default_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) _kver=$(parse_kver_from_path "$_kernel") # The second argument is for the case of aarch64, where installing a 64k variant on a 4k kernel, or vice versa - _new_default_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode" "$_kver") - if [[ $_old_default_crashkernel != "$_new_default_crashkernel" ]]; then - _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) - if _update_kernel_cmdline "$_kernel" "$_new_default_crashkernel" "$_dump_mode" "$_fadump_val"; then - _msg="For kernel=$_kernel, crashkernel=$_new_default_crashkernel now. Please reboot the system for the change to take effet." - _msg+=" Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." - dinfo "$_msg" - fi + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_dump_mode" "$_kver") + if _update_kernel_cmdline "$_kernel" crashkernel "$_old_ck" "$_new_ck"; then + _msg="For kernel=$_kernel, crashkernel=$_new_ck now. Please reboot the system for the change to take effect." + _msg+=" Note if you don't want kexec-tools to manage the crashkernel kernel parameter, please set auto_reset_crashkernel=no in /etc/kdump.conf." + dinfo "$_msg" fi }
Prevent the --fadump option to be used on non-PPC systems. This not only prevents user errors but also guarantees that _dump_mode and _fadump_val are empty on these systems.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdumpctl | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 618b35e..3431bf7 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1551,6 +1551,10 @@ reset_crashkernel() for _opt in "$@"; do case "$_opt" in --fadump=*) + if [[ $(uname -m) != ppc64le ]]; then + derror "Option --fadump only valid on PPC" + exit 1 + fi _val=${_opt#*=} if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then _fadump_val=$_val @@ -1592,12 +1596,7 @@ reset_crashkernel() return fi
- # For non-ppc64le systems, the dump mode is always kdump since only ppc64le - # has FADump. - if [[ -z $_dump_mode && $(uname -m) != ppc64le ]]; then - _dump_mode=kdump - _fadump_val=off - fi + [[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
# If the dump mode is determined, we can also know the default crashkernel value if [[ -n $_dump_mode ]]; then
With multiple kernel variants on the same architecture, e.g. the 4k and 64k kernel on aarch64, we can no longer assume that the crashkernel value for the currently running kernel will work for all installed kernels. This also means that we can no longer update the grub config as we don't know which value to set it to. Thus get the crashkernel value for each kernel and stop updating the grub config.
While at it merge the _new_fadump and _fadump_val variables and remove _read_kernel_arg_in_grub_etc_default which has no user.
Signed-off-by: Philipp Rudo prudo@redhat.com Reviewed-by: Pingfan Liu piliu@redhat.com --- kdumpctl | 96 +++----------------------------------------------------- 1 file changed, 5 insertions(+), 91 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 3431bf7..ce58cc9 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1480,69 +1480,9 @@ _get_all_kernels_from_grubby() echo -n "$_kernels" }
-GRUB_ETC_DEFAULT="/etc/default/grub" -# Update a kernel parameter in default grub conf -# -# If a value is specified, it will be inserted in the end. Otherwise it -# would remove given kernel parameter. -# -# Note this function doesn't address the following cases, -# 1. The kernel ignores everything on the command line after a '--'. So -# simply adding the new entry to the end will fail if the cmdline -# contains a --. -# 2. If the value for a parameter contains spaces it can be quoted using -# double quotes, for example param="value with spaces". This will -# break the [^[:space:]"] regex for the value. -# 3. Dashes and underscores in the parameter name are equivalent. So -# some_parameter and some-parameter are identical. -# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. -# 5. Some kernel parameters, e.g. quiet, doesn't have value -# -# $1: the name of the kernel command line parameter -# $2: new value. If empty, given parameter would be removed -_update_kernel_arg_in_grub_etc_default() -{ - local _para=$1 _val=$2 _para_val - - if [[ $(uname -m) == s390x ]]; then - return - fi - - if [[ -n $_val ]]; then - _para_val="$_para=$_val" - fi - - # Update the command line /etc/default/grub, i.e. - # on the line that starts with 'GRUB_CMDLINE_LINUX=', - # 1) remove $para=$val if the it's the first arg - # 2) remove all occurences of $para=$val - # 3) insert $_para_val to end - # 4) remove duplicate spaces left over by 1) or 2) or 3) - # 5) remove space at the beginning of the string left over by 1) or 2) or 3) - # 6) remove space at the end of the string left over by 1) or 2) or 3) - sed -i -E "/^GRUB_CMDLINE_LINUX=/ { - s/"${_para}=[^[:space:]"]*/"/g; - s/[[:space:]]+${_para}=[^[:space:]"]*/ /g; - s/"$/ ${_para_val}"/ - s/[[:space:]]+/ /g; - s/(")[[:space:]]+/\1/g; - s/[[:space:]]+(")/\1/g; - }" "$GRUB_ETC_DEFAULT" -} - -# Read the kernel arg in default grub conf. - -# Note reading a kernel parameter that doesn't have a value isn't supported. -# -# $1: the name of the kernel command line parameter -_read_kernel_arg_in_grub_etc_default() -{ - sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]"]${1}=([^[:space:]"]*).*$/\1/p" "$GRUB_ETC_DEFAULT" -} - reset_crashkernel() { - local _opt _val _fadump_val _reboot _grubby_kernel_path _kernel _kernels + local _opt _val _reboot _grubby_kernel_path _kernel _kernels local _dump_mode _new_dump_mode local _has_changed _needs_reboot local _old_ck _new_ck @@ -1555,13 +1495,12 @@ reset_crashkernel() derror "Option --fadump only valid on PPC" exit 1 fi - _val=${_opt#*=} - if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then - _fadump_val=$_val - else + _new_fadump=${_opt#*=} + if ! _dump_mode=$(get_dump_mode_by_fadump_val $_new_fadump); then derror "failed to determine dump mode" exit fi + [[ "$_new_fadump" == off ]] && _new_fadump="" ;; --kernel=*) _val=${_opt#*=} @@ -1598,29 +1537,6 @@ reset_crashkernel()
[[ $(uname -m) != ppc64le ]] && _dump_mode=kdump
- # If the dump mode is determined, we can also know the default crashkernel value - if [[ -n $_dump_mode ]]; then - _crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode") - fi - - # If --kernel-path=ALL, update GRUB_CMDLINE_LINUX in /etc/default/grub. - # - # An exception case is when the ppc64le user doesn't specify the fadump value. - # In this case, the dump mode would be determined by parsing the kernel - # command line of the kernel(s) to be updated thus don't update GRUB_CMDLINE_LINUX. - # - # The following code has been simplified because of what has been done early, - # - set the dump mode as kdump for non-ppc64le cases - # - retrieved the default crashkernel value for given dump mode - if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then - _update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel" - # remove the fadump if fadump is disabled - if [[ $_fadump_val == off ]]; then - _fadump_val="" - fi - _update_kernel_arg_in_grub_etc_default fadump "$_fadump_val" - fi - # If kernel-path not specified, either # - use KDUMP_KERNELVER if it's defined # - use current running kernel @@ -1638,15 +1554,13 @@ reset_crashkernel() _has_changed="" if [[ -z $_dump_mode ]]; then _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel") - _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _new_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else _new_dump_mode=$_dump_mode - _new_ck=$_crashkernel - _new_fadump=$_fadump_val fi
_old_ck=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel) + _new_ck=$(kdump_get_arch_recommend_crashkernel "$_new_dump_mode") _old_fadump=$(get_grub_kernel_boot_parameter "$_kernel" fadump) if _update_kernel_cmdline "$_kernel" fadump "$_old_fadump" "$_new_fadump"; then _has_changed="Updated fadump=$_new_fadump"
When override_resettable was introduced in 2013 with 4b850d2 ("Check if block device as dump target is resettable") it was forgotten to add the new option to check_config (today the function is called parse_config). So if a user would have set override_resettable check_config would have returned an error ("Invalid kdump config option override_resettable") and starting the kdump service would have failed. As there has been no bug report in the last ~10 years it is safe to assume that the option was never used. Thus simply remove the option.
Fixes: 4b850d2 ("Check if block device as dump target is resettable") Signed-off-by: Philipp Rudo prudo@redhat.com --- gen-kdump-conf.sh | 6 ------ kdump.conf.5 | 8 ------- mkdumprd | 55 ----------------------------------------------- 3 files changed, 69 deletions(-)
diff --git a/gen-kdump-conf.sh b/gen-kdump-conf.sh index 4abcd39..a8e7fdd 100755 --- a/gen-kdump-conf.sh +++ b/gen-kdump-conf.sh @@ -157,12 +157,6 @@ generate() # force_no_rebuild and force_rebuild options are mutually # exclusive and they should not be set to 1 simultaneously. # -# override_resettable <0 | 1> -# - Usually an unresettable block device can't be a dump target. -# Specifying 1 when you want to dump even though the block -# target is unresettable -# By default, it is 0, which will not try dumping destined to fail. -# # dracut_args <arg(s)> # - Pass extra dracut options when rebuilding kdump initrd. # diff --git a/kdump.conf.5 b/kdump.conf.5 index ec28552..9117aa7 100644 --- a/kdump.conf.5 +++ b/kdump.conf.5 @@ -218,14 +218,6 @@ force_no_rebuild and force_rebuild options are mutually exclusive and they should not be set to 1 simultaneously. .RE
-.B override_resettable <0 | 1> -.RS -Usually an unresettable block device can't be a dump target. Specifying 1 means -that even though the block target is unresettable, the user wants to try dumping anyway. -By default, it's set to 0, which will not try something destined to fail. -.RE - - .B dracut_args <arg(s)> .RS Kdump uses dracut to generate initramfs for second kernel. This option diff --git a/mkdumprd b/mkdumprd index 4feac28..31c4b76 100644 --- a/mkdumprd +++ b/mkdumprd @@ -24,7 +24,6 @@ fi
SSH_KEY_LOCATION=$DEFAULT_SSHKEY SAVE_PATH=$(get_save_path) -OVERRIDE_RESETTABLE=0
extra_modules="" dracut_args=(--add kdumpbase --quiet --hostonly --hostonly-cmdline --hostonly-i18n --hostonly-mode strict --hostonly-nics '' --aggressive-strip -o "plymouth resume ifcfg earlykdump") @@ -309,56 +308,6 @@ handle_default_dump_target() check_size fs "$_target" }
-# $1: function name -for_each_block_target() -{ - local dev majmin - - for dev in $(get_kdump_targets); do - [[ -b $dev ]] || continue - majmin=$(get_maj_min "$dev") - check_block_and_slaves "$1" "$majmin" && return 1 - done - - return 0 -} - -#judge if a specific device with $1 is unresettable -#return false if unresettable. -is_unresettable() -{ - local path device resettable=1 - - path="/sys/$(udevadm info --query=all --path="/sys/dev/block/$1" | awk '/^P:/ {print $2}' | sed -e 's/(cciss[0-9]+/).*/\1/g' -e 's//block/.*$//')/resettable" - if [[ -f $path ]]; then - resettable="$(< "$path")" - [[ $resettable -eq 0 ]] && [[ $OVERRIDE_RESETTABLE -eq 0 ]] && { - device=$(udevadm info --query=all --path="/sys/dev/block/$1" | awk -F= '/DEVNAME/{print $2}') - derror "Error: Can not save vmcore because device $device is unresettable" - return 0 - } - fi - - return 1 -} - -#check if machine is resettable. -#return true if resettable -check_resettable() -{ - local _target _override_resettable - - _override_resettable=$(kdump_get_conf_val override_resettable) - OVERRIDE_RESETTABLE=${_override_resettable:-$OVERRIDE_RESETTABLE} - if [ "$OVERRIDE_RESETTABLE" != "0" ] && [ "$OVERRIDE_RESETTABLE" != "1" ]; then - perror_exit "override_resettable value '$OVERRIDE_RESETTABLE' is invalid" - fi - - for_each_block_target is_unresettable && return - - return 1 -} - check_crypt() { local _dev @@ -370,10 +319,6 @@ check_crypt() done }
-if ! check_resettable; then - exit 1 -fi - if ! check_crypt; then dwarn "Warning: Encrypted device is in dump path, which is not recommended, see kexec-kdump-howto.txt for more details." fi
The indirection seems to be a leftover from a time when there was no modeules.d directory to drop in the dracut modules. Remove this indirection and combine the three macros to remove the prefix into one.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kexec-tools.spec | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/kexec-tools.spec b/kexec-tools.spec index 5a6648e..a477c8e 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -228,39 +228,28 @@ mkdir -p $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ install -m 644 makedumpfile-%{mkdf_ver}/eppic_scripts/* $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ %endif
-%define remove_dracut_prefix() %(echo -n %1|sed 's/.*dracut-//g') -%define remove_dracut_early_kdump_prefix() %(echo -n %1|sed 's/.*dracut-early-kdump-//g') -%define remove_dracut_fadump_prefix() %(echo -n %1|sed 's/.*dracut-fadump-//g') +%define dracutdir %{_prefix}/lib/dracut/modules.d +%define remove_prefix() %(echo -n %2|sed 's/.*%1-//g')
# deal with dracut modules -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase -cp %{SOURCE100} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -cp %{SOURCE101} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -cp %{SOURCE102} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE102}} -cp %{SOURCE104} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE104}} -cp %{SOURCE106} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE106}} -cp %{SOURCE107} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE107}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump -cp %{SOURCE108} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -cp %{SOURCE109} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase +install -m 755 %{SOURCE100} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE100}} +install -m 755 %{SOURCE101} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE101}} +install -m 644 %{SOURCE102} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE102}} +install -m 644 %{SOURCE104} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE104}} +install -m 644 %{SOURCE106} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE106}} +install -m 644 %{SOURCE107} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE107}} + +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump +install -m 755 %{SOURCE108} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut %{SOURCE108}} +install -m 755 %{SOURCE109} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut-early-kdump %{SOURCE109}} + %ifarch ppc64 ppc64le -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit -cp %{SOURCE200} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -cp %{SOURCE201} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit +install -m 755 %{SOURCE200} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE200}} +install -m 755 %{SOURCE201} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE201}} %endif
- -%define dracutlibdir %{_prefix}/lib/dracut -#and move the custom dracut modules to the dracut directory -mkdir -p $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ -mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ - %post # Initial installation %systemd_post kdump.service @@ -363,7 +352,7 @@ fi %config %{_udevrulesdir} %{_udevrulesdir}/../kdump-udev-throttler %endif -%{dracutlibdir}/modules.d/* +%{dracutdir}/* %dir %{_localstatedir}/crash %dir %{_sysconfdir}/kdump %dir %{_sysconfdir}/kdump/pre.d
On Fri, Jul 21, 2023 at 10:24 PM Philipp Rudo prudo@redhat.com wrote:
The indirection seems to be a leftover from a time when there was no modeules.d directory to drop in the dracut modules. Remove this
Here I am not sure whether I catch your meaning. But the directory 'modules.d' seems to exist before this patch, otherwise how can it be achieved for the statement 'mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/'.
So I think you try to directly operate on modules.d instead of a temporary directory.
For the rest.
Reviewed-by: Pingfan Liu piliu@redhat.com
indirection and combine the three macros to remove the prefix into one.
Signed-off-by: Philipp Rudo prudo@redhat.com
kexec-tools.spec | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/kexec-tools.spec b/kexec-tools.spec index 5a6648e..a477c8e 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -228,39 +228,28 @@ mkdir -p $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ install -m 644 makedumpfile-%{mkdf_ver}/eppic_scripts/* $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ %endif
-%define remove_dracut_prefix() %(echo -n %1|sed 's/.*dracut-//g') -%define remove_dracut_early_kdump_prefix() %(echo -n %1|sed 's/.*dracut-early-kdump-//g') -%define remove_dracut_fadump_prefix() %(echo -n %1|sed 's/.*dracut-fadump-//g') +%define dracutdir %{_prefix}/lib/dracut/modules.d +%define remove_prefix() %(echo -n %2|sed 's/.*%1-//g')
# deal with dracut modules -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase -cp %{SOURCE100} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -cp %{SOURCE101} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -cp %{SOURCE102} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE102}} -cp %{SOURCE104} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE104}} -cp %{SOURCE106} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE106}} -cp %{SOURCE107} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE107}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump -cp %{SOURCE108} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -cp %{SOURCE109} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase +install -m 755 %{SOURCE100} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE100}} +install -m 755 %{SOURCE101} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE101}} +install -m 644 %{SOURCE102} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE102}} +install -m 644 %{SOURCE104} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE104}} +install -m 644 %{SOURCE106} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE106}} +install -m 644 %{SOURCE107} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE107}}
+mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump +install -m 755 %{SOURCE108} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut %{SOURCE108}} +install -m 755 %{SOURCE109} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut-early-kdump %{SOURCE109}}
%ifarch ppc64 ppc64le -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit -cp %{SOURCE200} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -cp %{SOURCE201} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit +install -m 755 %{SOURCE200} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE200}} +install -m 755 %{SOURCE201} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE201}} %endif
-%define dracutlibdir %{_prefix}/lib/dracut -#and move the custom dracut modules to the dracut directory -mkdir -p $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ -mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/
%post # Initial installation %systemd_post kdump.service @@ -363,7 +352,7 @@ fi %config %{_udevrulesdir} %{_udevrulesdir}/../kdump-udev-throttler %endif -%{dracutlibdir}/modules.d/* +%{dracutdir}/* %dir %{_localstatedir}/crash %dir %{_sysconfdir}/kdump %dir %{_sysconfdir}/kdump/pre.d -- 2.40.0
Hi Pingfan,
On Tue, 25 Jul 2023 10:50:59 +0800 Pingfan Liu piliu@redhat.com wrote:
On Fri, Jul 21, 2023 at 10:24 PM Philipp Rudo prudo@redhat.com wrote:
The indirection seems to be a leftover from a time when there was no modeules.d directory to drop in the dracut modules. Remove this
Here I am not sure whether I catch your meaning. But the directory 'modules.d' seems to exist before this patch, otherwise how can it be achieved for the statement 'mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/'.
Good point. My original thought was that modules.d was introduced with 005c06f ("Update kdump dracut module to use the latest dracut feature") as that's the last commit that touched the line that moves everything to modules.d. So I thought that modules.d was one of the new features. But I've missed that the director already existed before. In fact it was introduced with the move to dracut...
So yeah, not sure what the reason was to introduce the kdump-adv-conf dir in the first place but the commit message as is is wrong.
So I think you try to directly operate on modules.d instead of a temporary directory.
How rephrasing the commit message to this
Currently the dracut modules are first prepared in a temporary directory before they are moved to modules.d. All the preparation work can be done by a single call to 'install' per file. Thus get rid off the indirection and install the dracut modules directly to modules.d.
While at it merge the three macros to remove the prefix into one.
For the rest.
Reviewed-by: Pingfan Liu piliu@redhat.com
Thanks! Philipp
indirection and combine the three macros to remove the prefix into one.
Signed-off-by: Philipp Rudo prudo@redhat.com
kexec-tools.spec | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/kexec-tools.spec b/kexec-tools.spec index 5a6648e..a477c8e 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -228,39 +228,28 @@ mkdir -p $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ install -m 644 makedumpfile-%{mkdf_ver}/eppic_scripts/* $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ %endif
-%define remove_dracut_prefix() %(echo -n %1|sed 's/.*dracut-//g') -%define remove_dracut_early_kdump_prefix() %(echo -n %1|sed 's/.*dracut-early-kdump-//g') -%define remove_dracut_fadump_prefix() %(echo -n %1|sed 's/.*dracut-fadump-//g') +%define dracutdir %{_prefix}/lib/dracut/modules.d +%define remove_prefix() %(echo -n %2|sed 's/.*%1-//g')
# deal with dracut modules -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase -cp %{SOURCE100} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -cp %{SOURCE101} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -cp %{SOURCE102} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE102}} -cp %{SOURCE104} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE104}} -cp %{SOURCE106} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE106}} -cp %{SOURCE107} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE107}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump -cp %{SOURCE108} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -cp %{SOURCE109} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase +install -m 755 %{SOURCE100} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE100}} +install -m 755 %{SOURCE101} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE101}} +install -m 644 %{SOURCE102} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE102}} +install -m 644 %{SOURCE104} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE104}} +install -m 644 %{SOURCE106} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE106}} +install -m 644 %{SOURCE107} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE107}}
+mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump +install -m 755 %{SOURCE108} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut %{SOURCE108}} +install -m 755 %{SOURCE109} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut-early-kdump %{SOURCE109}}
%ifarch ppc64 ppc64le -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit -cp %{SOURCE200} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -cp %{SOURCE201} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit +install -m 755 %{SOURCE200} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE200}} +install -m 755 %{SOURCE201} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE201}} %endif
-%define dracutlibdir %{_prefix}/lib/dracut -#and move the custom dracut modules to the dracut directory -mkdir -p $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ -mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/
%post # Initial installation %systemd_post kdump.service @@ -363,7 +352,7 @@ fi %config %{_udevrulesdir} %{_udevrulesdir}/../kdump-udev-throttler %endif -%{dracutlibdir}/modules.d/* +%{dracutdir}/* %dir %{_localstatedir}/crash %dir %{_sysconfdir}/kdump %dir %{_sysconfdir}/kdump/pre.d -- 2.40.0
On Wed, Jul 26, 2023 at 8:56 PM Philipp Rudo prudo@redhat.com wrote:
Hi Pingfan,
On Tue, 25 Jul 2023 10:50:59 +0800 Pingfan Liu piliu@redhat.com wrote:
On Fri, Jul 21, 2023 at 10:24 PM Philipp Rudo prudo@redhat.com wrote:
The indirection seems to be a leftover from a time when there was no modeules.d directory to drop in the dracut modules. Remove this
Here I am not sure whether I catch your meaning. But the directory 'modules.d' seems to exist before this patch, otherwise how can it be achieved for the statement 'mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/'.
Good point. My original thought was that modules.d was introduced with 005c06f ("Update kdump dracut module to use the latest dracut feature") as that's the last commit that touched the line that moves everything to modules.d. So I thought that modules.d was one of the new features. But I've missed that the director already existed before. In fact it was introduced with the move to dracut...
So yeah, not sure what the reason was to introduce the kdump-adv-conf dir in the first place but the commit message as is is wrong.
So I think you try to directly operate on modules.d instead of a temporary directory.
How rephrasing the commit message to this
Currently the dracut modules are first prepared in a temporary directory before they are moved to modules.d. All the preparation work can be done by a single call to 'install' per file. Thus get rid off the indirection and install the dracut modules directly to modules.d.
While at it merge the three macros to remove the prefix into one.
LGTM.
Thanks,
Pingfan
For the rest.
Reviewed-by: Pingfan Liu piliu@redhat.com
Thanks! Philipp
indirection and combine the three macros to remove the prefix into one.
Signed-off-by: Philipp Rudo prudo@redhat.com
kexec-tools.spec | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/kexec-tools.spec b/kexec-tools.spec index 5a6648e..a477c8e 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -228,39 +228,28 @@ mkdir -p $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ install -m 644 makedumpfile-%{mkdf_ver}/eppic_scripts/* $RPM_BUILD_ROOT/usr/share/makedumpfile/eppic_scripts/ %endif
-%define remove_dracut_prefix() %(echo -n %1|sed 's/.*dracut-//g') -%define remove_dracut_early_kdump_prefix() %(echo -n %1|sed 's/.*dracut-early-kdump-//g') -%define remove_dracut_fadump_prefix() %(echo -n %1|sed 's/.*dracut-fadump-//g') +%define dracutdir %{_prefix}/lib/dracut/modules.d +%define remove_prefix() %(echo -n %2|sed 's/.*%1-//g')
# deal with dracut modules -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase -cp %{SOURCE100} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -cp %{SOURCE101} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -cp %{SOURCE102} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE102}} -cp %{SOURCE104} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE104}} -cp %{SOURCE106} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE106}} -cp %{SOURCE107} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE107}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE100}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99kdumpbase/%{remove_dracut_prefix %{SOURCE101}} -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump -cp %{SOURCE108} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -cp %{SOURCE109} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_prefix %{SOURCE108}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99earlykdump/%{remove_dracut_early_kdump_prefix %{SOURCE109}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase +install -m 755 %{SOURCE100} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE100}} +install -m 755 %{SOURCE101} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE101}} +install -m 644 %{SOURCE102} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE102}} +install -m 644 %{SOURCE104} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE104}} +install -m 644 %{SOURCE106} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE106}} +install -m 644 %{SOURCE107} $RPM_BUILD_ROOT/%{dracutdir}/99kdumpbase/%{remove_prefix dracut %{SOURCE107}}
+mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump +install -m 755 %{SOURCE108} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut %{SOURCE108}} +install -m 755 %{SOURCE109} $RPM_BUILD_ROOT/%{dracutdir}/99earlykdump/%{remove_prefix dracut-early-kdump %{SOURCE109}}
%ifarch ppc64 ppc64le -mkdir -p -m755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit -cp %{SOURCE200} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -cp %{SOURCE201} $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE200}} -chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpinit/%{remove_dracut_fadump_prefix %{SOURCE201}} +mkdir -p -m755 $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit +install -m 755 %{SOURCE200} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE200}} +install -m 755 %{SOURCE201} $RPM_BUILD_ROOT/%{dracutdir}/99zz-fadumpinit/%{remove_prefix dracut-fadump %{SOURCE201}} %endif
-%define dracutlibdir %{_prefix}/lib/dracut -#and move the custom dracut modules to the dracut directory -mkdir -p $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ -mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/
%post # Initial installation %systemd_post kdump.service @@ -363,7 +352,7 @@ fi %config %{_udevrulesdir} %{_udevrulesdir}/../kdump-udev-throttler %endif -%{dracutlibdir}/modules.d/* +%{dracutdir}/* %dir %{_localstatedir}/crash %dir %{_sysconfdir}/kdump %dir %{_sysconfdir}/kdump/pre.d -- 2.40.0
rpmbuild throws a warning with
line 80: It's not recommended to have unversioned Obsoletes: Obsoletes: diskdumputils netdump kexec-tools-eppic
In that diskdump and netdump were last used in RHEL4 and kexec-tools-eppic was removed with Fedora 22. There is no supported update path in which a current package could replace one of these three. Thus simply drop the Obsoletes.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kexec-tools.spec | 3 --- 1 file changed, 3 deletions(-)
diff --git a/kexec-tools.spec b/kexec-tools.spec index a477c8e..ca3179e 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -75,9 +75,6 @@ BuildRequires: zlib-devel elfutils-devel glib2-devel bzip2-devel ncurses-devel b BuildRequires: pkgconfig intltool gettext BuildRequires: systemd-rpm-macros BuildRequires: automake autoconf libtool -%ifarch %{ix86} x86_64 ppc64 ppc s390x ppc64le -Obsoletes: diskdumputils netdump kexec-tools-eppic -%endif
%ifnarch s390x Requires: systemd-udev%{?_isa}
_crashkernel_add currently always assumes the good case, i.e. that the value of the crashkernel parameter has the correct syntax and that the delta added is a number. Both doesn't have to be true when the values are provided by users. Thus add some additional checks.
Furthermore require the delta to have a explicit unit, i.e. no longer assume that is in megabytes, i.e. 100 -> 100M.
Signed-off-by: Philipp Rudo prudo@redhat.com --- kdump-lib.sh | 159 ++++++++++++++++++++++++++++------------- spec/kdump-lib_spec.sh | 38 ++++++---- 2 files changed, 136 insertions(+), 61 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index 5fad026..c2c7e26 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -840,62 +840,125 @@ has_aarch64_smmu() ls /sys/devices/platform/arm-smmu-* 1> /dev/null 2>&1 }
-# $1 crashkernel="" -# $2 delta in unit of MB -_crashkernel_add() +is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[KkMmGg]?$ ]]; } + +# range defined for crashkernel parameter +# i.e. <start>-[<end>] +is_memrange() +{ + is_memsize "${1%-*}" || return 1 + [[ -n ${1#*-} ]] || return 0 + is_memsize "${1#*-}" +} + +to_bytes() { - local _ck _add _entry _ret - local _range _size _offset + local _s + + _s="$1" + is_memsize "$_s" || return 1 + + case "${_s: -1}" in + K|k) + _s=${_s::-1} + _s="$((_s * 1024))" + ;; + M|m) + _s=${_s::-1} + _s="$((_s * 1024 * 1024))" + ;; + G|g) + _s=${_s::-1} + _s="$((_s * 1024 * 1024 * 1024))" + ;; + *) + ;; + esac + echo "$_s" +}
- _ck="$1" - _add="$2" - _ret="" +memsize_add() +{ + local -a units=("" "K" "M" "G") + local i a b
- if [[ "$_ck" == *@* ]]; then - _offset="@${_ck##*@}" - _ck=${_ck%@*} - elif [[ "$_ck" == *,high ]] || [[ "$_ck" == *,low ]]; then - _offset=",${_ck##*,}" - _ck=${_ck%,*} + a=$(to_bytes "$1") || return 1 + b=$(to_bytes "$2") || return 1 + i=0 + + (( a += b )) + while :; do + [[ $(( a / 1024 )) -eq 0 ]] && break + [[ $(( a % 1024 )) -ne 0 ]] && break + [[ $(( ${#units[@]} - 1 )) -eq $i ]] && break + + (( a /= 1024 )) + (( i += 1 )) + done + + echo "${a}${units[$i]}" +} + +_crashkernel_parse() +{ + local ck entry + local range size offset + + ck="$1" + + if [[ "$ck" == *@* ]]; then + offset="@${ck##*@}" + ck=${ck%@*} + elif [[ "$ck" == *,high ]] || [[ "$ck" == *,low ]]; then + offset=",${ck##*,}" + ck=${ck%,*} else - _offset='' + offset='' fi
- while read -d , -r _entry; do - [[ -n "$_entry" ]] || continue - if [[ "$_entry" == *:* ]]; then - _range=${_entry%:*} - _size=${_entry#*:} + while read -d , -r entry; do + [[ -n "$entry" ]] || continue + if [[ "$entry" == *:* ]]; then + range=${entry%:*} + size=${entry#*:} else - _range="" - _size=${_entry} + range="" + size=${entry} + fi + + echo "$size;$range;" + done <<< "$ck," + echo ";;$offset" +} + +# $1 crashkernel command line parameter +# $2 size to be added +_crashkernel_add() +{ + local ck delta ret + local range size offset + + ck="$1" + delta="$2" + ret="" + + while IFS=';' read -r size range offset; do + if [[ -n "$offset" ]]; then + ret="${ret%,}$offset" + break fi
- case "${_size: -1}" in - K) - _size=${_size::-1} - _size="$((_size + (_add * 1024)))K" - ;; - M) - _size=${_size::-1} - _size="$((_size + _add))M" - ;; - G) - _size=${_size::-1} - _size="$((_size * 1024 + _add))M" - ;; - *) - _size="$((_size + (_add * 1024 * 1024)))" - ;; - esac - - [[ -n "$_range" ]] && _ret+="$_range:" - _ret+="$_size," - done <<< "$_ck," - - _ret=${_ret%,} - [[ -n "$_offset" ]] && _ret+=$_offset - echo "$_ret" + [[ -n "$size" ]] || continue + if [[ -n "$range" ]]; then + is_memrange "$range" || return 1 + ret+="$range:" + fi + + size=$(memsize_add "$size" "$delta") || return 1 + ret+="$size," + done < <( _crashkernel_parse "$ck") + + echo "${ret%,}" }
# get default crashkernel @@ -945,7 +1008,7 @@ kdump_get_arch_recommend_crashkernel() #4k kernel, mlx5 consumes extra 124M memory, and choose 150M has_mlx5 && ((_delta += 150)) fi - _ck_cmdline=$(_crashkernel_add "$_ck_cmdline" "$_delta") + _ck_cmdline=$(_crashkernel_add "$_ck_cmdline" "${_delta}M") elif [[ $_arch == "ppc64le" ]]; then if [[ $_dump_mode == "fadump" ]]; then _ck_cmdline="4G-16G:768M,16G-64G:1G,64G-128G:2G,128G-1T:4G,1T-2T:6G,2T-4T:12G,4T-8T:20G,8T-16T:36G,16T-32T:64G,32T-64T:128G,64T-:180G" diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 81f0f86..814f961 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -49,21 +49,33 @@ Describe 'kdump-lib' End
Describe "_crashkernel_add()" - Context "when the input parameter is '1G-4G:256M,4G-64G:320M,64G-:576M'" - delta=100 + Context "For valid input values" Parameters - "1G-4G:256M,4G-64G:320M,64G-:576M" "1G-4G:356M,4G-64G:420M,64G-:676M" - "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" - "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" - "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G" - "300M,high" "400M,high" - "300M,low" "400M,low" - "500M@1G" "600M@1G" + "1G-4G:256M,4G-64G:320M,64G-:576M" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M" + "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" + "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "100M" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" + "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "100M" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G" + "1,high" "1" "2,high" + "1K,low" "1" "1025,low" + "1M@1G" "1k" "1025K@1G" + "500M@1G" "-100m" "400M@1G" + "1099511627776" "0" "1024G" End - It "should add delta to the values after ':'" - - When call _crashkernel_add "$1" "$delta" - The output should equal "$2" + It "should add delta to every value after ':'" + When call _crashkernel_add "$1" "$2" + The output should equal "$3" + End + End + Context "For invalid input values" + Parameters + "1G-4G:256M.4G-64G:320M" "100M" + "foo" "1" + "1" "bar" + End + It "shall return an error" + When call _crashkernel_add "$1" "$2" + The output should equal "" + The status should be failure End End End
Hi Philipp,
Glad to see these chores are done. They make the code neat.
I only have a minor disagreement on the commit log of [10/12].
For the rest of the series,
Reviewed-by: Pingfan Liu piliu@redhat.com
On Fri, Jul 21, 2023 at 10:24 PM Philipp Rudo prudo@redhat.com wrote:
Hi everybody,
this series got spit from the CLI rework series as they are independent and can be merged earlier to reduce the overall series size. While most of the patches were part of the rfc v1 of the CLI rework series patches 9-12 are new.
@Pingfan: I've dropped your Reviewed-by from patch 1 as I've added two more fixes to the patch. But I've kept it for patch 2&4 where I've updated the commit message and patch 3 where I've dropped the definition of _is_valid_kver as after the rebase an identical function already existed. Hope that is ok with you.
To simplify review I've also pushed the series to branch cleanup on https://src.fedoraproject.org/forks/prudo/rpms/kexec-tools.git.
Thanks Philipp
rfc -> v1:
- Rebased to latest rawhide
- Patch 1 added two more fixes.
- Patch 2,4&8 improved the commit message
- Patch 3 dropped definition of _is_valid_kver as it was already present on latest rawhide
- Added new patches 9-12
Philipp Rudo (12): Fix various shellcheck findings kdump-lib: make is_zstd_command_available more generic kdump-lib: simplify _get_kdump_kernel_version kdumpctl: drop _get_current_running_kernel_path kdumpctl: drop condrestart subcommand kdumpctl: simplify _update_kernel_cmdline kdumpctl: Prevent option --fadump on non-PPC in reset_crashkernel kdumpctl: Stop updating grub config in reset_crashkernel kdump.conf: Remove option override_resettable spec: Clean up handling of dracut files spec: Silence unversioned Obsolete warning kdump-lib: Harden _crashkernel_add
gen-kdump-conf.sh | 6 -- kdump-lib.sh | 175 ++++++++++++++++++++---------- kdump.conf.5 | 8 -- kdumpctl | 236 +++++++++++++---------------------------- kexec-tools.spec | 50 ++++----- mkdumprd | 57 +--------- mkfadumprd | 2 +- spec/kdump-lib_spec.sh | 38 ++++--- 8 files changed, 235 insertions(+), 337 deletions(-)
-- 2.40.0
Hi Pingfan,
On Tue, 25 Jul 2023 10:55:36 +0800 Pingfan Liu piliu@redhat.com wrote:
Hi Philipp,
Glad to see these chores are done. They make the code neat.
I only have a minor disagreement on the commit log of [10/12].
Yeah, after double checking I've noticed that that commit message is wrong. Please see my reply for the patch.
For the rest of the series,
Reviewed-by: Pingfan Liu piliu@redhat.com
Thanks for the review! Philipp
On Fri, Jul 21, 2023 at 10:24 PM Philipp Rudo prudo@redhat.com wrote:
Hi everybody,
this series got spit from the CLI rework series as they are independent and can be merged earlier to reduce the overall series size. While most of the patches were part of the rfc v1 of the CLI rework series patches 9-12 are new.
@Pingfan: I've dropped your Reviewed-by from patch 1 as I've added two more fixes to the patch. But I've kept it for patch 2&4 where I've updated the commit message and patch 3 where I've dropped the definition of _is_valid_kver as after the rebase an identical function already existed. Hope that is ok with you.
To simplify review I've also pushed the series to branch cleanup on https://src.fedoraproject.org/forks/prudo/rpms/kexec-tools.git.
Thanks Philipp
rfc -> v1:
- Rebased to latest rawhide
- Patch 1 added two more fixes.
- Patch 2,4&8 improved the commit message
- Patch 3 dropped definition of _is_valid_kver as it was already present on latest rawhide
- Added new patches 9-12
Philipp Rudo (12): Fix various shellcheck findings kdump-lib: make is_zstd_command_available more generic kdump-lib: simplify _get_kdump_kernel_version kdumpctl: drop _get_current_running_kernel_path kdumpctl: drop condrestart subcommand kdumpctl: simplify _update_kernel_cmdline kdumpctl: Prevent option --fadump on non-PPC in reset_crashkernel kdumpctl: Stop updating grub config in reset_crashkernel kdump.conf: Remove option override_resettable spec: Clean up handling of dracut files spec: Silence unversioned Obsolete warning kdump-lib: Harden _crashkernel_add
gen-kdump-conf.sh | 6 -- kdump-lib.sh | 175 ++++++++++++++++++++---------- kdump.conf.5 | 8 -- kdumpctl | 236 +++++++++++++---------------------------- kexec-tools.spec | 50 ++++----- mkdumprd | 57 +--------- mkfadumprd | 2 +- spec/kdump-lib_spec.sh | 38 ++++--- 8 files changed, 235 insertions(+), 337 deletions(-)
-- 2.40.0
-
Philipp Rudo -
Pingfan Liu