[RESEND PATCH v2 1/4] update kernel crashkernel in posttrans RPM scriptlet when updating kexec-tools

[Coiby Xu]

When doing in-place upgrading using leapp on x86_64, kdumpcl can't
acquire instance lock when running in %post RPM scriplet on x86_64,
  localhost upgrade[1306]: /bin/kdumpctl: line 49: /var/lock/kdump: No such file or directory
  localhost upgrade[1306]: kdump: Create file lock failed

and running "touch /var/lock/dkump" also fails with
"No such file or directory". Thus kdumpctl can't be run in %post
scriptlet. But kdumpctl can be run in %posttrans RPM scriplet.

Besides, it's better to update crashkernel after the kernel has been
updated. So let's update kernel crashkernel in the %posttrans
scriptlet which will be run in the end of a transaction i.e. after
the kernel has been updated.

Note for %posttrans scriptlet, "$1 == 1" means both installing a new
package and upgrading a package.

[1] https://github.com/apptainer/singularity/issues/2386#issuecomment-474747054

Reported-by: Jie Li 
Signed-off-by: Coiby Xu 
---
 kexec-tools.spec | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/kexec-tools.spec b/kexec-tools.spec
index ac134da..6b7ef14 100644
--- a/kexec-tools.spec
+++ b/kexec-tools.spec
@@ -305,19 +305,6 @@ then
    mv /etc/sysconfig/kdump.new /etc/sysconfig/kdump
 fi

-# try to reset kernel crashkernel value to new default value when upgrading
-# the package
-if ! grep -qs "ostree" /proc/cmdline && [ $1 == 2 ]; then
-  kdumpctl reset-crashkernel-after-update
-  rm /tmp/old_default_crashkernel 2>/dev/null
-%ifarch ppc64 ppc64le
-  rm /tmp/old_default_crashkernel_fadump 2>/dev/null
-%endif
-  # dnf would complain about the exit code not being 0. To keep it happy,
-  # always return 0
-  :
-fi
-

%postun
 %systemd_postun_with_restart kdump.service
@@ -350,6 +337,21 @@ do
    fi
 done

+%posttrans
+# try to reset kernel crashkernel value to new default value when upgrading
+# the package
+if ! grep -qs "ostree" /proc/cmdline && [ $1 == 1 ]; then
+  kdumpctl reset-crashkernel-after-update
+  rm /tmp/old_default_crashkernel 2>/dev/null
+%ifarch ppc64 ppc64le
+  rm /tmp/old_default_crashkernel_fadump 2>/dev/null
+%endif
+  # dnf would complain about the exit code not being 0. To keep it happy,
+  # always return 0
+  :
+fi
+
+
 %files
 /usr/sbin/kexec
 %ifarch %{ix86} x86_64 ppc64 s390x ppc64le aarch64

[RESEND PATCH v2 2/4] simplify updating GRUB_CMDLINE_LINUX in GRUB_ETC_DEFAULT

[Coiby Xu]

Use sed command group and conditional control to get rid of grep.

Suggested-by: Philipp Rudo 
Signed-off-by: Coiby Xu 
---
 kdumpctl | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 9fd76ac..998ac6e 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1405,19 +1405,22 @@ GRUB_ETC_DEFAULT="/etc/default/grub"
 # $2: new value. If empty, the parameter would be removed
 _update_kernel_cmdline_in_grub_etc_default()
 {
-	local _para=$1 _val=$2 _para_val _regex
+	local _para=$1 _val=$2 _para_val

if [[ -n $_val ]]; then
    	_para_val="$_para=$_val"
    fi

-	_regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$'
-	if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then
-		sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT"
-	elif [[ -n $_para_val ]]; then
-		# If the kernel parameter doesn't exist, put it in the first
-		sed -i -E 's/^(GRUB_CMDLINE_LINUX=")/\1'"$_para_val"' /' "$GRUB_ETC_DEFAULT"
-	fi
+	# If the parameter already exists, replace it with new value.
+	# Otherwise insert it to the end.
+	sed -i -E "/^GRUB_CMDLINE_LINUX=/ {
+	           s/([[:space:]"])crashkernel=[^[:space:]"]*(.*)$/\1${_para_val}\2/;
+	           # t: jump to a label only if a s/// command has succeeded.
+	           #    Since there is no label, it would jump to the end i.e.
+	           #    skip the last command if the parameter already exist
+	           t;
+	           s/"$/ ${_para_val}"/
+          }" "$GRUB_ETC_DEFAULT"
 }

reset_crashkernel()

[RESEND PATCH v2 2/4] simplify updating GRUB_CMDLINE_LINUX in GRUB_ETC_DEFAULT

[Philipp Rudo]

Hi Coiby,

please see my reply to v1

Thanks
Philipp

On Wed, 16 Feb 2022 14:06:03 +0800
Coiby Xu  wrote:
> Use sed command group and conditional control to get rid of grep.
>
> Suggested-by: Philipp Rudo 
> Signed-off-by: Coiby Xu
>
> kdumpctl | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 9fd76ac..998ac6e 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1405,19 +1405,22 @@ GRUB_ETC_DEFAULT="/etc/default/grub"
>  # $2: new value. If empty, the parameter would be removed
>  _update_kernel_cmdline_in_grub_etc_default()
>  {
> local _para=$1 _val=$2 _para_val _regex
>
> local _para=$1 _val=$2 _para_val
>
> if [[ -n $_val ]]; then
> _para_val="$_para=$_val"
> fi
> _regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$'
> if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then
> sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT"
> elif [[ -n $_para_val ]]; then
> # If the kernel parameter doesn't exist, put it in the first
> sed -i -E 's/^(GRUB_CMDLINE_LINUX=")/\1'"$_para_val"' /' "$GRUB_ETC_DEFAULT"
> fi
> # If the parameter already exists, replace it with new value.
> # Otherwise insert it to the end.
> sed -i -E "/^GRUB_CMDLINE_LINUX=/ {
>           s/([[:space:]\"])crashkernel=[^[:space:]\"]*(.*)$/\1${_para_val}\2/;
>           # t: jump to a label only if a s/// command has succeeded.
>           #    Since there is no label, it would jump to the end i.e.
>           #    skip the last command if the parameter already exist
>           t;
>           s/\"$/ ${_para_val}\"/
>      }" "$GRUB_ETC_DEFAULT"
>
> }
>
> reset_crashkernel()

[RESEND PATCH v2 3/4] add a helper function to read kernel cmdline parameter from GRUB_ETC_DEFAULT

[Coiby Xu]

This function would be used to read kernel cmdline parameter like fadump
or crashkernel.

Signed-off-by: Coiby Xu 
---
 kdumpctl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index 998ac6e..42730cb 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1423,6 +1423,14 @@ _update_kernel_cmdline_in_grub_etc_default()
           }" "$GRUB_ETC_DEFAULT"
 }

+# read the kernel command line parameter in default grub conf
+#
+# $1: the name of the kernel command line parameter
+_read_kernel_cmdline_in_grub_etc_default()
+{
+	sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]"]${1}=([^[:space:]"]*).*$/\1/p" "$GRUB_ETC_DEFAULT"
+}
+
 reset_crashkernel()
 {
    local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel _kernels

[RESEND PATCH v2 3/4] add a helper function to read kernel cmdline parameter from GRUB_ETC_DEFAULT

[Philipp Rudo]

Hi Coiby,

On Wed, 16 Feb 2022 14:06:04 +0800
Coiby Xu  wrote:
> This function would be used to read kernel cmdline parameter like fadump
> or crashkernel.
> Signed-off-by: Coiby Xu
>
> kdumpctl | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index 998ac6e..42730cb 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1423,6 +1423,14 @@ _update_kernel_cmdline_in_grub_etc_default()
>            }" "$GRUB_ETC_DEFAULT"
>  }
>
> +# read the kernel command line parameter in default grub conf
> +#
> +# $1: the name of the kernel command line parameter
> +_read_kernel_cmdline_in_grub_etc_default()
> +{
> sed -n -E "s/^GRUB_CMDLINE_LINUX=.*[[:space:]"]${1}=([^[:space:]"]*).*$/\1/p" "$GRUB_ETC_DEFAULT"
>
> +}

Similar to the "set" version this one has a problem with boolean
parameters like e.g. quiet. So how about this

# Get the value for $_para set in /etc/default/grub. If $_para is boolean
# return $_para (if it exist). If it is not set return an empty string.
sed -n "/^GRUB_CMDLINE_LINUX=/ {
        s/.*<$_para=([^[:space:]"]*>).*/\1/p; t;
        s/.*(<$_para>).*/\1/p;
        }" "$GRUB_ETC_DEFAULT"

Furthermore, I think you can merge this patch with patch 4. Personally
I don't think there is a benefit in having this in an extra patch.

Thanks
Philipp
> reset_crashkernel()
>  {
>    local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel _kernels

[RESEND PATCH v2 4/4] try to update the crashkernel in GRUB_ETC_DEFAULT after kexec-tools updates the default crashkernel value

[Coiby Xu]

If GRUB_ETC_DEFAULT use crashkernel=auto or
crashkernel=OLD_DEFAULT_CRASHKERNEL, it should be updated as well.

Signed-off-by: Coiby Xu 
---
 kdumpctl | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index 42730cb..5027f47 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1562,6 +1562,34 @@ reset_crashkernel()
    fi
 }

+# update the crashkernel value in GRUB_ETC_DEFAULT if necessary
+#
+# called by reset_crashkernel_after_update and inherit its array variable
+# _crashkernel_vals
+update_crashkernel_in_grub_etc_default_after_update()
+{
+	local _crashkernel _fadump_val
+	local _dump_mode _old_default_crashkernel _new_default_crashkernel
+
+	_crashkernel=$(_read_kernel_cmdline_in_grub_etc_default crashkernel)
+
+	if [[ -z $_crashkernel ]]; then
+		return
+	fi
+
+	_fadump_val=$(read_kernel_cmdline_in_grub_etc_default fadump)
+	_dump_mode=$(get_dump_mode_by_fadump_val "$_fadump_val")
+
+	_old_default_crashkernel=${_crashkernel_vals[old_${_dump_mode}]}
+	_new_default_crashkernel=${_crashkernel_vals[new_${_dump_mode}]}
+
+	if [[ $_crashkernel == auto ]] ||
+		  [[ $_crashkernel == "$_old_default_crashkernel" &&
+		     $_new_default_crashkernel != "$_old_default_crashkernel" ]]; then
+			_update_kernel_cmdline_in_grub_etc_default crashkernel "$_new_default_crashkernel"
+	fi
+}
+
 # shellcheck disable=SC2154 # false positive when dereferencing an array
 reset_crashkernel_after_update()
 {
@@ -1590,6 +1618,8 @@ reset_crashkernel_after_update()
    		fi
    	fi
    done
+
+	update_crashkernel_in_grub_etc_default_after_update
 }

# read the value of an environ variable from given environ file path

[Philipp Rudo]

Hi Coiby

On Wed, 16 Feb 2022 14:06:02 +0800
Coiby Xu  wrote:
> When doing in-place upgrading using leapp on x86_64, kdumpcl can't
> acquire instance lock when running in %post RPM scriplet on x86_64,
>   localhost upgrade[1306]: /bin/kdumpctl: line 49: /var/lock/kdump: No such file or directory
>   localhost upgrade[1306]: kdump: Create file lock failed
>
> and running "touch /var/lock/dkump" also fails with
> "No such file or directory". Thus kdumpctl can't be run in %post
> scriptlet. But kdumpctl can be run in %posttrans RPM scriplet.
>
> Besides, it's better to update crashkernel after the kernel has been
> updated. So let's update kernel crashkernel in the %posttrans
> scriptlet which will be run in the end of a transaction i.e. after
> the kernel has been updated.
>
> Note for %posttrans scriptlet, "$1 == 1" means both installing a new
> package and upgrading a package.
>
> [1] https://github.com/apptainer/singularity/issues/2386#issuecomment-474747054
>
> Reported-by: Jie Li 
> Signed-off-by: Coiby Xu

this patch looks good to me

Reviewed-by: Philipp Rudo
> kexec-tools.spec | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/kexec-tools.spec b/kexec-tools.spec
> index ac134da..6b7ef14 100644
> --- a/kexec-tools.spec
> +++ b/kexec-tools.spec
> @@ -305,19 +305,6 @@ then
>    mv /etc/sysconfig/kdump.new /etc/sysconfig/kdump
>  fi
>
> -# try to reset kernel crashkernel value to new default value when upgrading
> -# the package
> -if ! grep -qs "ostree" /proc/cmdline && [ $1 == 2 ]; then
> kdumpctl reset-crashkernel-after-update
> rm /tmp/old_default_crashkernel 2>/dev/null
>
> -%ifarch ppc64 ppc64le
> rm /tmp/old_default_crashkernel_fadump 2>/dev/null
>
> -%endif
> # dnf would complain about the exit code not being 0. To keep it happy,
> # always return 0
> :
>
> -fi
>
> %postun
>  %systemd_postun_with_restart kdump.service
> @@ -350,6 +337,21 @@ do
>    fi
>  done
>
> +%posttrans
> +# try to reset kernel crashkernel value to new default value when upgrading
> +# the package
> +if ! grep -qs "ostree" /proc/cmdline && [ $1 == 1 ]; then
> kdumpctl reset-crashkernel-after-update
> rm /tmp/old_default_crashkernel 2>/dev/null
>
> +%ifarch ppc64 ppc64le
> rm /tmp/old_default_crashkernel_fadump 2>/dev/null
>
> +%endif
> # dnf would complain about the exit code not being 0. To keep it happy,
> # always return 0
> :
>
> +fi
>
> %files
>  /usr/sbin/kexec
>  %ifarch %{ix86} x86_64 ppc64 s390x ppc64le aarch64

[Coiby Xu]

Hi Philipp,

On Thu, Feb 17, 2022 at 06:33:19PM +0100, Philipp Rudo wrote:
> Hi Coiby
>
> On Wed, 16 Feb 2022 14:06:02 +0800
> Coiby Xu  wrote:
> > When doing in-place upgrading using leapp on x86_64, kdumpcl can't
> > acquire instance lock when running in %post RPM scriplet on x86_64,
> >   localhost upgrade[1306]: /bin/kdumpctl: line 49: /var/lock/kdump: No such file or directory
> >   localhost upgrade[1306]: kdump: Create file lock failed
> >
> > and running "touch /var/lock/dkump" also fails with
> > "No such file or directory". Thus kdumpctl can't be run in %post
> > scriptlet. But kdumpctl can be run in %posttrans RPM scriplet.
> >
> > Besides, it's better to update crashkernel after the kernel has been
> > updated. So let's update kernel crashkernel in the %posttrans
> > scriptlet which will be run in the end of a transaction i.e. after
> > the kernel has been updated.
> >
> > Note for %posttrans scriptlet, "$1 == 1" means both installing a new
> > package and upgrading a package.
> >
> > [1] https://github.com/apptainer/singularity/issues/2386#issuecomment-474747054
> >
> > Reported-by: Jie Li 
> > Signed-off-by: Coiby Xu
>
> this patch looks good to me
>
> Reviewed-by: Philipp Rudo

This patch has been merged alone, thanks for reviewing it!
> > kexec-tools.spec | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/kexec-tools.spec b/kexec-tools.spec
> > index ac134da..6b7ef14 100644
> > --- a/kexec-tools.spec
> > +++ b/kexec-tools.spec
> > @@ -305,19 +305,6 @@ then
> >    mv /etc/sysconfig/kdump.new /etc/sysconfig/kdump
> >  fi
> >
> > -# try to reset kernel crashkernel value to new default value when upgrading
> > -# the package
> > -if ! grep -qs "ostree" /proc/cmdline && [ $1 == 2 ]; then
> > kdumpctl reset-crashkernel-after-update
> > rm /tmp/old_default_crashkernel 2>/dev/null
> >
> > -%ifarch ppc64 ppc64le
> > rm /tmp/old_default_crashkernel_fadump 2>/dev/null
> >
> > -%endif
> > # dnf would complain about the exit code not being 0. To keep it happy,
> > # always return 0
> > :
> >
> > -fi
> >
> > %postun
> >  %systemd_postun_with_restart kdump.service
> > @@ -350,6 +337,21 @@ do
> >    fi
> >  done
> >
> > +%posttrans
> > +# try to reset kernel crashkernel value to new default value when upgrading
> > +# the package
> > +if ! grep -qs "ostree" /proc/cmdline && [ $1 == 1 ]; then
> > kdumpctl reset-crashkernel-after-update
> > rm /tmp/old_default_crashkernel 2>/dev/null
> >
> > +%ifarch ppc64 ppc64le
> > rm /tmp/old_default_crashkernel_fadump 2>/dev/null
> >
> > +%endif
> > # dnf would complain about the exit code not being 0. To keep it happy,
> > # always return 0
> > :
> >
> > +fi
> >
> > %files
> >  /usr/sbin/kexec
> >  %ifarch %{ix86} x86_64 ppc64 s390x ppc64le aarch64