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(a)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(a)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
> 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(a)redhat.com>
> Suggested-by: Philipp Rudo <prudo(a)redhat.com>
> Signed-off-by: Coiby Xu <coxu(a)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
--
Best regards,
Coiby