Hi Paolo,
On 22/05/2023 19:45, Paolo Abeni wrote:
> On Mon, 2023-05-22 at 18:38 +0200, Matthieu Baerts wrote:
>> Selftests are supposed to run on any kernels, including the old ones not
>> supporting all MPTCP features.
>>
>> One of them is the support of MP_FASTCLOSE introduced in commit
>> f284c0c77321 ("mptcp: implement fastclose xmit path").
>>
>> If the MIB counter is not available, the test cannot be verified and the
>> behaviour will not be the expected one. So we can skip the test if the
>> counter is missing.
>>
>> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368
>> Fixes: 01542c9bf9ab ("selftests: mptcp: add fastclose testcase")
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 1f24495308f9..ccf52aba8a1c 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -263,6 +263,19 @@ reset()
>> return 0
>> }
>>
>> +# $1: test name ; $2: counter to check
>> +reset_check_counter()
>> +{
>> + reset "${1}" || return 1
>> +
>> + local counter="${2}"
>> +
>> + if ! nstat -asz "${counter}" | grep -wq "${counter}"; then
>> + mark_as_skipped "counter '${counter}' is not available"
>> + return 1
>> + fi
>> +}
>> +
>> # $1: test name
>> reset_with_cookies()
>> {
>> @@ -3179,14 +3192,14 @@ fullmesh_tests()
>>
>> fastclose_tests()
>> {
>> - if reset "fastclose test"; then
>> + if reset_check_counter "fastclose test" "MPTcpExtMPFastcloseTx"; then
>> run_tests $ns1 $ns2 10.0.1.1 1024 0 fastclose_client
>> chk_join_nr 0 0 0
>> chk_fclose_nr 1 1
>> chk_rst_nr 1 1 invert
>> fi
>>
>> - if reset "fastclose server test"; then
>> + if reset_check_counter "fastclose server test" "MPTcpExtMPFastcloseRx"; then
>> run_tests $ns1 $ns2 10.0.1.1 1024 0 fastclose_server
>> chk_join_nr 0 0 0
>> chk_fclose_nr 1 1 invert
>>
>
> Here and in a few later patches, I would do the 'feature check' only
> once at the beginning of the functional block ('fastclose_test' in this
> case), in the most conservative way possible. The goal would be
> minimize the conflicts caused by present/absence of such patch.
I initially did that but then it breaks the feature to start a specific
test. Also, the ID of the tests would be different on the different
kernels which might make the tracking of the different issues more
difficult.
On older kernels, we already have conflicts because of all the "if reset
(...)" I introduced one or two years ago :)
But likely, we don't need to backport such checks to know if a test can
be started or not too far: as long as we can backport such checks to the
last stable kernels, that's enough for the different CIs using the last
stable version of the kselftests on older kernels.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net