Re: [PATCH mptcp-next v2 2/4] selftests: mptcp: use setup_ns helper in lib.sh

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2024-05-23 at 11:18 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 23/05/2024 10:08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch includes lib.sh into mptcp_lib.sh, uses setup_ns()
> > helper
> > defined in lib.sh to set up namespaces in mptcp_lib_ns_init(). Then
> > for
> > each namespace in NS_LIST, run all sysctl commands. This can drop
> > some
> > duplicate code.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_lib.sh  | 19 +++++++--------
> > ----
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > index ad2ebda5cb64..59eb77e7813d 100644
> > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> 
> 
> (...)
> 
> > @@ -412,20 +414,13 @@ mptcp_lib_check_tools() {
> >  }
> >  
> >  mptcp_lib_ns_init() {
> > -	local sec rndh
> > -
> > -	sec=$(date +%s)
> > -	rndh=$(printf %x "${sec}")-$(mktemp -u XXXXXX)
> > +	setup_ns "${@}"
> >  
> >  	local netns
> > -	for netns in "${@}"; do
> > -		eval "${netns}=${netns}-${rndh}"
> > -
> > -		ip netns add "${!netns}" || exit ${KSFT_SKIP}
> > -		ip -net "${!netns}" link set lo up
> > -		ip netns exec "${!netns}" sysctl -q
> > net.mptcp.enabled=1
> > -		ip netns exec "${!netns}" sysctl -q
> > net.ipv4.conf.all.rp_filter=0
> > -		ip netns exec "${!netns}" sysctl -q
> > net.ipv4.conf.default.rp_filter=0
> > +	for netns in $NS_LIST; do
> 
> Could you add {} (${NS_LIST}), to keep the same style (and usually
> recommended in order to avoid typos, etc.)
> 
> Also, I wonder if it is a good idea to use '${NS_LIST}': it might
> contain existing netns. e.g. with your patch 3/4, in mptcp_connect,
> mptcp_lib_ns_init will be called twice, modifying the first netns
> twice.
> 
> What if you keep:
> 
>   for netns in "${@}"; do
> 
> and use "${!netns}" instead?

We don't know the name of ns now because "mktemp -u XXXXXX" is executed
within setup_ns(). So we have to use $NS_LIST.

> 
> 
> > +		ip netns exec "${netns}" sysctl -q
> > net.mptcp.enabled=1
> > +		ip netns exec "${netns}" sysctl -q
> > net.ipv4.conf.all.rp_filter=0
> > +		ip netns exec "${netns}" sysctl -q
> > net.ipv4.conf.default.rp_filter=0
> >  	done
> >  }
> >  
> 
> Cheers,
> Matt