From: Matthieu Baerts <matttbe@kernel.org>
To: Geliang Tang <geliang@kernel.org>, mptcp@lists.linux.dev
Cc: Geliang Tang <tanggeliang@kylinos.cn>
Subject: Re: [PATCH mptcp-next v4 2/2] selftests: mptcp: test last time mptcp_info
Date: Tue, 2 Apr 2024 13:28:48 +0200 [thread overview]
Message-ID: <4ae5e3df-27fe-4d07-9a5d-3299ef255348@kernel.org> (raw)
In-Reply-To: <0e14deb5db6de99fcfd30673ddf54aa9a1191007.1712049966.git.tanggeliang@kylinos.cn>
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.
next prev parent reply other threads:[~2024-04-02 11:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 9:28 [PATCH mptcp-next v4 0/2] display "last time" actions info Geliang Tang
2024-04-02 9:28 ` [PATCH mptcp-next v4 1/2] mptcp: add last time fields in mptcp_info Geliang Tang
2024-04-02 9:28 ` [PATCH mptcp-next v4 2/2] selftests: mptcp: test last time mptcp_info Geliang Tang
2024-04-02 11:28 ` Matthieu Baerts [this message]
2024-04-02 10:30 ` [PATCH mptcp-next v4 0/2] display "last time" actions info MPTCP CI
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ae5e3df-27fe-4d07-9a5d-3299ef255348@kernel.org \
--to=matttbe@kernel.org \
--cc=geliang@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=tanggeliang@kylinos.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).