Re: [PATCH mptcp-next v3 28/29] selftests: mptcp: add mptcp_lib_check_transfer

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 25/09/2023 10:42, Geliang Tang wrote:
> check_transfer() helper is defined both in mptcp_connect.sh and
> mptcp_sockopt.sh, export it into mptcp_lib.sh and rename it with
> mptcp_lib_ prefix. Use this new helper in both scripts.

Good idea!

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 7b0d03c40f89..fba62cdef2cd 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -353,3 +353,27 @@ mptcp_lib_make_file() {
>  	dd if=/dev/urandom of="$name" bs=$bs count=$size 2> /dev/null
>  	echo -e "\nMPTCP_TEST_FILE_END_MARKER" >> "$name"
>  }
> +
> +print_file_err()

Please prefix it with "mptcp_lib_" + add a comment:

  # $1: file

> +{
> +	ls -l "$1" 1>&2

Please use {} around variables: "${1}"

(like in the rest of the file)

> +	echo "Trailing bytes are: "
> +	tail -c 27 "$1"
> +}
> +
> +mptcp_lib_check_transfer() {

Please add a comment:

  # $1: input file ; $2: output file ; $3: what kind of file

> +	local in=$1

Please use {} and quotes around variables: "${1}". Same below.

> +	local out=$2
> +	local what=$3
> +
> +	cmp "$in" "$out" > /dev/null 2>&1
> +	if [ $? -ne 0 ] ;then

Small detail: probably better to do this (I guess Shellcheck would
complain if it is not done like that)

  if ! cmp "$in" "$out" > /dev/null 2>&1; then

> +		echo "[ FAIL ] $what does not match (in, out):"
> +		print_file_err "$in"
> +		print_file_err "$out"
> +
> +		return 1
> +	fi
> +
> +	return 0
> +}
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> index 39128fca99dd..aa4b9a4e6a56 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.sh
> @@ -135,32 +135,6 @@ check_mark()
>  	return 0
>  }
>  
> -print_file_err()
> -{
> -	ls -l "$1" 1>&2
> -	echo "Trailing bytes are: "
> -	tail -c 27 "$1"
> -}
> -
> -check_transfer()
> -{
> -	local in=$1
> -	local out=$2
> -	local what=$3
> -
> -	cmp "$in" "$out" > /dev/null 2>&1
> -	if [ $? -ne 0 ] ;then
> -		echo "[ FAIL ] $what does not match (in, out):"
> -		print_file_err "$in"
> -		print_file_err "$out"
> -		ret=1

Could you add a comment in the commit message: here it is OK to drop
'ret=1' because it will be set in run_tests() anyway (if I'm not mistaken).

Otherwise a reviewer could think the test is no longer marked as failed
in case of error and he might ask the question or take time to find the
answer and we want to avoid that ;)

> -
> -		return 1
> -	fi
> -
> -	return 0
> -}
> -
>  do_transfer()
>  {
>  	local listener_ns="$1"
> @@ -232,7 +206,7 @@ do_transfer()
>  		check_mark $connector_ns 4 || retc=1
>  	fi
>  
> -	check_transfer $cin $sout "file received by server"
> +	mptcp_lib_check_transfer $cin $sout "file received by server"
>  	rets=$?
>  
>  	mptcp_lib_result_code "${retc}" "mark ${ip}"

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net