1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v2 0/4] update userspace pm mptcp_info fields part 3
  4. View patch

[PATCH mptcp-next v2 1/4] selftests: mptcp: set all env vars as local ones

Geliang Tang posted 4 patches 2 years, 7 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>
[PATCH mptcp-next v2 1/4] selftests: mptcp: set all env vars as local ones
Posted by Geliang Tang 2 years, 7 months ago 
It would be better to move the declaration of all the env variables to
do_transfer(), run_tests(), or pm_nl_set_endpoint() as local variables,
instead of exporting them globally at the beginning of the file.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e6c9d5451c5b..ad0717cb0d7e 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -49,11 +49,11 @@ TEST_COUNT=0
 TEST_NAME=""
 nr_blank=40
 
-export FAILING_LINKS=""
-export test_linkfail=0
-export addr_nr_ns1=0
-export addr_nr_ns2=0
-export sflags=""
+FAILING_LINKS=""
+test_linkfail=0
+addr_nr_ns1=0
+addr_nr_ns2=0
+sflags=""
 
 # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
 #				  (ip6 && (ip6[74] & 0xf0) == 0x30)'"
@@ -100,7 +100,6 @@ init_partial()
 	stats_dumped=0
 	check_invert=0
 	validate_checksum=$checksum
-	FAILING_LINKS=""
 
 	#  ns1         ns2
 	# ns1eth1    ns2eth1
@@ -828,6 +827,10 @@ pm_nl_set_endpoint()
 	local connector_ns="$2"
 	local connect_addr="$3"
 
+	local addr_nr_ns1=${addr_nr_ns1:-0}
+	local addr_nr_ns2=${addr_nr_ns2:-0}
+	local sflags=${sflags:-""}
+
 	# let the mptcp subflow be established in background before
 	# do endpoint manipulation
 	if [ $addr_nr_ns1 != "0" ] || [ $addr_nr_ns2 != "0" ]; then
@@ -979,6 +982,7 @@ do_transfer()
 
 	local port=$((10000 + TEST_COUNT - 1))
 	local cappid
+	local FAILING_LINKS=${FAILING_LINKS:-""}
 
 	:> "$cout"
 	:> "$sout"
@@ -1158,6 +1162,7 @@ run_tests()
 	local speed="${4:-fast}"
 
 	local size
+	local test_linkfail=${test_linkfail:-0}
 
 	# The values above 2 are reused to make test files
 	# with the given sizes (KB)
-- 
2.35.3
Re: [PATCH mptcp-next v2 1/4] selftests: mptcp: set all env vars as local ones
Posted by Matthieu Baerts 2 years, 7 months ago 
Hi Geliang,

On 30/06/2023 04:42, Geliang Tang wrote:
> It would be better to move the declaration of all the env variables to
> do_transfer(), run_tests(), or pm_nl_set_endpoint() as local variables,
> instead of exporting them globally at the beginning of the file.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e6c9d5451c5b..ad0717cb0d7e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -49,11 +49,11 @@ TEST_COUNT=0
>  TEST_NAME=""
>  nr_blank=40
>  
> -export FAILING_LINKS=""
> -export test_linkfail=0
> -export addr_nr_ns1=0
> -export addr_nr_ns2=0
> -export sflags=""
> +FAILING_LINKS=""
> +test_linkfail=0
> +addr_nr_ns1=0
> +addr_nr_ns2=0
> +sflags=""

Ah yes, you still need to make sure they are not already set before
using them later and that's why you set their default value here, right?

But then, I see you are setting the default values twice: here and in
the different functions where you declared the local variables. To avoid
that, I would suggest to either:

- "unset" the variables here and keep the default values below
- declare the default value only here at the top of the file and in the
different functions, only do:

   local XXX=${XXX}

I *think* it might be clearer to do the unset (+ a comment) and keep the
rest as it is:

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index b89077510080..9fc5a78c6063 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -49,14 +49,15 @@ TEST_COUNT=0
>  TEST_NAME=""
>  nr_blank=40
>  
> -FAILING_LINKS=""
> -test_linkfail=0
> -addr_nr_ns1=0
> -addr_nr_ns2=0
> -sflags=""
> -fastclose=""
> -fullmesh=""
> -speed="fast"
> +# These var are used only in some tests, make sure they are not already set
> +unset FAILING_LINKS
> +unset test_linkfail
> +unset addr_nr_ns1
> +unset addr_nr_ns2
> +unset sflags
> +unset fastclose
> +unset fullmesh
> +unset speed
>  
>  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
>  #                                (ip6 && (ip6[74] & 0xf0) == 0x30)'"

WDYT?

I can do the modifications when applying the patches if it is easier.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v2 1/4] selftests: mptcp: set all env vars as local ones
Posted by Geliang Tang 2 years, 7 months ago 
On Fri, Jun 30, 2023 at 10:42:47AM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 30/06/2023 04:42, Geliang Tang wrote:
> > It would be better to move the declaration of all the env variables to
> > do_transfer(), run_tests(), or pm_nl_set_endpoint() as local variables,
> > instead of exporting them globally at the beginning of the file.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index e6c9d5451c5b..ad0717cb0d7e 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -49,11 +49,11 @@ TEST_COUNT=0
> >  TEST_NAME=""
> >  nr_blank=40
> >  
> > -export FAILING_LINKS=""
> > -export test_linkfail=0
> > -export addr_nr_ns1=0
> > -export addr_nr_ns2=0
> > -export sflags=""
> > +FAILING_LINKS=""
> > +test_linkfail=0
> > +addr_nr_ns1=0
> > +addr_nr_ns2=0
> > +sflags=""
> 
> Ah yes, you still need to make sure they are not already set before
> using them later and that's why you set their default value here, right?
> 
> But then, I see you are setting the default values twice: here and in
> the different functions where you declared the local variables. To avoid
> that, I would suggest to either:
> 
> - "unset" the variables here and keep the default values below
> - declare the default value only here at the top of the file and in the
> different functions, only do:
> 
>    local XXX=${XXX}
> 
> I *think* it might be clearer to do the unset (+ a comment) and keep the
> rest as it is:
> 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index b89077510080..9fc5a78c6063 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -49,14 +49,15 @@ TEST_COUNT=0
> >  TEST_NAME=""
> >  nr_blank=40
> >  
> > -FAILING_LINKS=""
> > -test_linkfail=0
> > -addr_nr_ns1=0
> > -addr_nr_ns2=0
> > -sflags=""
> > -fastclose=""
> > -fullmesh=""
> > -speed="fast"
> > +# These var are used only in some tests, make sure they are not already set
> > +unset FAILING_LINKS
> > +unset test_linkfail
> > +unset addr_nr_ns1
> > +unset addr_nr_ns2
> > +unset sflags
> > +unset fastclose
> > +unset fullmesh
> > +unset speed
> >  
> >  # generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
> >  #                                (ip6 && (ip6[74] & 0xf0) == 0x30)'"
> 
> WDYT?
> 
> I can do the modifications when applying the patches if it is easier.

Sure, I agree. Please modify it for me, thanks very much.

-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.