Hi Coiby,
looks good to me. I've got a small comment on the implementation below. But
that is not blocking this patch to move forward IMO.
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
Furthermore I had an other look at abrt-addon-vmcore. And while I must admit
that I had a major brain fart end of last week it looks like it really needs
kdump.service to be running. But only so kdumpctl sets the correct selinux
labels to the files in /var/crash. All in all abrt-addon-vmcore seems pretty
broken to me since 071ea2a ("kdumpctl: bail out earlier in case of no reserved
memory").
Anyway, my head is spinning right now. Let's discuss this tomorrow, when
(hopefully) I can think straight again.
On Mon, 16 Oct 2023 15:57:39 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=2243068
Currently, when kexec-tools is installed, the kernel will automatically
have the crashkernel parameter set up. In the case where users only want
the kexec reboot feature, this is not what users want as a 1G-RAM system
will lose 192M memory. Considering Fedora's systemd preset policy has
kdump.service disabled and RHEL' has kdump.service enabled, this patch
makes kexec-tools only reset crashkernel when kdump.service is enabled.
Reported-by: Chris Murphy <bugzilla(a)colorremedies.com>
Cc: Philipp Rudo <prudo(a)redhat.com>
Cc: Adam Williamson <awilliam(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdumpctl | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl
index b6d5994d..01433e28 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1649,6 +1649,10 @@ reset_crashkernel_for_installed_kernel()
_update_crashkernel "$_installed_kernel"
}
+_should_reset_crashkernel() {
+ [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]] && systemctl
is-enabled kdump &> /dev/null
+}
I'm don't really like the check for auto_reset_crashkernel != no. Reason is
that it defaults to 'yes' even when something other than the required yes/no is
given. Changing the check to auto_reset_crashkernel == yes is better in my
opinion. (One of my patches for the cli rework sneaks this change in without
mentioning but it's probably better to make this change on its own so it's
properly documented).
Thanks
Philipp
+
main()
{
# Determine if the dump mode is kdump or fadump
@@ -1715,12 +1719,12 @@ main()
reset_crashkernel "$@"
;;
_reset-crashkernel-after-update)
- if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then
+ if _should_reset_crashkernel; then
reset_crashkernel_after_update
fi
;;
_reset-crashkernel-for-installed_kernel)
- if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then
+ if _should_reset_crashkernel; then
reset_crashkernel_for_installed_kernel "$2"
fi
;;