[PATCH 1/3] update kernel crashkernel in posttrans RPM scriptlet when updating kexec-tools
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.
Reported-by: Jie Li jieli@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com --- 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
So the regex can be reused to retrieve fadump and crashkernel value.
Signed-off-by: Coiby Xu coxu@redhat.com --- kdumpctl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..cd1dd5b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,6 +1399,11 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" +_construct_grub_etc_default_regex() +{ + echo -n '^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$1"'=([^[:space:]"]*)(.*)$' +} + # modify the kernel command line parameter in default grub conf # # $1: the name of the kernel command line parameter @@ -1411,9 +1416,9 @@ _update_kernel_cmdline_in_grub_etc_default() _para_val="$_para=$_val" fi
- _regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$' + _regex=$(_construct_grub_etc_default_regex "$_para") if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then - sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT" + sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\4/' "$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"
Hi Coiby,
On Tue, 15 Feb 2022 16:47:43 +0800 Coiby Xu coxu@redhat.com wrote:
So the regex can be reused to retrieve fadump and crashkernel value.
Signed-off-by: Coiby Xu coxu@redhat.com
kdumpctl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..cd1dd5b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,6 +1399,11 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" +_construct_grub_etc_default_regex() +{
- echo -n '^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$1"'=([^[:space:]"]*)(.*)$'
+}
# modify the kernel command line parameter in default grub conf # # $1: the name of the kernel command line parameter @@ -1411,9 +1416,9 @@ _update_kernel_cmdline_in_grub_etc_default() _para_val="$_para=$_val" fi
- _regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$'
- _regex=$(_construct_grub_etc_default_regex "$_para") if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT"
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\4/' "$GRUB_ETC_DEFAULT"
I'm not sure.. In my opinion adding groups to a regex should only be done directly where the regex is used. Otherwise you need to check every caller of the function and check if they still use the correct group in their sed call when something changes. But that sounds quite error prone to me.
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
Hi Philipp,
On Tue, Feb 15, 2022 at 01:38:22PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Tue, 15 Feb 2022 16:47:43 +0800 Coiby Xu coxu@redhat.com wrote:
So the regex can be reused to retrieve fadump and crashkernel value.
Signed-off-by: Coiby Xu coxu@redhat.com
kdumpctl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..cd1dd5b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,6 +1399,11 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" +_construct_grub_etc_default_regex() +{
- echo -n '^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$1"'=([^[:space:]"]*)(.*)$'
+}
# modify the kernel command line parameter in default grub conf # # $1: the name of the kernel command line parameter @@ -1411,9 +1416,9 @@ _update_kernel_cmdline_in_grub_etc_default() _para_val="$_para=$_val" fi
- _regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$'
- _regex=$(_construct_grub_etc_default_regex "$_para") if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT"
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\4/' "$GRUB_ETC_DEFAULT"I'm not sure.. In my opinion adding groups to a regex should only be done directly where the regex is used. Otherwise you need to check every caller of the function and check if they still use the correct group in their sed call when something changes. But that sounds quite error prone to me.
Thanks for the suggestion. I will eliminate this risk in v2. Btw, I've implemented the unit tests [1] for this function. But it's better to adopt a less error-prone way.
[1] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools/c/4f7ccd4245ea0420...
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
I notice there could be three issues with the above code, 1. you mean -E instead of "-e", right? 2. If the parameter is right in the beginning i.e. GRUB_CMDLINE_LINUX="crashkerlel=xxM ...", the first '"' would be removed 3. the parameter is always moved to the end. Of course, this is not a real problem as there should be no functional difference.
In v2, I will use the follow code,
# 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"
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
Hi Coiby,
On Wed, 16 Feb 2022 11:11:45 +0800 Coiby Xu coxu@redhat.com wrote:
Hi Philipp,
On Tue, Feb 15, 2022 at 01:38:22PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Tue, 15 Feb 2022 16:47:43 +0800 Coiby Xu coxu@redhat.com wrote:
So the regex can be reused to retrieve fadump and crashkernel value.
Signed-off-by: Coiby Xu coxu@redhat.com
kdumpctl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..cd1dd5b 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,6 +1399,11 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" +_construct_grub_etc_default_regex() +{
- echo -n '^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$1"'=([^[:space:]"]*)(.*)$'
+}
# modify the kernel command line parameter in default grub conf # # $1: the name of the kernel command line parameter @@ -1411,9 +1416,9 @@ _update_kernel_cmdline_in_grub_etc_default() _para_val="$_para=$_val" fi
- _regex='^(GRUB_CMDLINE_LINUX=.*)([[:space:]"])'"$_para"'=[^[:space:]"]*(.*)$'
- _regex=$(_construct_grub_etc_default_regex "$_para") if grep -q -E "$_regex" "$GRUB_ETC_DEFAULT"; then
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\3/' "$GRUB_ETC_DEFAULT"
sed -i -E 's/'"$_regex"'/\1\2'"$_para_val"'\4/' "$GRUB_ETC_DEFAULT"I'm not sure.. In my opinion adding groups to a regex should only be done directly where the regex is used. Otherwise you need to check every caller of the function and check if they still use the correct group in their sed call when something changes. But that sounds quite error prone to me.
Thanks for the suggestion. I will eliminate this risk in v2. Btw, I've implemented the unit tests [1] for this function. But it's better to adopt a less error-prone way.
Thanks!
[1] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools/c/4f7ccd4245ea0420...
Great, it is always good to have more test cases. A few small comments though:
1) For the test cases where you update the crashkernel= value I think it would make sense to not only verify that the line contains the new value but also that it no longer contains the old value.
2) I think you should also add test cases with multiple crashkernel= entries on the commandline because that is where your regex in v2 will fail (see below).
3) I think you should also add a test case that adds/updates a boolean entry (like e.g. 'quiet'). That's an other case where your regex fails (see below).
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
I notice there could be three issues with the above code,
- you mean -E instead of "-e", right?
No, I actually meant -e. I admit it is not strictly necessary but more a habit of mine to always add it.
You can also add -E if you like and use extended regex (and use + instead of +). But in my opinion that doesn't make much of a difference here and is more a matter of taste.
- If the parameter is right in the beginning i.e. GRUB_CMDLINE_LINUX="crashkerlel=xxM ...", the first '"' would be removed
Right... I totally missed that...
- the parameter is always moved to the end. Of course, this is not a real problem as there should be no functional difference.
Yes, for the command I gave above that doesn't make a difference. But can make one as the command line parser in the kernel always takes the last value it finds. So when you don't remove the entry or there are multiple identical entries you have to make sure that the new/updated one is the last on the command line. Otherwise the kernel won't pick the new value.
In v2, I will use the follow code,
# 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/;
^^^^^^^^^^^ $para ?
# 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"
As said above I see two problems with this regex:
1) It will fail when there are multiple entries as it will only update the first match. E.g. consider the line
GRUB_CMDLINE_LINUX="foo=1 foo=3"
after running '_update_kernel_cmdline_in_grub_etc_default foo xxx' it will become
GRUB_CMDLINE_LINUX="foo=xxx foo=3"
which is not what you want.
2) For boolean values it doesn't remove already existing entries so you can end up with multiple once. E.g. consider the line
GRUB_CMDLINE_LINUX="quiet"
after running '_update_kernel_cmdline_in_grub_etc_default quiet' it will become
GRUB_CMDLINE_LINUX="quiet quiet"
It's not a real problem but while we are on it we can make it right ;)
So, how about this command
# Update the command line /etc/default/grub, i.e. # on the lines that start with 'GRUB_CMDLINE_LINUX=' do # 1) remove all occurences of $para=$val # 2) remove all occurences of $para # 3) remove duplicate spaces left over by 1) or 2) # 4) remove space at the beginning of the string left over by 1) or 2) # 5) remove space at the end of the string left over by 1) or 2) # 6) add the new entry for $para at the end of the command line sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/<$_para=[^[:space:]"]*>//g; s/<$_para>//g; s/[[:space:]]{2,}/ /g; s/("+)[[:space:]]{1,}/\1/g; s/[[:space:]]("+){1,}/\1/g; s/"$/ $_para_val"/ }" "$GRUB_ETC_DEFAULT"
Thanks Philipp
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
Hi Philipp,
On Wed, Feb 16, 2022 at 02:20:11PM +0100, Philipp Rudo wrote:
Hi Coiby,
[...]
Thanks for the suggestion. I will eliminate this risk in v2. Btw, I've implemented the unit tests [1] for this function. But it's better to adopt a less error-prone way.
Thanks!
[1] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools/c/4f7ccd4245ea0420...
Great, it is always good to have more test cases. A few small comments though:
- For the test cases where you update the crashkernel= value I think
it would make sense to not only verify that the line contains the new value but also that it no longer contains the old value.
- I think you should also add test cases with multiple crashkernel=
entries on the commandline because that is where your regex in v2 will fail (see below).
- I think you should also add a test case that adds/updates a boolean
entry (like e.g. 'quiet'). That's an other case where your regex fails (see below).
Thanks for the suggestions! I've added new tests with a slight change. The tests would not verify the old value is removed but only make sure the updated entry appear in the end.
Note current implementation doesn't fully support updating boolean as it couldn't tell if it's a boolean parameter. I will state in the comment that the user has to explicitly call say "_update_kernel_cmdline_in_grub_etc_default quiet 1".
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
I notice there could be three issues with the above code,
- you mean -E instead of "-e", right?
No, I actually meant -e. I admit it is not strictly necessary but more a habit of mine to always add it.
You can also add -E if you like and use extended regex (and use + instead of +). But in my opinion that doesn't make much of a difference here and is more a matter of taste.
Thanks for the clarification!
- If the parameter is right in the beginning i.e. GRUB_CMDLINE_LINUX="crashkerlel=xxM ...", the first '"' would be removed
Right... I totally missed that...
- the parameter is always moved to the end. Of course, this is not a real problem as there should be no functional difference.
Yes, for the command I gave above that doesn't make a difference. But can make one as the command line parser in the kernel always takes the last value it finds. So when you don't remove the entry or there are multiple identical entries you have to make sure that the new/updated one is the last on the command line. Otherwise the kernel won't pick the new value.
Thanks for bringing this pitfall to my attention!
In v2, I will use the follow code,
# 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/;
^^^^^^^^^^^ $para ?
Oh, thanks for catching this issue!
# 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"As said above I see two problems with this regex:
- It will fail when there are multiple entries as it will only update
the first match. E.g. consider the line
GRUB_CMDLINE_LINUX="foo=1 foo=3"
after running '_update_kernel_cmdline_in_grub_etc_default foo xxx' it will become
GRUB_CMDLINE_LINUX="foo=xxx foo=3"
which is not what you want.
- For boolean values it doesn't remove already existing entries so you
can end up with multiple once. E.g. consider the line
GRUB_CMDLINE_LINUX="quiet"
after running '_update_kernel_cmdline_in_grub_etc_default quiet' it will become
GRUB_CMDLINE_LINUX="quiet quiet"
It's not a real problem but while we are on it we can make it right ;)
So, how about this command
# Update the command line /etc/default/grub, i.e. # on the lines that start with 'GRUB_CMDLINE_LINUX=' do # 1) remove all occurences of $para=$val # 2) remove all occurences of $para # 3) remove duplicate spaces left over by 1) or 2) # 4) remove space at the beginning of the string left over by 1) or 2) # 5) remove space at the end of the string left over by 1) or 2) # 6) add the new entry for $para at the end of the command line sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/<$_para=[^[:space:]"]*>//g; s/<$_para>//g; s/[[:space:]]{2,}/ /g; s/("+)[[:space:]]{1,}/\1/g; s/[[:space:]]("+){1,}/\1/g; s/"$/ $_para_val"/ }" "$GRUB_ETC_DEFAULT"
Thanks for sharing this regex! I notice two issues, 1) as catched by the unit tests, this regex fails to address the case "ckcrashkernel=220M" i.e. when the given parameter is suffix of another parameter 2) this regex would delete all values for a parameter that allows multiples values specified, e.g. efivar_ssdt efivar_ssdt= [EFI; X86] Name of an EFI variable that contains an SSDT that is to be dynamically loaded by Linux. If there are multiple variables with the same name but with different vendor GUIDs, all of them will be loaded. See Documentation/acpi/ssdt-overlays.txt for details.
After playing with grubby which allows "crashkernel=20M crashkernel=30M crashernel=SPECIFIED_VALUE", I would prefer a simpler solution,
# Update the command line /etc/default/grub, i.e. # on the line that starts with 'GRUB_CMDLINE_LINUX=', # 1) if given parameter appear in the end, replace it with new entry # 2) otherwise, insert the entry in the end sed -i -E "/^GRUB_CMDLINE_LINUX=/ { s/([[:space:]"])${_para}=[^[:space:]"]*[[:space:]]*"$/\1${_para_val}"/; # 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"
Thanks Philipp
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
Hi Coiby,
You are right in many points in you email. But I also had a look at Documentation/admin-guide/kernel-parameters.rst and found some problems in both our suggestions if we want to be completely generic. Especially
1) The kernel ignores everything on the command line after a '--'. So simply adding the the new entry to the end will fail if the cmdline contains a --.
2) If the value for a parameter contains spaces it can be quoted using double quotes, for example param="value with spaces". This will break the [^[:space:]"] regex for the value.
3) Dashes and underscores in the parameter name are equivalent. So some_parameter and some-parameter are identical.
Together with the efivar_ssdt special case this will become pretty messy...
I'm tented at the moment to stick with your original patch for the moment and then work on a proper fix afterwards.
Thanks Philipp
On Thu, 17 Feb 2022 15:32:41 +0800 Coiby Xu coxu@redhat.com wrote:
Hi Philipp,
On Wed, Feb 16, 2022 at 02:20:11PM +0100, Philipp Rudo wrote:
Hi Coiby,
[...]
Thanks for the suggestion. I will eliminate this risk in v2. Btw, I've implemented the unit tests [1] for this function. But it's better to adopt a less error-prone way.
Thanks!
[1] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools/c/4f7ccd4245ea0420...
Great, it is always good to have more test cases. A few small comments though:
- For the test cases where you update the crashkernel= value I think
it would make sense to not only verify that the line contains the new value but also that it no longer contains the old value.
- I think you should also add test cases with multiple crashkernel=
entries on the commandline because that is where your regex in v2 will fail (see below).
- I think you should also add a test case that adds/updates a boolean
entry (like e.g. 'quiet'). That's an other case where your regex fails (see below).
Thanks for the suggestions! I've added new tests with a slight change. The tests would not verify the old value is removed but only make sure the updated entry appear in the end.
Note current implementation doesn't fully support updating boolean as it couldn't tell if it's a boolean parameter. I will state in the comment that the user has to explicitly call say "_update_kernel_cmdline_in_grub_etc_default quiet 1".
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
I notice there could be three issues with the above code,
- you mean -E instead of "-e", right?
No, I actually meant -e. I admit it is not strictly necessary but more a habit of mine to always add it.
You can also add -E if you like and use extended regex (and use + instead of +). But in my opinion that doesn't make much of a difference here and is more a matter of taste.
Thanks for the clarification!
- If the parameter is right in the beginning i.e. GRUB_CMDLINE_LINUX="crashkerlel=xxM ...", the first '"' would be removed
Right... I totally missed that...
- the parameter is always moved to the end. Of course, this is not a real problem as there should be no functional difference.
Yes, for the command I gave above that doesn't make a difference. But can make one as the command line parser in the kernel always takes the last value it finds. So when you don't remove the entry or there are multiple identical entries you have to make sure that the new/updated one is the last on the command line. Otherwise the kernel won't pick the new value.
Thanks for bringing this pitfall to my attention!
In v2, I will use the follow code,
# 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/;
^^^^^^^^^^^ $para ?Oh, thanks for catching this issue!
# 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"As said above I see two problems with this regex:
- It will fail when there are multiple entries as it will only update
the first match. E.g. consider the line
GRUB_CMDLINE_LINUX="foo=1 foo=3"
after running '_update_kernel_cmdline_in_grub_etc_default foo xxx' it will become
GRUB_CMDLINE_LINUX="foo=xxx foo=3"
which is not what you want.
- For boolean values it doesn't remove already existing entries so you
can end up with multiple once. E.g. consider the line
GRUB_CMDLINE_LINUX="quiet"
after running '_update_kernel_cmdline_in_grub_etc_default quiet' it will become
GRUB_CMDLINE_LINUX="quiet quiet"
It's not a real problem but while we are on it we can make it right ;)
So, how about this command
# Update the command line /etc/default/grub, i.e. # on the lines that start with 'GRUB_CMDLINE_LINUX=' do # 1) remove all occurences of $para=$val # 2) remove all occurences of $para # 3) remove duplicate spaces left over by 1) or 2) # 4) remove space at the beginning of the string left over by 1) or 2) # 5) remove space at the end of the string left over by 1) or 2) # 6) add the new entry for $para at the end of the command line sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/<$_para=[^[:space:]"]*>//g; s/<$_para>//g; s/[[:space:]]{2,}/ /g; s/("+)[[:space:]]{1,}/\1/g; s/[[:space:]]("+){1,}/\1/g; s/"$/ $_para_val"/ }" "$GRUB_ETC_DEFAULT"
Thanks for sharing this regex! I notice two issues,
- as catched by the unit tests, this regex fails to address the case "ckcrashkernel=220M" i.e. when the given parameter is suffix of another parameter
- this regex would delete all values for a parameter that allows multiples values specified, e.g. efivar_ssdt efivar_ssdt= [EFI; X86] Name of an EFI variable that contains an SSDT that is to be dynamically loaded by Linux. If there are multiple variables with the same name but with different vendor GUIDs, all of them will be loaded. See Documentation/acpi/ssdt-overlays.txt for details.
After playing with grubby which allows "crashkernel=20M crashkernel=30M crashernel=SPECIFIED_VALUE", I would prefer a simpler solution,
# Update the command line /etc/default/grub, i.e. # on the line that starts with 'GRUB_CMDLINE_LINUX=', # 1) if given parameter appear in the end, replace it with new entry # 2) otherwise, insert the entry in the end sed -i -E "/^GRUB_CMDLINE_LINUX=/ { s/([[:space:]"])${_para}=[^[:space:]"]*[[:space:]]*"$/\1${_para_val}"/; # 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"
Thanks Philipp
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
Hi Philipp,
On Thu, Feb 17, 2022 at 06:59:06PM +0100, Philipp Rudo wrote:
Hi Coiby,
You are right in many points in you email. But I also had a look at Documentation/admin-guide/kernel-parameters.rst and found some problems in both our suggestions if we want to be completely generic. Especially
- The kernel ignores everything on the command line after a '--'. So
simply adding the the new entry to the end will fail if the cmdline contains a --.
- If the value for a parameter contains spaces it can be quoted using
double quotes, for example param="value with spaces". This will break the [^[:space:]"] regex for the value.
- Dashes and underscores in the parameter name are equivalent. So
some_parameter and some-parameter are identical.
Together with the efivar_ssdt special case this will become pretty messy...
Thanks for digging out these cases!
I'm tented at the moment to stick with your original patch for the moment and then work on a proper fix afterwards.
Since in foreseeable future, we are unlikely to encounter these cases. So it's better not to implement something that won't be used. But for the case of multiple values for the same kernel parameter, it's very likely to happen considering the behaviour of grubby. So in v3, I only address this cases and document the unsupported cased found by you.
Thanks Philipp
On Thu, 17 Feb 2022 15:32:41 +0800 Coiby Xu coxu@redhat.com wrote:
Hi Philipp,
On Wed, Feb 16, 2022 at 02:20:11PM +0100, Philipp Rudo wrote:
Hi Coiby,
[...]
Thanks for the suggestion. I will eliminate this risk in v2. Btw, I've implemented the unit tests [1] for this function. But it's better to adopt a less error-prone way.
Thanks!
[1] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools/c/4f7ccd4245ea0420...
Great, it is always good to have more test cases. A few small comments though:
- For the test cases where you update the crashkernel= value I think
it would make sense to not only verify that the line contains the new value but also that it no longer contains the old value.
- I think you should also add test cases with multiple crashkernel=
entries on the commandline because that is where your regex in v2 will fail (see below).
- I think you should also add a test case that adds/updates a boolean
entry (like e.g. 'quiet'). That's an other case where your regex fails (see below).
Thanks for the suggestions! I've added new tests with a slight change. The tests would not verify the old value is removed but only make sure the updated entry appear in the end.
Note current implementation doesn't fully support updating boolean as it couldn't tell if it's a boolean parameter. I will state in the comment that the user has to explicitly call say "_update_kernel_cmdline_in_grub_etc_default quiet 1".
Furthermore the if-block can be simplified to a single sed call, i.e.
sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/[[:space:]"]$para=[^[:space:]"]+//g; s/"$/ $para_val"/ }" $GRUB_ETC_DEFAULT
i.e. on the line that starts with GRUB_CMDLINE_LINUX=, remove all occurrences of $para, then add a new $para_val to the end of the list.
I notice there could be three issues with the above code,
- you mean -E instead of "-e", right?
No, I actually meant -e. I admit it is not strictly necessary but more a habit of mine to always add it.
You can also add -E if you like and use extended regex (and use + instead of +). But in my opinion that doesn't make much of a difference here and is more a matter of taste.
Thanks for the clarification!
- If the parameter is right in the beginning i.e. GRUB_CMDLINE_LINUX="crashkerlel=xxM ...", the first '"' would be removed
Right... I totally missed that...
- the parameter is always moved to the end. Of course, this is not a real problem as there should be no functional difference.
Yes, for the command I gave above that doesn't make a difference. But can make one as the command line parser in the kernel always takes the last value it finds. So when you don't remove the entry or there are multiple identical entries you have to make sure that the new/updated one is the last on the command line. Otherwise the kernel won't pick the new value.
Thanks for bringing this pitfall to my attention!
In v2, I will use the follow code,
# 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/;
^^^^^^^^^^^ $para ?Oh, thanks for catching this issue!
# 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"As said above I see two problems with this regex:
- It will fail when there are multiple entries as it will only update
the first match. E.g. consider the line
GRUB_CMDLINE_LINUX="foo=1 foo=3"
after running '_update_kernel_cmdline_in_grub_etc_default foo xxx' it will become
GRUB_CMDLINE_LINUX="foo=xxx foo=3"
which is not what you want.
- For boolean values it doesn't remove already existing entries so you
can end up with multiple once. E.g. consider the line
GRUB_CMDLINE_LINUX="quiet"
after running '_update_kernel_cmdline_in_grub_etc_default quiet' it will become
GRUB_CMDLINE_LINUX="quiet quiet"
It's not a real problem but while we are on it we can make it right ;)
So, how about this command
# Update the command line /etc/default/grub, i.e. # on the lines that start with 'GRUB_CMDLINE_LINUX=' do # 1) remove all occurences of $para=$val # 2) remove all occurences of $para # 3) remove duplicate spaces left over by 1) or 2) # 4) remove space at the beginning of the string left over by 1) or 2) # 5) remove space at the end of the string left over by 1) or 2) # 6) add the new entry for $para at the end of the command line sed -i -e "/^GRUB_CMDLINE_LINUX=/ { s/<$_para=[^[:space:]"]*>//g; s/<$_para>//g; s/[[:space:]]{2,}/ /g; s/("+)[[:space:]]{1,}/\1/g; s/[[:space:]]("+){1,}/\1/g; s/"$/ $_para_val"/ }" "$GRUB_ETC_DEFAULT"
Thanks for sharing this regex! I notice two issues,
- as catched by the unit tests, this regex fails to address the case "ckcrashkernel=220M" i.e. when the given parameter is suffix of another parameter
- this regex would delete all values for a parameter that allows multiples values specified, e.g. efivar_ssdt efivar_ssdt= [EFI; X86] Name of an EFI variable that contains an SSDT that is to be dynamically loaded by Linux. If there are multiple variables with the same name but with different vendor GUIDs, all of them will be loaded. See Documentation/acpi/ssdt-overlays.txt for details.
After playing with grubby which allows "crashkernel=20M crashkernel=30M crashernel=SPECIFIED_VALUE", I would prefer a simpler solution,
# Update the command line /etc/default/grub, i.e. # on the line that starts with 'GRUB_CMDLINE_LINUX=', # 1) if given parameter appear in the end, replace it with new entry # 2) otherwise, insert the entry in the end sed -i -E "/^GRUB_CMDLINE_LINUX=/ { s/([[:space:]"])${_para}=[^[:space:]"]*[[:space:]]*"$/\1${_para_val}"/; # 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"
Thanks Philipp
With that you need to duplicate parts of the regex when you try read values from the command line. But all in all I think that will be easier to maintain compared to your approach.
Thanks Philipp
If GRUB_ETC_DEFAULT use crashkernel=auto or crashkernel=OLD_DEFAULT_CRASHKERNEL, it should be updated as well.
Signed-off-by: Coiby Xu coxu@redhat.com --- kdumpctl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/kdumpctl b/kdumpctl index cd1dd5b..802e15a 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1556,6 +1556,36 @@ 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_regex _fadump_regex _crashkernel _fadump_val + local _dump_mode _old_default_crashkernel _new_default_crashkernel + + _crashkernel_regex=$(_construct_grub_etc_default_regex crashkernel) + _crashkernel=$(sed -n -E 's/'"$_crashkernel_regex"'/\3/p' "$GRUB_ETC_DEFAULT") + + if [[ -z $_crashkernel ]]; then + return + fi + + _fadump_regex=$(_construct_grub_etc_default_regex fadump) + _fadump_val=$(sed -n -E 's/'"$_fadump_regex"'/\3/p' "$GRUB_ETC_DEFAULT") + _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() { @@ -1584,6 +1614,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
-
Coiby Xu -
Philipp Rudo