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 v2 3/4] selftests: mptcp: use cleanup_all_ns helper in lib.sh
Date: Thu, 23 May 2024 11:22:49 +0200 [thread overview]
Message-ID: <77b181f2-cff1-4747-9a4e-5633d54c3121@kernel.org> (raw)
In-Reply-To: <d341c904ef486b7e0039c3812ddd659ef7791f9f.1716451525.git.tanggeliang@kylinos.cn>
Hi Geliang,
On 23/05/2024 10:08, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch uses cleanup_all_ns() helper defined in lib.sh instead of
> all mptcp_lib_ns_exit() in mptcp seltests. And drop this duplicate
> mptcp helper in mptcp_lib.sh.
>
> In mptcp_connect.sh, drop mptcp_lib_ns_exit in check_mptcp_disabled()
> directly, this "disabled_ns" will be deleted by cleanup_all_ns() in
> cleanup(), together with "ns1 - ns4".
Mmh, I think it is better to clear resources when we don't need them,
than cleaning them all at the end. New code doing that might be OK, but
here switching to that while not really gaining anything new looks less
good.
> In mptcp_join.sh, drop mptcp_lib_ns_exit in cleanup_partial() directly,
> each existing namespace will delete automaticly in setup_ns(), only
> adding cleanup_all_ns in cleanup() is enough.
Mmh, same here: I think it is better to clean at the end of a test, than
at the beginning of a new one: if there are issues to delete it, it will
be clearer it is due to the previous test, not the new one.
(...)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 59eb77e7813d..bd7d78e4aa83 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -424,14 +424,6 @@ mptcp_lib_ns_init() {
> done
> }
>
> -mptcp_lib_ns_exit() {
> - local netns
> - for netns in "${@}"; do
> - ip netns del "${netns}"
> - rm -f /tmp/"${netns}".{nstat,out}
These files are no longer removed. It is then no longer just a "simple"
refactoring. Such behaviour change should be mentioned in the commit
message, explaining why it is OK to do that.
I would suggest to keep mptcp_lib_ns_exit helper(), and simply call
'cleanup_ns "${@}"' here instead. If you do that, there is no need to
modify the other selftests, and you can squash this in patch 2 with
'setup_ns()'. WDYT?
> - done
> -}
> -
> mptcp_lib_events() {
> local ns="${1}"
> local evts="${2}"
(...)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
next prev parent reply other threads:[~2024-05-23 9:22 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-23 8:08 [PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh Geliang Tang
2024-05-23 8:08 ` [PATCH mptcp-next v2 1/4] selftests: mptcp: rename ns to ns1 in diag.sh Geliang Tang
2024-05-23 9:14 ` Matthieu Baerts
2024-05-23 8:08 ` [PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh Geliang Tang
2024-05-23 9:18 ` Matthieu Baerts
2024-05-23 9:26 ` Geliang Tang
2024-05-23 9:34 ` Matthieu Baerts
2024-05-23 8:08 ` [PATCH mptcp-next v2 3/4] selftests: mptcp: use cleanup_all_ns " Geliang Tang
2024-05-23 8:39 ` Geliang Tang
2024-05-23 9:22 ` Matthieu Baerts [this message]
2024-05-23 8:09 ` [PATCH mptcp-next v2 4/4] selftests: mptcp: use wait_local_port_listen helper Geliang Tang
2024-05-23 9:24 ` Matthieu Baerts
2024-05-23 8:58 ` [PATCH mptcp-next v2 0/4] use helpers in lib.sh and net_helpers.sh MPTCP CI
2024-05-23 9:13 ` Matthieu Baerts
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=77b181f2-cff1-4747-9a4e-5633d54c3121@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).