From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper chk_msk_info() to show the counters in
mptcp_info of the given info, and check that the timestamps move
forward. Use it to show newly added last_data_sent, last_data_recv
and last_ack_recv in mptcp_info in chk_last_time_info().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/diag.sh | 48 +++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index bc97ab33a00e..3d60e6544e5f 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -200,6 +200,53 @@ chk_msk_cestab()
 		 "${expected}" "${msg}" ""
 }
 
+msk_info_get_value()
+{
+	local port="${1}"
+	local info="${2}"
+
+	ss -N ${ns} -inHM dport ${port} | \
+		mptcp_lib_get_info_value "${info}" "${info}"
+}
+
+chk_msk_info()
+{
+	local port="${1}"
+	local info="${2}"
+	local cnt="${3}"
+	local now=$(msk_info_get_value ${port} "${info}")
+	local delta_ms=250
+
+	mptcp_lib_print_title "....chk ${info}"
+	if { [ -z "${cnt}" ] || [ -z "${now}" ]; } &&
+	   ! mptcp_lib_expect_all_features; then
+		mptcp_lib_pr_skip "Feature probably not supported"
+		mptcp_lib_result_skip "${info}"
+	elif [ "$((cnt1 + ${delta_ms}))" -lt "${now}" ]; then
+		mptcp_lib_pr_ok
+		mptcp_lib_result_pass "${info}"
+	else
+		mptcp_lib_pr_fail "value of ${info} changed by $((now - cnt))ms," \
+				  "expected at least ${delta_ms}ms"
+		mptcp_lib_result_fail "${info}"
+		ret=${KSFT_FAIL}
+	fi
+}
+
+chk_last_time_info()
+{
+	local port="${1}"
+	local cnt1=$(msk_info_get_value ${port} "last_data_sent")
+	local cnt2=$(msk_info_get_value ${port} "last_data_recv")
+	local cnt3=$(msk_info_get_value ${port} "last_ack_recv")
+
+	sleep 0.5
+
+	chk_msk_info "${port}" "last_data_sent" "${cnt1}"
+	chk_msk_info "${port}" "last_data_recv" "${cnt2}"
+	chk_msk_info "${port}" "last_ack_recv" "${cnt3}"
+}
+
 wait_connected()
 {
 	local listener_ns="${1}"
@@ -233,6 +280,7 @@ echo "b" | \
 				127.0.0.1 >/dev/null &
 wait_connected $ns 10000
 chk_msk_nr 2 "after MPC handshake "
+chk_last_time_info 10000
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
 chk_msk_inuse 2
-- 
2.40.1

Hi Geliang,

I did a review of this patch before, but I didn't send it because I was
looking at the CI in parallel (see below). Later, I noticed your
squash-to patch (and the report from the CI). I think you fixed all the
issues I reported here below, except my suggestion about not using
'cnt1', 'cnt2', etc. but something more explicit. Do you mind looking at
that, please?

Also, do you mind sending a v5 instead? The CI will not pick-up the
squash-to patch.

Please note that the CI will now [1] use your IPRoute2 patch [2], so the
diag.sh test should no longer fail:

[1]
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker/commit/fbf8f1a
[2] https://github.com/multipath-tcp/iproute2/commit/591f0654c

On 02/04/2024 11:28, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper chk_msk_info() to show the counters in
> mptcp_info of the given info, and check that the timestamps move
> forward. Use it to show newly added last_data_sent, last_data_recv
> and last_ack_recv in mptcp_info in chk_last_time_info().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/net/mptcp/diag.sh | 48 +++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index bc97ab33a00e..3d60e6544e5f 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -200,6 +200,53 @@ chk_msk_cestab()
>  		 "${expected}" "${msg}" ""
>  }
>  
> +msk_info_get_value()
> +{
> +	local port="${1}"
> +	local info="${2}"
> +
> +	ss -N ${ns} -inHM dport ${port} | \
> +		mptcp_lib_get_info_value "${info}" "${info}"
> +}
> +
> +chk_msk_info()
> +{
> +	local port="${1}"
> +	local info="${2}"
> +	local cnt="${3}"
> +	local now=$(msk_info_get_value ${port} "${info}")
> +	local delta_ms=250
> +
> +	mptcp_lib_print_title "....chk ${info}"
> +	if { [ -z "${cnt}" ] || [ -z "${now}" ]; } &&
> +	   ! mptcp_lib_expect_all_features; then
> +		mptcp_lib_pr_skip "Feature probably not supported"
> +		mptcp_lib_result_skip "${info}"
> +	elif [ "$((cnt1 + ${delta_ms}))" -lt "${now}" ]; then

It should be 'cnt', not 'cnt1', and without ${} around delta_ms when
used inside '$((...))'. shellcheck should complain about that.

> +		mptcp_lib_pr_ok
> +		mptcp_lib_result_pass "${info}"
> +	else
> +		mptcp_lib_pr_fail "value of ${info} changed by $((now - cnt))ms," \
> +				  "expected at least ${delta_ms}ms"
> +		mptcp_lib_result_fail "${info}"
> +		ret=${KSFT_FAIL}
> +	fi
> +}
> +
> +chk_last_time_info()
> +{
> +	local port="${1}"
> +	local cnt1=$(msk_info_get_value ${port} "last_data_sent")
> +	local cnt2=$(msk_info_get_value ${port} "last_data_recv")
> +	local cnt3=$(msk_info_get_value ${port} "last_ack_recv")

I would recommend not using '<generic name>X', but something more readable:

  data_sent=$(...)
  data_recv=$(...)
  ack_recv=$(...)

Also, I guess shellcheck will complain here: double quotes around
"${port}", and declaring and assigning separately, no?

If you are not using them, there are shellcheck plugins for many text
editors [1], or you can use a git hook [1].

[1] https://github.com/koalaman/shellcheck#user-content-in-your-editor
[2] https://github.com/koalaman/shellcheck-precommit

> +
> +	sleep 0.5
> +
> +	chk_msk_info "${port}" "last_data_sent" "${cnt1}"
> +	chk_msk_info "${port}" "last_data_recv" "${cnt2}"
> +	chk_msk_info "${port}" "last_ack_recv" "${cnt3}"
> +}
> +
>  wait_connected()
>  {
>  	local listener_ns="${1}"
> @@ -233,6 +280,7 @@ echo "b" | \
>  				127.0.0.1 >/dev/null &
>  wait_connected $ns 10000
>  chk_msk_nr 2 "after MPC handshake "
> +chk_last_time_info 10000
>  chk_msk_remote_key_nr 2 "....chk remote_key"
>  chk_msk_fallback_nr 0 "....chk no fallback"
>  chk_msk_inuse 2

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.