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

[Geliang Tang <geliang.tang@suse.com>]

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