Re: [PATCH] Get rid of "grep: warning: stray \ before /"

[Philipp Rudo]

Hi Coiby,

On Tue, 30 Jan 2024 14:39:38 +0800
Coiby Xu <coxu(a)redhat.com&gt; wrote:

>  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2242185
>  
>  grep (3.8) warnings when running the unit tests or running
>  "kdumpctl reset-crashkernel" on >= F39,
>      # unit tests
>      Examples:
>        1) kdumpctl _find_kernel_path_by_release() returns the kernel path for the given
> release
>           When call _find_kernel_path_by_release vmlinuz-6.2.11-200.fc37.x86_64
>  
>           1.1) WARNING: There was output to stderr but not found expectation
>  
>                  stderr: grep: warning: stray \ before /
>  
>                # spec/kdumpctl_general_spec.sh:169-172
>  
>      # kdumpctl reset-crashkernel
>      grep: warning: stray \ before /
>      kdump: Updated crashkernel=1G-4G:192M,4G-64G:256M,64G-:512M for
> kernel=/boot/vmlinuz-6.6.8-200.fc39.x86_64. Please reboot the system for the change to
> take effect.
>  
>  This warning can be reproduced by
>    echo 'kernel="/boot/vmlinuz-6.4.6-200.fc38.x86_64"' | grep -E
> "^kernel=.*$_release(\/\w+)?\"$"
>  
>  This patch removes unneeded backslash. It also adds a test for
>  systemd-boot path. And for simplification, Parameters:dynamic is now
>  used to generate test data dynamically.
>  
>  Fixes: 8af05dc4 ("kdumpctl: Add support for systemd-boot paths")
>  Signed-off-by: Coiby Xu <coxu(a)redhat.com&gt;
>  ---
>   kdumpctl                      |  2 +-
>   spec/kdumpctl_general_spec.sh | 21 +++++++++++++++------
>   2 files changed, 16 insertions(+), 7 deletions(-)
>  
>  diff --git a/kdumpctl b/kdumpctl
>  index 45277a79..30eb27dd 100755
>  --- a/kdumpctl
>  +++ b/kdumpctl
>  @@ -1412,7 +1412,7 @@ _find_kernel_path_by_release()
>   
>   	# Insert '/' before '+' to cope with grep's EREs
>   	_release=${_release//+/\\+}
>  -	_grubby_kernel_str=$(grubby --info ALL | grep -E
> "^kernel=.*$_release(\/\w+)?\"$")
>  +	_grubby_kernel_str=$(grubby --info ALL | grep -E
> "^kernel=.*$_release(/\w+)?\"$")
>   	_kernel_path=$(_filter_grubby_kernel_str "$_grubby_kernel_str")
>   	if [[ -z $_kernel_path ]]; then
>   		ddebug "kernel $_release doesn't exist"
>  diff --git a/spec/kdumpctl_general_spec.sh b/spec/kdumpctl_general_spec.sh
>  index 243aeca3..f69924f6 100644
>  --- a/spec/kdumpctl_general_spec.sh
>  +++ b/spec/kdumpctl_general_spec.sh
>  @@ -156,16 +156,25 @@ Describe 'kdumpctl'
>   	End
>   
>   	Describe '_find_kernel_path_by_release()'
>  +		# When the array length changes, the Parameters:dynamic should change as well
>  +		kernel_paths=(/boot/vmlinuz-6.2.11-200.fc37.x86_64
> /boot/vmlinuz-5.14.0-316.el9.aarch64+64k /boot/vmlinuz-5.14.0-322.el9.aarch64
> /boot/efi/36b54597c46383/6.4.0-0.rc0.20230427git6e98b09da931.5.fc39.aarch64/linux)
>  +		kernels=(vmlinuz-6.2.11-200.fc37.x86_64 vmlinuz-5.14.0-316.el9.aarch64+64k
> vmlinuz-5.14.0-322.el9.aarch64 6.4.0-0.rc0.20230427git6e98b09da931.5.fc39.aarch64)
>  + 
all in all the patch looks good. Only the two lines above are extremely
hard to read because they are so long. Could you change formatting so
every line only contains a single kernel. I believe that will improve
readability a lot.

Besides this tiny nit
Reviewed-by: Philipp Rudo <prudo(a)redhat.com&gt;

>   		grubby() {
>  -			echo -e
> 'kernel="/boot/vmlinuz-6.2.11-200.fc37.x86_64"\nkernel="/boot/vmlinuz-5.14.0-322.el9.aarch64"\nkernel="/boot/vmlinuz-5.14.0-316.el9.aarch64+64k"'
>  +			for key in "${!kernel_paths[@]}"; do
>  +				echo "kernel=\"${kernel_paths[$key]}\""
>  +			done
>   		}
>   
>  -		Parameters
>  -			# parameter          answer
>  -			vmlinuz-6.2.11-200.fc37.x86_64	/boot/vmlinuz-6.2.11-200.fc37.x86_64
>  -			vmlinuz-5.14.0-322.el9.aarch64	/boot/vmlinuz-5.14.0-322.el9.aarch64
>  -			vmlinuz-5.14.0-316.el9.aarch64+64k	/boot/vmlinuz-5.14.0-316.el9.aarch64+64k
>  +		Parameters:dynamic
>  +			# Due to a bug [1] in shellspec, hardcode the loop number instead of using the
>  +			# array length
>  +			# [1] https://github.com/shellspec/shellspec/issues/259
>  +			for key in {0..3}; do
>  +				%data "${kernels[$key]}" "${kernel_paths[$key]}"
>  +			done
>   		End
>  +
>   		It 'returns the kernel path for the given release'
>   			When call _find_kernel_path_by_release "$1"
>   			The output should equal "$2"