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(a)redhat.com> wrote:
> So the regex can be reused to retrieve fadump and crashkernel value.
>
> Signed-off-by: Coiby Xu <coxu(a)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"
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/4f7ccd4245ea0...
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
--
Best regards,
Coiby