On Mon, Oct 16, 2023 at 06:25:58PM +0200, Philipp Rudo wrote:
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>
The patch has been merged, thanks for reviewing the patch!
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.
Yes, I agree we need to do something to abrt-addon-vmcore. For example,
abrt-vmcore.service depends on kdump. But only abrt-vmcore is enabled in
the systemd policy and kdump.service is not enabled currently.
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
> ;;
--
Best regards,
Coiby