Hi Coiby,
I'm not convinced the patch will work as you intend. Please see below.
On Fri, 1 Jul 2022 15:39:20 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
Resolves: bz2089871
Currently, kexec-tools can't be updated using virt-customize because
older version of kdumpctl can't acquire instance lock for the
get-default-crashkernel subcommand. The reason is /var/lock is linked to
/run/lock however /run doesn't exist in the case of virt-customize.
This patch fixes this problem by creating /run/lock before creating the
lock file.
Note
1. The lock file is now created in /run/lock instead of /var/run/lock since
Fedora has adopted adopted /run [2] since F15.
2. %pre scriptlet now always return success since package update won't
be blocked
[1]
https://fedoraproject.org/wiki/Features/var-run-tmpfs
Fixes: 0adb0f4 ("try to reset kernel crashkernel when kexec-tools updates the
default crashkernel value")
Reported-by: Nicolas Hicher <nhicher(a)redhat.com>
Suggested-by: Laszlo Ersek <lersek(a)redhat.com>
Signed-off-by: Coiby Xu <coxu(a)redhat.com>
---
kdumpctl | 18 +++++++++++++++++-
kexec-tools.spec | 2 ++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/kdumpctl b/kdumpctl
index 6188d47..439276d 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -47,11 +47,27 @@ if ! dlog_init; then
exit 1
fi
+# rmdir /run/lock if it's created by kdumpctl
+clean_up_run_lock()
+{
+ if [[ $(echo /run/lock/*) == /run/lock/kdump ]]; then
How does this check verify that /run/lock was created by kdumpctl? The
way I see it it only returns true when /run/lock/kdump exists and is
the only file in the directory. But that doesn't necessarily mean that
the directory was created by kdumpctl.
+ rm /run/lock/kdump
+ rmdir /run/lock
In the commit message you say that for virt-customize /run does not
exist but you are only removing /run/lock and /run/lock/kdump but
not /run. With the same argument the call to
mkdir /run/lock
will fail as /run does not exist.
Is the commit message correct?
+ fi
+}
+
+trap clean_up_run_lock EXIT
I find it a little bit confusing to have code that is executed in
between all the function definitions. Personally I would prefer to move
this line either to the bottom of the file (after the definition of
main) or to the place where you create the directory in
single_instance_lock. Or maybe use a completely different approach...
+
single_instance_lock()
{
local rc timeout=5
- if ! exec 9> /var/lock/kdump; then
+ # when updating package using virt-customize, /run/lock doesn't exist
+ if [[ ! -d /run/lock ]]; then
+ mkdir /run/lock
+ fi
+
+ if ! exec 9> /run/lock/kdump; then
... and instead of creating /run/lock simply move the lock file to a
place that we know exists when using virt-customize, like /tmp (if that
exists...). With that approach you could avoid the trap and cleanup
altogether and the patch would simplify to something like
local lockfile
if [[ -d /run/lock ]]; then
lockfile=/run/lock/kdump
else
lockfile=/tmp/kdump.lock
fi
if ! exec 9> $lockfile; then
[...]
Thanks
Philipp
derror "Create file lock failed"
exit 1
fi
diff --git a/kexec-tools.spec b/kexec-tools.spec
index 9c6ea8d..b66f97e 100644
--- a/kexec-tools.spec
+++ b/kexec-tools.spec
@@ -269,6 +269,8 @@ if ! grep -qs "ostree" /proc/cmdline && [ $1 == 2 ]
&& grep -q get-default-crash
kdumpctl get-default-crashkernel fadump > /tmp/old_default_crashkernel_fadump
2>/dev/null
%endif
fi
+# don't block package update
+:
%post
# Initial installation