diff mbox series

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

Message ID 644ac151204efcd05b047d4e9b1545747ba785f1.1716451525.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series use helpers in lib.sh and net_helpers.sh | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 33 lines checked
matttbe/shellcheck success No ShellCheck issues
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang May 23, 2024, 8:08 a.m. UTC 
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(-)

Comments

Matthieu Baerts (NGI0) May 23, 2024, 9:18 a.m. UTC | #1 
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?


> +		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
Geliang Tang May 23, 2024, 9:26 a.m. UTC | #2 
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
Matthieu Baerts (NGI0) May 23, 2024, 9:34 a.m. UTC | #3 
On 23/05/2024 11:26, Geliang Tang wrote:
> 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.

Will 'setup_ns' not assign the variable names given in parameter? If we
give 'ns1', it will set 'ns1=xxxx'. So we can use '${ns1}' to get the
name. If 'netns="ns1"', we can then get the name with '${!netns}', using
'!' (${!netns}) to resolve the name, no?

Cheers,
Matt
diff mbox series

Patch

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
@@ -1,6 +1,8 @@ 
 #! /bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
+. "$(dirname "${0}")/../lib.sh"
+
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
 readonly KSFT_SKIP=4
@@ -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
+		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
 }