Re: [PATCH] Split kexec-tools into kexec-tools, kdump-utils and makedumpfile

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 11, 2023 at 06:34:05PM +0200, Philipp Rudo wrote:
Hi Coiby,

Hi Philipp,


I finally found time to look at your patch. All in all it looks good to
me. I've also built the new three packages on a fresh F38 with only
rpmdevtools installed that lead to some problems in the new
makedumpfile.spec and kdump-utils.spec. But those are only minor and
can be fixed easily (see below). With those nits fixed the files
contained in the new three rpms were identical to the old kexec-tools
(modulo the obvious changes like moving the docs from kexec-tools to
kdump-utils).

Thanks for taking time to carefully review the patch!


On Thu, 24 Aug 2023 15:45:05 +0800
Coiby Xu <coxu@xxxxxxxxxx> wrote:

Related: https://bugzilla.redhat.com/show_bug.cgi?id=2121912

This patch splits current kexec-tools into three packages kexec-tools,
kdump-utils and makedumpfile.
 - the new kexec-tools is the same to the upstream kexec-tools which
   provides the kexec and vmcore-dmesg binaries
 - kdump-utils is responsible for building the kdump initrd and loading
   it via the kexec binary
 - makedumpfile is responsible for making a small dumpfile of vmcore

It's desirable to make this package modular because 1) now there is a
growing user base to use the kexec reboot 2) packaging makedumpfile and
kexec-tools alone will reduce the maintenance work.

"dnf repoquery --whatrequires kexec-tools" shows the following packages
requires kexec-tools,
 - abrt-addon-vmcore
 - anaconda-install-env-deps
 - cockpit-kdump
 - dracut-kiwi-oem-dump
 - realtime-setup
 - retrace-server

For those packages that need kdump, the dependency needs to point to
the new kdump-utils package instead.

Note some small improvements are also made in this change,
- stop mkdir'ing kcp as kcp.c has been gone long time ago
- use autosetup to automatically apply patches
- add the GPL2 LICENSE for kdump-utils (the source code of makedumpfile
  and kexec-tools already contain the license)
- remove unneeded Obsoletes

Cc: fedora-devel@xxxxxxxxxxxxxxxxxxxxxx
Cc: Dusty Mabe <dustymabe@xxxxxxxxxx>
Cc: crash-catcher-owner@xxxxxxxxxxxxxxxxxxxxxx
CC: anaconda-devel@xxxxxxxxxxxxxxxxxxxxxxx
Cc: Martin Pitt <mpitt@xxxxxxxxxx>
Cc: kiwi-images@xxxxxxxxxxxxxxxx
Cc: Clark Williams <williams@xxxxxxxxxx>
Cc: Matěj Grabovský <mgrabovs@xxxxxxxxxx>
Suggested-by: Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx>
Suggested-by: Philipp Rudo <prudo@xxxxxxxxxx>
Signed-off-by: Coiby Xu <coxu@xxxxxxxxxx>
---
v1
 - Zbigniew
	 - Use Provides correctly
	 - use one-per-line for BuildRequires, Requires and etc.
		- don't change kdump.service state unexpectedly
	- further split out makedumpfile [Philipp]
	- s/kdump-tools/kdump-utils to have a unique name [Dave]
---
 COPYING           | 341 ++++++++++++++++++++++++++++++++++++++++++
 kdump-utils.spec  | 297 +++++++++++++++++++++++++++++++++++++
 kexec-tools.spec  | 368 +++-------------------------------------------
 makedumpfile.spec |  79 ++++++++++
 4 files changed, 734 insertions(+), 351 deletions(-)
 create mode 100644 COPYING
 create mode 100644 kdump-utils.spec
 create mode 100644 makedumpfile.spec

diff --git a/COPYING b/COPYING
new file mode 100644
index 00000000..a52b16e4
--- /dev/null
+++ b/COPYING
@@ -0,0 +1,341 @@

[...]

I'm no lawyer so I haven't checked the COPYING ;-)

The COPYING is from the upstream kexec-tools. But I notice that one is
outdated so in next version I use
https://www.gnu.org/licenses/old-licenses/gpl-2.0.txt instead.

diff --git a/kdump-utils.spec b/kdump-utils.spec
new file mode 100644
index 00000000..af10df62
--- /dev/null
+++ b/kdump-utils.spec
@@ -0,0 +1,297 @@
+Name: kdump-utils
+Version: 2.0.26
+Release: 9%{?dist}

In the meantime...

s/2.0.26-9/2.0.27-1/g

Thanks for the reminder! To be more precise, it's 2.0.27-2:)

+Summary: Kernel crash dump collection tools
+
+License: GPL-2.0-only
+Source1: kdumpctl
+Source2: COPYING
+Source3: gen-kdump-sysconfig.sh
+Source4: gen-kdump-conf.sh
+Source7: mkdumprd
+Source12: mkdumprd.8
+Source13: 98-kexec.rules
...

This would be a great time to group this historically grown list into
namespaces. That's some tedious work but I believe it will be worth
it in the long run. Anyway, your the author so it's up to you if you
want to include it or not.

Thanks for the suggestion! I will reply in a separate email as I think
it may need further discussion.


+%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/

Just FYI, I've posted a patch that cleans up this indirect handling of
the dracut files. So there will be some conflict.

Thanks for the info! I've rebased next version onto your cleanup patch
set.


+
+
+%triggerprein -- kexec-tools < 2.0.26-9
+touch %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset
+
+%post
+rm %{_localstatedir}/lib/rpm-state/kexec-tools.no-preset 2>/dev/null && return 0

Just a quick question for my understanding. This kexec-tools.no-preset
file is a workaround to distinguish between a fresh install and an
upgrade from an older kexec-tools. So that we can skip the %post stage
if this is an upgrade. Is that correct?

If so I would appreciate if you could add a short comment ideally with
a deadline when this workaround is no longer needed and can be removes.
That would really help to clean up the spec in the future when there is
no more supported update path from an old kexec-tools package.

+# Initial installation
+%systemd_post kdump.service

Yes, your understanding is correct. But after learning more about
triggerprein, I find this solution doesn't work as expected. For the
next version, I made two changes to make the solution truly work - move "systemd_post ..." to %postrans as %triggerprein is triggered
  after %post
- remove "return 0" as it doesn't make sense for a scriptlet which is
  executed as a script instead of a function

I've also added a comment as suggested to you, thanks!

+then
+	# This is for SGI SN boxes
+	# They require the --noio option to kexec
+	# since they don't support legacy io
+	sed -e's/\(^KEXEC_ARGS.*\)\("$\)/\1 --noio"/' \
+	/etc/sysconfig/kdump > /etc/sysconfig/kdump.new
+	mv /etc/sysconfig/kdump.new /etc/sysconfig/kdump
+fi

Just FYI, I've posted a patch that cleans up this legacy IA64 support.

Thanks, the next version has been rebased.


+
+%postun
+%systemd_postun_with_restart kdump.service
+
+%preun
+%ifarch ppc64 ppc64le
+servicelog_notify --remove --command=/usr/lib/kdump/kdump-migrate-action.sh
+%endif
+%systemd_preun kdump.service
+
+%triggerin -- kernel-kdump
+touch %{_sysconfdir}/kdump.conf

There is no more kernel-kdump. So I believe we can remove this part.

+
+%triggerpostun -- kernel kernel-xen kernel-debug kernel-PAE kernel-kdump

Same here. This should be handled by 60-kdump.install nowadays.

Thanks, the two suggestions have been applied to next version.

-# setup the docs
-cp %{SOURCE10} .
-cp %{SOURCE11} .
-cp %{SOURCE21} .
-cp %{SOURCE26} .
-cp %{SOURCE27} .
-cp %{SOURCE34} .

These lines are missing in the new kdump-utils.spec. This caused a
build breakage with

RPM build errors:
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/kexec-kdump-howto.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/early-kdump-howto.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/fadump-howto.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/kdump-in-cluster-environment.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/live-image-kdump-howto.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/doc/kdump-utils/crashkernel-howto.txt
   File not found: /root/rpmbuild/BUILDROOT/kdump-utils-2.0.26-9.fc38.x86_64/usr/share/licenses/kdump-utils/COPYING

Thanks for catching this problem! Previously, I only tested "fedpkg local" and copr
build. With these lines, it actually caused building failures for
fedpkg.


Alternatively you could create the doc dir and install the files
manually under %install.

The %doc and %license have the ability to track the docs and license in
the database automatically. So the next version I detect the existence
of the COPYING file exists before trying copying the doc files and
license to make it work for both fedpkg and mock build.


diff --git a/makedumpfile.spec b/makedumpfile.spec
new file mode 100644
index 00000000..b80be181
--- /dev/null
+++ b/makedumpfile.spec
@@ -0,0 +1,79 @@
+%global eppic_ver e8844d3793471163ae4a56d8f95897be9e5bd554
+%global eppic_shortver %(c=%{eppic_ver}; echo ${c:0:7})
+Name: makedumpfile
+Version: 1.7.3
+Summary: makedumpfile package
+Release: 1%{?dist}
+
+License: GPL-2.0-only
+URL: https://github.com/makedumpfile/makedumpfile
+Source0: https://github.com/makedumpfile/rhkdump/archive/%{version}/%{name}-%{version}.tar.gz

spectools -g returned with an 404 to me. Did you mean
s/rhkdump/makedumpfile/?

Yes, you are right! Thanks for catching it.


+Source1: https://github.com/lucchouina/eppic/archive/%{eppic_ver}/eppic-%{eppic_shortver}.tar.gz
+# [PATCH] Mark start of 1.7.4 development phase with version 1.7.3++
+# Author: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
+Patch1:  0001-PATCH-Mark-start-of-1.7.4-development-phase-with-ver.patch
+# [PATCH] Add debugging information for DWARF information retrieval
+# Author: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
+Patch2:  0002-PATCH-Add-debugging-information-for-DWARF-informatio.patch
+# [PATCH] Support struct module_memory on Linux 6.4 and later
+# Author: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
+Patch3:  0003-PATCH-Support-struct-module_memory-on-Linux-6.4-and-.patch

Not entirely sure how packit works. But shouldn't those patches be added
to the dist_git with patch as well? At least for me rpmbuild
complained that they are missing.

Yes, these patches are supposed to be dist-git of the new makedumfile.
I'll drop them for now in next version to make it easier for review and
I also need more time to play with packit.


Thanks
Philipp


--
Best regards,
Coiby
_______________________________________________
Anaconda-devel mailing list -- anaconda-devel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to anaconda-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/anaconda-devel@xxxxxxxxxxxxxxxxxxxxxxx
Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue




[Index of Archives]     [Kickstart]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]
  Powered by Linux