Hi Philipp,

Welcome back:) Thanks for reviewing the patch!

On Wed, Jul 20, 2022 at 05:49:15PM +0200, Philipp Rudo wrote:

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@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@redhat.com Suggested-by: Laszlo Ersek lersek@redhat.com Signed-off-by: Coiby Xu coxu@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.

Yeah, it's better to explicitly state /run/lock/kdump is the only file in /run/lock.

...

• 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?

Sorry, I meant /run/lock doesn't exit.

...

• 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

Yes, moving a lock file to another place is a smarter idea! Previously, I choose to delete /run/lock/kdump and /run/lock is because /run/lock/kdump persists and it's visible after the system boots. If we create /tmp/kdump.lock in virt-customize, the file still persists but it's invisible after the system boots. Considering there is already /tmp/builder.log, I think a new /tmp/kdump.lock is also harmless.

Thanks Philipp

Hi Coiby,

looks good to me. Thanks a lot!

Reviewed-by: Philipp Rudo prudo@redhat.com

On Thu, 21 Jul 2022 18:49:09 +0800 Coiby Xu coxu@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 which however doesn't exist in the case of virt-customize.

This patch fixes this problem by using /tmp/kdump.lock as the lock file if /run/lock doesn't exist.

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@redhat.com Suggested-by: Laszlo Ersek lersek@redhat.com Suggested-by: Philipp Rudo prudo@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com

---

v2

• use /tmp/kdump.lock as the lock file if /run/lock doesn't exist [Philipp]

---

kdumpctl | 11 +++++++++-- kexec-tools.spec | 2 ++ 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 6188d47..29bf533 100755 --- a/kdumpctl +++ b/kdumpctl @@ -49,9 +49,16 @@ fi

single_instance_lock() {

• local rc timeout=5

• local rc timeout=5 lockfile

• if ! exec 9> /var/lock/kdump; then

• if [[ -d /run/lock ]]; then

• lockfile=/run/lock/kdump
  

• else

• # when updating package using virt-customize, /run/lock doesn't exist
  

• lockfile=/tmp/kdump.lock
  

• fi
• if ! exec 9> $lockfile; then 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