When updating the kernel command line in /etc/default/grub, grubby only replaces a given entry if it is the last entry on the command line. In all other cases it appends a new entry to it. This behavior makes it quite likely that there are multiple entries of the same parameter on the command line, e.g. GRUB_CMDLINE_LINUX="crashkernel=110M crashkernel=220M fadump=on crashkernel=330M".
In such an situation _update_kernel_cmdline_in_grub_etc_default only updates/removes the last entry. Which is usually not what you want as the kernel (for crashkernel) takes the last entry it can find.
Thus make sure the case with multiple entries of the same parameter is handled properly by removing all occurrences of given parameter first.
Note 1. sed command group and conditional control has been used to get rid of grep. 2. Fully supporting kernel cmdline as documented in Documentation/admin-guide/kernel-parameters.rst is complex and in foreseeable future a full implementation is not needed. So simply document the unsupported cases instead.
Reported-by: Philipp Rudo prudo@redhat.com Suggested-by: Philipp Rudo prudo@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com --- kdumpctl | 54 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..978aee1 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,25 +1399,49 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" -# modify the kernel command line parameter in default grub conf +# Update a kernel parameter in default grub conf +# +# If a value is specified, it will be inserted in the end. Otherwise it +# would remove given kernel parameter. +# +# Note this function doesn't address the following cases, +# 1. The kernel ignores everything on the command line after a '--'. So +# simply adding 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. +# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. +# 5. Some kernel parameters, e.g. quiet, doesn't have value # # $1: the name of the kernel command line parameter -# $2: new value. If empty, the parameter would be removed -_update_kernel_cmdline_in_grub_etc_default() +# $2: new value. If empty, given parameter would be removed +_update_kernel_arg_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 + # Update the command line /etc/default/grub, i.e. + # on the line that starts with 'GRUB_CMDLINE_LINUX=', + # 1) remove $para=$val if the it's the first arg + # 2) remove all occurences of $para=$val + # 3) insert $_para_val to end + # 4) remove duplicate spaces left over by 1) or 2) or 3) + # 5) remove space at the beginning of the string left over by 1) or 2) or 3) + # 6) remove space at the end of the string left over by 1) or 2) or 3) + sed -i -E "/^GRUB_CMDLINE_LINUX=/ { + s/"${_para}=[^[:space:]"]*/"/g; + s/[[:space:]]+${_para}=[^[:space:]"]*/ /g; + s/"$/ ${_para_val}"/ + s/[[:space:]]+/ /g; + s/(")[[:space:]]+/\1/g; + s/[[:space:]]+(")/\1/g; + }" "$GRUB_ETC_DEFAULT" }
reset_crashkernel() @@ -1496,10 +1520,12 @@ reset_crashkernel() # - set the dump mode as kdump for non-ppc64le cases # - retrieved the default crashkernel value for given dump mode if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then - _update_kernel_cmdline_in_grub_etc_default crashkernel "$_crashkernel" + _update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel" # remove the fadump if fadump is disabled - [[ $_fadump_val == off ]] && _fadump_val="" - _update_kernel_cmdline_in_grub_etc_default fadump "$_fadump_val" + if [[ $_fadump_val == off ]]; then + _fadump_val="" + fi + _update_kernel_arg_in_grub_etc_default fadump "$_fadump_val" fi
# If kernel-path not specified, either
If GRUB_ETC_DEFAULT use crashkernel=auto or crashkernel=OLD_DEFAULT_CRASHKERNEL, it should be updated as well.
Add a helper function to read kernel cmdline parameter from GRUB_ETC_DEFAULT. This function is used to read kernel cmdline parameter like fadump or crashkernel.
Reviewed-by: Philipp Rudo prudo@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com --- kdumpctl | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/kdumpctl b/kdumpctl index 978aee1..3189e98 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1444,6 +1444,16 @@ _update_kernel_arg_in_grub_etc_default() }" "$GRUB_ETC_DEFAULT" }
+# Read the kernel arg in default grub conf. + +# Note reading a kernel parameter that doesn't have a value isn't supported. +# +# $1: the name of the kernel command line parameter +_read_kernel_arg_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 @@ -1577,6 +1587,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_arg_in_grub_etc_default crashkernel) + + if [[ -z $_crashkernel ]]; then + return + fi + + _fadump_val=$(_read_kernel_arg_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_arg_in_grub_etc_default crashkernel "$_new_default_crashkernel" + fi +} + # shellcheck disable=SC2154 # false positive when dereferencing an array reset_crashkernel_after_update() { @@ -1605,6 +1643,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
Hi Coiby,
On Wed, 23 Feb 2022 15:45:46 +0800 Coiby Xu coxu@redhat.com wrote:
When updating the kernel command line in /etc/default/grub, grubby only replaces a given entry if it is the last entry on the command line. In all other cases it appends a new entry to it. This behavior makes it quite likely that there are multiple entries of the same parameter on the command line, e.g. GRUB_CMDLINE_LINUX="crashkernel=110M crashkernel=220M fadump=on crashkernel=330M".
I played around a little with grubby and for me it looks like it has the same behavior you are implementing below, i.e. that it always removes all occurrences of $param= and then append the new $param= at the end. That contradicts what you are writing above. Can you please double check this? Thanks
Code looks good, though.
Reviewed-by: Philipp Rudo prudo@redhat.com
In such an situation _update_kernel_cmdline_in_grub_etc_default only updates/removes the last entry. Which is usually not what you want as the kernel (for crashkernel) takes the last entry it can find.
Thus make sure the case with multiple entries of the same parameter is handled properly by removing all occurrences of given parameter first.
Note
- sed command group and conditional control has been used to get rid of grep.
- Fully supporting kernel cmdline as documented in Documentation/admin-guide/kernel-parameters.rst is complex and in foreseeable future a full implementation is not needed. So simply document the unsupported cases instead.
Reported-by: Philipp Rudo prudo@redhat.com Suggested-by: Philipp Rudo prudo@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com
kdumpctl | 54 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..978aee1 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,25 +1399,49 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" -# modify the kernel command line parameter in default grub conf +# Update a kernel parameter in default grub conf +# +# If a value is specified, it will be inserted in the end. Otherwise it +# would remove given kernel parameter. +# +# Note this function doesn't address the following cases, +# 1. The kernel ignores everything on the command line after a '--'. So +# simply adding 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. +# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. +# 5. Some kernel parameters, e.g. quiet, doesn't have value # # $1: the name of the kernel command line parameter -# $2: new value. If empty, the parameter would be removed -_update_kernel_cmdline_in_grub_etc_default() +# $2: new value. If empty, given parameter would be removed +_update_kernel_arg_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 firstsed -i -E 's/^(GRUB_CMDLINE_LINUX=")/\1'"$_para_val"' /' "$GRUB_ETC_DEFAULT"- fi
- # Update the command line /etc/default/grub, i.e.
- # on the line that starts with 'GRUB_CMDLINE_LINUX=',
- # 1) remove $para=$val if the it's the first arg
- # 2) remove all occurences of $para=$val
- # 3) insert $_para_val to end
- # 4) remove duplicate spaces left over by 1) or 2) or 3)
- # 5) remove space at the beginning of the string left over by 1) or 2) or 3)
- # 6) remove space at the end of the string left over by 1) or 2) or 3)
- sed -i -E "/^GRUB_CMDLINE_LINUX=/ {
s/\"${_para}=[^[:space:]\"]*/\"/g;s/[[:space:]]+${_para}=[^[:space:]\"]*/ /g;s/\"$/ ${_para_val}\"/s/[[:space:]]+/ /g;s/(\")[[:space:]]+/\1/g;s/[[:space:]]+(\")/\1/g;}" "$GRUB_ETC_DEFAULT"}
reset_crashkernel() @@ -1496,10 +1520,12 @@ reset_crashkernel() # - set the dump mode as kdump for non-ppc64le cases # - retrieved the default crashkernel value for given dump mode if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then
_update_kernel_cmdline_in_grub_etc_default crashkernel "$_crashkernel"
_update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel"
[[ $_fadump_val == off ]] && _fadump_val=""_update_kernel_cmdline_in_grub_etc_default fadump "$_fadump_val"
if [[ $_fadump_val == off ]]; then_fadump_val=""fi_update_kernel_arg_in_grub_etc_default fadump "$_fadump_val"fi
# If kernel-path not specified, either
Hi Philipp,
On Mon, Feb 28, 2022 at 06:11:12PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Wed, 23 Feb 2022 15:45:46 +0800 Coiby Xu coxu@redhat.com wrote:
When updating the kernel command line in /etc/default/grub, grubby only replaces a given entry if it is the last entry on the command line. In all other cases it appends a new entry to it. This behavior makes it quite likely that there are multiple entries of the same parameter on the command line, e.g. GRUB_CMDLINE_LINUX="crashkernel=110M crashkernel=220M fadump=on crashkernel=330M".
I played around a little with grubby and for me it looks like it has the same behavior you are implementing below, i.e. that it always removes all occurrences of $param= and then append the new $param= at the end. That contradicts what you are writing above. Can you please double check this? Thanks
Although I don't know why I made the wrong statement, you are right about the above observation.
Code looks good, though.
Reviewed-by: Philipp Rudo prudo@redhat.com
Thanks for the review! I've merged the patch with corrected commit msg.
In such an situation _update_kernel_cmdline_in_grub_etc_default only updates/removes the last entry. Which is usually not what you want as the kernel (for crashkernel) takes the last entry it can find.
Thus make sure the case with multiple entries of the same parameter is handled properly by removing all occurrences of given parameter first.
Note
- sed command group and conditional control has been used to get rid of grep.
- Fully supporting kernel cmdline as documented in Documentation/admin-guide/kernel-parameters.rst is complex and in foreseeable future a full implementation is not needed. So simply document the unsupported cases instead.
Reported-by: Philipp Rudo prudo@redhat.com Suggested-by: Philipp Rudo prudo@redhat.com Signed-off-by: Coiby Xu coxu@redhat.com
kdumpctl | 54 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/kdumpctl b/kdumpctl index 9fd76ac..978aee1 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1399,25 +1399,49 @@ _get_all_kernels_from_grubby() }
GRUB_ETC_DEFAULT="/etc/default/grub" -# modify the kernel command line parameter in default grub conf +# Update a kernel parameter in default grub conf +# +# If a value is specified, it will be inserted in the end. Otherwise it +# would remove given kernel parameter. +# +# Note this function doesn't address the following cases, +# 1. The kernel ignores everything on the command line after a '--'. So +# simply adding 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. +# 4. Some parameters, e.g. efivar_ssdt, can be given multiple times. +# 5. Some kernel parameters, e.g. quiet, doesn't have value # # $1: the name of the kernel command line parameter -# $2: new value. If empty, the parameter would be removed -_update_kernel_cmdline_in_grub_etc_default() +# $2: new value. If empty, given parameter would be removed +_update_kernel_arg_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 firstsed -i -E 's/^(GRUB_CMDLINE_LINUX=")/\1'"$_para_val"' /' "$GRUB_ETC_DEFAULT"- fi
- # Update the command line /etc/default/grub, i.e.
- # on the line that starts with 'GRUB_CMDLINE_LINUX=',
- # 1) remove $para=$val if the it's the first arg
- # 2) remove all occurences of $para=$val
- # 3) insert $_para_val to end
- # 4) remove duplicate spaces left over by 1) or 2) or 3)
- # 5) remove space at the beginning of the string left over by 1) or 2) or 3)
- # 6) remove space at the end of the string left over by 1) or 2) or 3)
- sed -i -E "/^GRUB_CMDLINE_LINUX=/ {
s/\"${_para}=[^[:space:]\"]*/\"/g;s/[[:space:]]+${_para}=[^[:space:]\"]*/ /g;s/\"$/ ${_para_val}\"/s/[[:space:]]+/ /g;s/(\")[[:space:]]+/\1/g;s/[[:space:]]+(\")/\1/g;}" "$GRUB_ETC_DEFAULT"}
reset_crashkernel() @@ -1496,10 +1520,12 @@ reset_crashkernel() # - set the dump mode as kdump for non-ppc64le cases # - retrieved the default crashkernel value for given dump mode if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then
_update_kernel_cmdline_in_grub_etc_default crashkernel "$_crashkernel"
_update_kernel_arg_in_grub_etc_default crashkernel "$_crashkernel"
[[ $_fadump_val == off ]] && _fadump_val=""_update_kernel_cmdline_in_grub_etc_default fadump "$_fadump_val"
if [[ $_fadump_val == off ]]; then_fadump_val=""fi_update_kernel_arg_in_grub_etc_default fadump "$_fadump_val"fi
# If kernel-path not specified, either
-
Coiby Xu -
Philipp Rudo