1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v2 0/2] mptcp: selftests: join: support BusyBox and ShellCheck
  4. View patch

[PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'

Matthieu Baerts posted 2 patches 2 years, 10 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>
There is a newer version of this series
[PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'
Posted by Matthieu Baerts 2 years, 10 months ago 
BusyBox's 'cmp' command doesn't support the '--bytes' parameter.

Some CIs -- i.e. LKFT -- use BusyBox and have the mptcp_join.sh test
failing [1] because their 'cmp' command doesn't support this '--bytes'
option:

    cmp: unrecognized option '--bytes=1024'
    BusyBox v1.35.0 () multi-call binary.

    Usage: cmp [-ls] [-n NUM] FILE1 [FILE2]

Instead, 'head --bytes' can be used as this option is supported by
BusyBox.

Because it is apparently quite common to use BusyBox, it is certainly
better to backport this fix to impacted kernels.

Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
Link: https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.3-rc5-5-g148341f0a2f5/testrun/16088933/suite/kselftest-net-mptcp/test/net_mptcp_userspace_pm_sh/log [1]
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
v2:
 - limit the input file, not the output of cmp...
   (strange that on my side, I didn't reproduce the issue but the CI did)
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fafd19ec7e1f..bb26c406f09c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -378,9 +378,14 @@ check_transfer()
 			fail_test
 			return 1
 		fi
-		bytes="--bytes=${bytes}"
+
+		# note: BusyBox's "cmp" command doesn't support --bytes
+		head --bytes="$bytes" "$in" > "$in.cut"
+		head --bytes="$bytes" "$out" > "$out.cut"
+		in="$in.cut"
+		out="$out.cut"
 	fi
-	cmp -l "$in" "$out" ${bytes} | while read -r i a b; do
+	cmp -l "$in" "$out" | while read -r i a b; do
 		local sum=$((0${a} + 0${b}))
 		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
 			echo "[ FAIL ] $what does not match (in, out):"
@@ -394,6 +399,10 @@ check_transfer()
 		fi
 	done
 
+	if [ -n "$bytes" ]; then
+		rm -f "$in" "$out"
+	fi
+
 	return 0
 }
 

-- 
2.39.2
Re: [PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'
Posted by Paolo Abeni 2 years, 10 months ago 
On Tue, 2023-04-04 at 18:21 +0200, Matthieu Baerts wrote:
> BusyBox's 'cmp' command doesn't support the '--bytes' parameter.
> 
> Some CIs -- i.e. LKFT -- use BusyBox and have the mptcp_join.sh test
> failing [1] because their 'cmp' command doesn't support this '--bytes'
> option:
> 
>     cmp: unrecognized option '--bytes=1024'
>     BusyBox v1.35.0 () multi-call binary.
> 
>     Usage: cmp [-ls] [-n NUM] FILE1 [FILE2]
> 
> Instead, 'head --bytes' can be used as this option is supported by
> BusyBox.
> 
> Because it is apparently quite common to use BusyBox, it is certainly
> better to backport this fix to impacted kernels.
> 
> Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
> Link: https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.3-rc5-5-g148341f0a2f5/testrun/16088933/suite/kselftest-net-mptcp/test/net_mptcp_userspace_pm_sh/log [1]
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> v2:
>  - limit the input file, not the output of cmp...
>    (strange that on my side, I didn't reproduce the issue but the CI did)
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fafd19ec7e1f..bb26c406f09c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -378,9 +378,14 @@ check_transfer()
>  			fail_test
>  			return 1
>  		fi
> -		bytes="--bytes=${bytes}"
> +
> +		# note: BusyBox's "cmp" command doesn't support --bytes
> +		head --bytes="$bytes" "$in" > "$in.cut"
> +		head --bytes="$bytes" "$out" > "$out.cut"
> +		in="$in.cut"
> +		out="$out.cut"
>  	fi
> -	cmp -l "$in" "$out" ${bytes} | while read -r i a b; do
> +	cmp -l "$in" "$out" | while read -r i a b; do
>  		local sum=$((0${a} + 0${b}))
>  		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
>  			echo "[ FAIL ] $what does not match (in, out):"
> @@ -394,6 +399,10 @@ check_transfer()
>  		fi
>  	done
>  
> +	if [ -n "$bytes" ]; then
> +		rm -f "$in" "$out"
> +	fi

I think it would be better additionally remove the above files in
cleanup(), too.

Thanks!

Paolo
Re: [PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'
Posted by Matthieu Baerts 2 years, 10 months ago 
Hi Paolo,

On 07/04/2023 15:30, Paolo Abeni wrote:
> On Tue, 2023-04-04 at 18:21 +0200, Matthieu Baerts wrote:
>> BusyBox's 'cmp' command doesn't support the '--bytes' parameter.
>>
>> Some CIs -- i.e. LKFT -- use BusyBox and have the mptcp_join.sh test
>> failing [1] because their 'cmp' command doesn't support this '--bytes'
>> option:
>>
>>     cmp: unrecognized option '--bytes=1024'
>>     BusyBox v1.35.0 () multi-call binary.
>>
>>     Usage: cmp [-ls] [-n NUM] FILE1 [FILE2]
>>
>> Instead, 'head --bytes' can be used as this option is supported by
>> BusyBox.
>>
>> Because it is apparently quite common to use BusyBox, it is certainly
>> better to backport this fix to impacted kernels.
>>
>> Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
>> Link: https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.3-rc5-5-g148341f0a2f5/testrun/16088933/suite/kselftest-net-mptcp/test/net_mptcp_userspace_pm_sh/log [1]
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>> v2:
>>  - limit the input file, not the output of cmp...
>>    (strange that on my side, I didn't reproduce the issue but the CI did)
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index fafd19ec7e1f..bb26c406f09c 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -378,9 +378,14 @@ check_transfer()
>>  			fail_test
>>  			return 1
>>  		fi
>> -		bytes="--bytes=${bytes}"
>> +
>> +		# note: BusyBox's "cmp" command doesn't support --bytes
>> +		head --bytes="$bytes" "$in" > "$in.cut"
>> +		head --bytes="$bytes" "$out" > "$out.cut"
>> +		in="$in.cut"
>> +		out="$out.cut"
>>  	fi
>> -	cmp -l "$in" "$out" ${bytes} | while read -r i a b; do
>> +	cmp -l "$in" "$out" | while read -r i a b; do
>>  		local sum=$((0${a} + 0${b}))
>>  		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
>>  			echo "[ FAIL ] $what does not match (in, out):"
>> @@ -394,6 +399,10 @@ check_transfer()
>>  		fi
>>  	done
>>  
>> +	if [ -n "$bytes" ]; then
>> +		rm -f "$in" "$out"
>> +	fi
> 
> I think it would be better additionally remove the above files in
> cleanup(), too.

Thank you for this review!

Should I maybe instead override the files? So have this above:

+		head --bytes="$bytes" "$in" > "$in.cut"
+		head --bytes="$bytes" "$out" > "$out.cut"
+		mv "$in.cut" "$in"
+		mv "$out.cut" "$out"

Then I don't need to make sure these specific files are really cleaned
up. We don't use these files after if I'm not mistaken.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'
Posted by Paolo Abeni 2 years, 10 months ago 
On Fri, 2023-04-07 at 15:35 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 07/04/2023 15:30, Paolo Abeni wrote:
> > On Tue, 2023-04-04 at 18:21 +0200, Matthieu Baerts wrote:
> > > BusyBox's 'cmp' command doesn't support the '--bytes' parameter.
> > > 
> > > Some CIs -- i.e. LKFT -- use BusyBox and have the mptcp_join.sh test
> > > failing [1] because their 'cmp' command doesn't support this '--bytes'
> > > option:
> > > 
> > >     cmp: unrecognized option '--bytes=1024'
> > >     BusyBox v1.35.0 () multi-call binary.
> > > 
> > >     Usage: cmp [-ls] [-n NUM] FILE1 [FILE2]
> > > 
> > > Instead, 'head --bytes' can be used as this option is supported by
> > > BusyBox.
> > > 
> > > Because it is apparently quite common to use BusyBox, it is certainly
> > > better to backport this fix to impacted kernels.
> > > 
> > > Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
> > > Link: https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.3-rc5-5-g148341f0a2f5/testrun/16088933/suite/kselftest-net-mptcp/test/net_mptcp_userspace_pm_sh/log [1]
> > > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > > ---
> > > v2:
> > >  - limit the input file, not the output of cmp...
> > >    (strange that on my side, I didn't reproduce the issue but the CI did)
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index fafd19ec7e1f..bb26c406f09c 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -378,9 +378,14 @@ check_transfer()
> > >  			fail_test
> > >  			return 1
> > >  		fi
> > > -		bytes="--bytes=${bytes}"
> > > +
> > > +		# note: BusyBox's "cmp" command doesn't support --bytes
> > > +		head --bytes="$bytes" "$in" > "$in.cut"
> > > +		head --bytes="$bytes" "$out" > "$out.cut"
> > > +		in="$in.cut"
> > > +		out="$out.cut"
> > >  	fi
> > > -	cmp -l "$in" "$out" ${bytes} | while read -r i a b; do
> > > +	cmp -l "$in" "$out" | while read -r i a b; do
> > >  		local sum=$((0${a} + 0${b}))
> > >  		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
> > >  			echo "[ FAIL ] $what does not match (in, out):"
> > > @@ -394,6 +399,10 @@ check_transfer()
> > >  		fi
> > >  	done
> > >  
> > > +	if [ -n "$bytes" ]; then
> > > +		rm -f "$in" "$out"
> > > +	fi
> > 
> > I think it would be better additionally remove the above files in
> > cleanup(), too.
> 
> Thank you for this review!
> 
> Should I maybe instead override the files? So have this above:
> 
> +		head --bytes="$bytes" "$in" > "$in.cut"
> +		head --bytes="$bytes" "$out" > "$out.cut"
> +		mv "$in.cut" "$in"
> +		mv "$out.cut" "$out"
> 
> Then I don't need to make sure these specific files are really cleaned
> up. We don't use these files after if I'm not mistaken.

I took to me validate the above, but I think you are right: should not
be used later.

Note that if the script is interrupted after:

head --bytes="$bytes" "$in" > "$in.cut"

and before:

mv "$out.cut" "$out"

the truncated files will be leaked anyway, so perhaps the cleanup()
function is the more straight forward solution?!?

Thanks,

Paolo
Re: [PATCH mptcp-next v2 1/2] selftests: mptcp: join: avoid using 'cmp --bytes'
Posted by Matthieu Baerts 2 years, 10 months ago 
Hi Paolo,

On 07/04/2023 18:29, Paolo Abeni wrote:
> On Fri, 2023-04-07 at 15:35 +0200, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 07/04/2023 15:30, Paolo Abeni wrote:
>>> On Tue, 2023-04-04 at 18:21 +0200, Matthieu Baerts wrote:
>>>> BusyBox's 'cmp' command doesn't support the '--bytes' parameter.
>>>>
>>>> Some CIs -- i.e. LKFT -- use BusyBox and have the mptcp_join.sh test
>>>> failing [1] because their 'cmp' command doesn't support this '--bytes'
>>>> option:
>>>>
>>>>     cmp: unrecognized option '--bytes=1024'
>>>>     BusyBox v1.35.0 () multi-call binary.
>>>>
>>>>     Usage: cmp [-ls] [-n NUM] FILE1 [FILE2]
>>>>
>>>> Instead, 'head --bytes' can be used as this option is supported by
>>>> BusyBox.
>>>>
>>>> Because it is apparently quite common to use BusyBox, it is certainly
>>>> better to backport this fix to impacted kernels.
>>>>
>>>> Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
>>>> Link: https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v6.3-rc5-5-g148341f0a2f5/testrun/16088933/suite/kselftest-net-mptcp/test/net_mptcp_userspace_pm_sh/log [1]
>>>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> ---
>>>> v2:
>>>>  - limit the input file, not the output of cmp...
>>>>    (strange that on my side, I didn't reproduce the issue but the CI did)
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++--
>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> index fafd19ec7e1f..bb26c406f09c 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -378,9 +378,14 @@ check_transfer()
>>>>  			fail_test
>>>>  			return 1
>>>>  		fi
>>>> -		bytes="--bytes=${bytes}"
>>>> +
>>>> +		# note: BusyBox's "cmp" command doesn't support --bytes
>>>> +		head --bytes="$bytes" "$in" > "$in.cut"
>>>> +		head --bytes="$bytes" "$out" > "$out.cut"
>>>> +		in="$in.cut"
>>>> +		out="$out.cut"
>>>>  	fi
>>>> -	cmp -l "$in" "$out" ${bytes} | while read -r i a b; do
>>>> +	cmp -l "$in" "$out" | while read -r i a b; do
>>>>  		local sum=$((0${a} + 0${b}))
>>>>  		if [ $check_invert -eq 0 ] || [ $sum -ne $((0xff)) ]; then
>>>>  			echo "[ FAIL ] $what does not match (in, out):"
>>>> @@ -394,6 +399,10 @@ check_transfer()
>>>>  		fi
>>>>  	done
>>>>  
>>>> +	if [ -n "$bytes" ]; then
>>>> +		rm -f "$in" "$out"
>>>> +	fi
>>>
>>> I think it would be better additionally remove the above files in
>>> cleanup(), too.
>>
>> Thank you for this review!
>>
>> Should I maybe instead override the files? So have this above:
>>
>> +		head --bytes="$bytes" "$in" > "$in.cut"
>> +		head --bytes="$bytes" "$out" > "$out.cut"
>> +		mv "$in.cut" "$in"
>> +		mv "$out.cut" "$out"
>>
>> Then I don't need to make sure these specific files are really cleaned
>> up. We don't use these files after if I'm not mistaken.
> 
> I took to me validate the above, but I think you are right: should not
> be used later.
> 
> Note that if the script is interrupted after:
> 
> head --bytes="$bytes" "$in" > "$in.cut"
> 
> and before:
> 
> mv "$out.cut" "$out"

you really need not to be lucky :-P

> the truncated files will be leaked anyway, so perhaps the cleanup()
> function is the more straight forward solution?!?

I can but then we can also have issue before:

	sin=$(mktemp)
	sout=$(mktemp)
	cin=$(mktemp)
	cinsent=$(mktemp)
	cout=$(mktemp)
	evts_ns1=$(mktemp)
	evts_ns2=$(mktemp)

	trap cleanup EXIT

If the script is unlikely interrupted before the cleanup trap, some
files will not be removed.

But yes, I can add the "rm" in the cleanup function.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.