Hi Coiby,
On Wed, 16 Feb 2022 11:11:45 +0800
Coiby Xu <coxu(a)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(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.
Thanks!
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,
1. 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.
2. 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...
3. 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
>