Hi Philipp,
On Mon, Mar 04, 2024 at 05:06:32PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Fri, 1 Mar 2024 18:00:31 +0800 Coiby Xu coxu@redhat.com wrote:
This patch upstreams the to-be-split-out kdump-utils to https://github.com/rhkdump/kdump-utils. And it also simplify the .spec file by putting the installation logic into a Makefile.
Can you please explain to which repo this patch shall be added. I found that the github repo already has this patch applied but it is still missing in the fedora repo. I'm a little bit confused because I would expect that when you make the github repo the upstream for the fedora repo you also remove all the scripts/configs from the fedora repo as they are now fetched from github.
Furthermore, I noticed that while you add the new kdump-utils.spec you don't remove the corresponding lines from the kexec-tools.spec. Won't that cause a conflict as kdump-utils and kexec-tools try to add the same files?
Finally, I noticed that the github repo still contains the kexec-tools.spec and backported patches. Can't they be removed when the repo is only meant to be upstream for our fedora specific tooling?
Thanks for raising these concerns! All of them are valid and will be addressed later. Sorry I should have explained why I sent this RFC patch. This patch isn't supposed to be applied now. I mainly sent this patch to see if there is any concern on the implementation of upstreaming kdump-utils. So to make it easier for review, I left out any work on cleaning up current kexec-tools.sepc and related files. Besides, there is an ongoing effort [1] to first make kdump-utils an subpackage as a way to break the whole work of splitting kexec-tools into smaller chunks to make it more reviewable for Carl.
[1] https://src.fedoraproject.org/rpms/kexec-tools/pull-request/22
Cc: Philipp Rudo prudo@redhat.com Cc: Carl George carl@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com
COPYING | 339 +++++++++++++++++++++++++++++++++++++++++++++++ Makefile | 85 ++++++++++++ kdump-utils.spec | 133 +++++++++++++++++++ 3 files changed, 557 insertions(+) create mode 100644 COPYING create mode 100644 Makefile create mode 100644 kdump-utils.spec
diff --git a/COPYING b/COPYING new file mode 100644 index 00000000..d159169d --- /dev/null +++ b/COPYING @@ -0,0 +1,339 @@
GNU GENERAL PUBLIC LICENSEVersion 2, June 1991The spec file says
+License: GPL-2.0-only AND LGPL-2.1-or-later
Do we need to ship the LGPL 2.1 license as well? If not, could we drop the license altogether?
I don't know if we need to ship LGPL 2.1. But as found by Carl, /usr/lib/dracut/modules.d/99kdumpbase/{kdump-capture.service,kdump-emergency.target} use this license.
[...]
diff --git a/Makefile b/Makefile new file mode 100644 index 00000000..ce05afa7 --- /dev/null +++ b/Makefile @@ -0,0 +1,85 @@ +prefix ?= /usr +libdir ?= ${prefix}/lib +datadir ?= ${prefix}/share +pkglibdir ?= ${libdir}/kdump +sysconfdir ?= /etc +bindir ?= ${prefix}/bin +sbindir ?= ${prefix}/sbin +mandir ?= ${prefix}/share/man +localstatedir ?= /var +sharedstatedir ?= /var/lib +udevrulesdir ?= ${libdir}/udev/rules.d +systemdsystemunitdir ?= ${libdir}/systemd/system/ +ARCH ?= $(shell uname -m) +dracutmoddir = $(DESTDIR)${libdir}/dracut/modules.d +kdumpbasemoddir = $(dracutmoddir)/99kdumpbase
+dracut-modules:
- mkdir -p $(dracutmoddir)
- mkdir -p -m755 $(kdumpbasemoddir)
- install -m 755 dracut-kdump.sh $(kdumpbasemoddir)/kdump.sh
- install -m 755 dracut-module-setup.sh $(kdumpbasemoddir)/module-setup.sh
- install -m 755 dracut-monitor_dd_progress $(kdumpbasemoddir)/monitor_dd_progress.sh
- install -m 644 dracut-kdump-emergency.service $(kdumpbasemoddir)/kdump-emergency.service
- install -m 644 dracut-kdump-capture.service $(kdumpbasemoddir)/kdump-capture.service
- install -m 644 dracut-kdump-emergency.target $(kdumpbasemoddir)/kdump-emergency.target
^^^^^ Unnecessary tab
- mkdir -p -m755 $(dracutmoddir)/99earlykdump
- install -m 755 dracut-early-kdump.sh $(dracutmoddir)/99earlykdump/kdump.sh
- install -m 755 dracut-early-kdump-module-setup.sh $(dracutmoddir)/99earlykdump/kdump-module-setup.sh
^^^^^ Unnecessary tab
Thanks for catching this issue!
+ifneq (,$(filter $(ARCH),ppc64 ppc64le))
I find the use of ifneq pretty confusing when you want to include the lines when ARCH matches one of the given ones. I'm afraid this can lead to bugs like the one below that easily could be avoided. Personally I would use
ifeq ($(filter ppcle64 ppc64, $(ARCH)), $(ARCH))
Although ifneq is more succinct, ifeq is more readable. I'll go for it. Thanks for coming up with a better idea!
- mkdir -p -m755 $(dracutmoddir)/99zz-fadumpinit
- install -m 755 dracut-fadump-init-fadump.sh $(dracutmoddir)/99zz-fadumpinit/init-fadump.sh
- install -m 755 dracut-fadump-module-setup.sh $(dracutmoddir)/99zz-fadumpinit/module-setup.sh
+endif
+kdump-conf: gen-kdump-conf.sh
- ./gen-kdump-conf.sh $(ARCH) > kdump.conf
+kdump-sysconfig: gen-kdump-sysconfig.sh
- ./gen-kdump-sysconfig.sh $(ARCH) > kdump.sysconfig
+manpages:
- install -D -m 644 mkdumprd.8 kdumpctl.8 -t $(DESTDIR)$(mandir)/man8
- install -D -m 644 kdump.conf.5 $(DESTDIR)$(mandir)/man5/kdump.conf.5
+install: dracut-modules kdump-conf kdump-sysconfig manpages
- mkdir -p $(DESTDIR)$(pkglibdir)
- mkdir -p -m755 $(DESTDIR)$(sysconfdir)/kdump/pre.d
- mkdir -p -m755 $(DESTDIR)$(sysconfdir)/kdump/post.d
- mkdir -p -m755 $(DESTDIR)$(localstatedir)/crash
- mkdir -p -m755 $(DESTDIR)$(udevrulesdir)
- mkdir -p -m755 $(DESTDIR)$(sharedstatedir)/kdump
^^^^^ Unnecessary tab
- install -D -m 755 kdumpctl $(DESTDIR)$(bindir)/kdumpctl
- install -D -m 755 mkdumprd $(DESTDIR)$(sbindir)/mkdumprd
^trailing space
The above issues will be fixed, thanks!
- install -D -m 644 kdump.conf $(DESTDIR)$(sysconfdir)
- install -D -m 644 kdump.sysconfig $(DESTDIR)$(sysconfdir)/sysconfig/kdump
- install -D -m 755 kdump-lib.sh kdump-lib-initramfs.sh kdump-logger.sh -t $(DESTDIR)$(pkglibdir)
+ifneq (,$(filter $(ARCH),ppc64 ppc64le))
- install -m 755 mkfadumprd $(DESTDIR)$(sbindir)
- install -m 755 kdump-migrate-action.sh kdump-restart.sh -t $(DESTDIR)$(pkglibdir)
+endif
+ifneq ($(ARCH),s390x)
- install -m 755 kdump-udev-throttler $(DESTDIR)$(udevrulesdir)/../kdump-udev-throttler
+endif
+ifneq (,$(filter $(ARCH),s390x ppc64 ppc64le))
^^^^^ ifeq? 98-kexec.rules should be included for all arches except the given ones, shouldn't it.
In general, why don't you use a else-if here instead, i.e.
ifeq ($(ARCH), s390x) # For s390x the ELF header is created in the kdump kernel and therefore kexec # udev rules are not required else ifeq ($(filter ppcle64 ppc64, $(ARCH)), $(ARCH)) install -m 644 98-kexec.rules.ppc64 $(DESTDIR)$(udevrulesdir)/98-kexec.rules install -m 755 -D %{SOURCE37} $(DESTDIR)$(libdir)/kernel/install.d/60-fadump.install else install -m 644 98-kexec.rules $(DESTDIR)$(udevrulesdir)/98-kexec.rules endif
Personally I find this much better readable.
Yes, it's better. I'll apply it. Thanks!
- # For s390x the ELF header is created in the kdump kernel and therefore kexec
- # udev rules are not required
- install -m 644 98-kexec.rules $(DESTDIR)$(udevrulesdir)/98-kexec.rules
+endif
+ifneq (,$(filter $(ARCH),ppc64 ppc64le))
- install -m 644 98-kexec.rules.ppc64 $(DESTDIR)$(udevrulesdir)/98-kexec.rules
- install -m 755 -D %{SOURCE37} $(DESTDIR)$(libdir)/kernel/install.d/60-fadump.install
+endif
- install -D -m 644 kdump.service $(DESTDIR)$(systemdsystemunitdir)/kdump.service
- install -m 755 -D kdump-dep-generator.sh $(DESTDIR)$(libdir)/systemd/system-generators/kdump-dep-generator.sh
- install -m 755 -D 60-kdump.install $(DESTDIR)$(libdir)/kernel/install.d/60-kdump.install
- install -m 755 -D 92-crashkernel.install $(DESTDIR)$(libdir)/kernel/install.d/92-crashkernel.install
diff --git a/kdump-utils.spec b/kdump-utils.spec new file mode 100644 index 00000000..c479376b --- /dev/null +++ b/kdump-utils.spec @@ -0,0 +1,133 @@ +Name: kdump-utils +Version: 1.0.42 +Release: 1%{?dist} +Summary: Kernel crash dump collection utilities
+License: GPL-2.0-only AND LGPL-2.1-or-later +URL: https://github.com/rhkdump/kdump-utils +Source0: https://github.com/rhkdump/kdump-utils/archive/v%%7Bversion%7D/%%7Bname%7D-%...
+%ifarch ppc64 ppc64le +Requires(post): servicelog +Recommends: keyutils +%endif +Requires(pre): coreutils +Requires(pre): sed +Requires: kexec-tools >= 2.0.28-5 +Requires: makedumpfile +Requires: dracut >= 058 +Requires: dracut-network >= 058 +Requires: dracut-squash >= 058 +Requires: ethtool +Requires: util-linux +# Needed for UKI support +Recommends: binutils +Recommends: grubby +Recommends: hostname +BuildRequires: systemd-rpm-macros
BuildRequires: make?
With the Makefile added
Nice catch! I didn't realize this mistake until Fedora Copr complained about it.
+%ifnarch s390x +Requires: systemd-udev%{?_isa} +%endif +%description +kdump-utils is reponsible for collecting the crash kernel dump. It builds and +loads the kdump initramfs so when a kernel crashes, the system will boot the +kdump kernel and initramfs to save the colletecd crash kernel dump to specified +target.
+%prep +%autosetup
+%install
Unnecessary newline
Will be removed, thanks!
Thanks Philipp