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(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.
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
--
Best regards,
Coiby