Re: [PATCH mptcp-next v5 4/4] selftests: mptcp: listener test for in-kernel PM

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

On Wed, Nov 23, 2022 at 12:49:58PM +0100, Paolo Abeni wrote:
> Hello,
> 
> sorry for the late feedback.
> 
> On Thu, 2022-11-17 at 13:31 +0800, Geliang Tang wrote:
> > This patch adds test coverage for listening sockets created by the
> > in-kernel path manager in mptcp_join.sh.
> > 
> > It adds the listener event checking in the existing "remove single
> > address with port" test. The output looks like this:
> > 
> >  003 remove single address with port syn[ ok ] - synack[ ok ] - ack[ ok ]
> >                                      add[ ok ] - echo  [ ok ] - pt [ ok ]
> >                                      syn[ ok ] - synack[ ok ] - ack[ ok ]
> >                                      syn[ ok ] - ack   [ ok ]
> >                                      rm [ ok ] - rmsf  [ ok ]   invert
> >                                      CREATE_LISTENER 10.0.2.1:10100[ ok ]
> >                                      CLOSE_LISTENER 10.0.2.1:10100 [ ok ]
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index f10ef65a7009..89a30225bbd7 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -2509,6 +2509,54 @@ backup_tests()
> >  	fi
> >  }
> >  
> > +LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> > +LISTENER_CLOSED=16  #MPTCP_EVENT_LISTENER_CLOSED
> > +
> > +AF_INET=2
> > +AF_INET6=10
> > +
> > +verify_listener_events()
> > +{
> > +	local evt=$1
> > +	local e_type=$2
> > +	local e_family=$3
> > +	local e_saddr=$4
> > +	local e_sport=$5
> > +	local type
> > +	local family
> > +	local saddr
> > +	local sport
> > +
> > +	if [ $e_type = $LISTENER_CREATED ]; then
> > +		stdbuf -o0 -e0 printf "\t\t\t\t\t CREATE_LISTENER %s:%s"\
> > +			$e_saddr $e_sport
> > +	elif [ $e_type = $LISTENER_CLOSED ]; then
> > +		stdbuf -o0 -e0 printf "\t\t\t\t\t CLOSE_LISTENER %s:%s "\
> > +			$e_saddr $e_sport
> > +	fi
> > +
> > +	type=$(grep "type:$e_type," $evt |
> > +	       sed --unbuffered -n 's/.*\(type:\)\([[:digit:]]*\).*$/\2/p;q')
> > +	family=$(grep "type:$e_type," $evt |
> > +		 sed --unbuffered -n 's/.*\(family:\)\([[:digit:]]*\).*$/\2/p;q')
> > +	sport=$(grep "type:$e_type," $evt |
> > +		sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
> 
> This does not handle properly error conditions: $family, $type or
> $sport could be undef in case of bugs, you need to quote them
> everywhere.
> 
> > +	if [ $family = $AF_INET6 ]; then
> > +		saddr=$(grep "type:$e_type," $evt |
> > +			sed --unbuffered -n 's/.*\(saddr6:\)\([0-9a-f:.]*\).*$/\2/p;q')
> > +	else
> > +		saddr=$(grep "type:$e_type," $evt |
> > +			sed --unbuffered -n 's/.*\(saddr4:\)\([0-9.]*\).*$/\2/p;q')
> > +	fi
> > +
> > +	if [ $type = $e_type ] && [ $family = $e_family ] &&
> > +	   [ $saddr = $e_saddr ] && [ $sport = $e_sport ]; then
> > +		stdbuf -o0 -e0 printf "[ ok ]\n"
> > +		return 0
> > +	fi
> > +	stdbuf -o0 -e0 printf "[fail]\n"
> 
> This is not enough, you must additionally call
> 
> 	fail_test
> 
> to properly report the failure to the CI, otherwise the test will be
> considered successful even when the condition is not meet.
> 
> > +}
> > +
> >  add_addr_ports_tests()
> >  {
> >  	# signal address with port
> > @@ -2533,7 +2581,16 @@ add_addr_ports_tests()
> >  	fi
> >  
> >  	# single address with port, remove
> > +	# pm listener events
> >  	if reset "remove single address with port"; then
> > +		local evts
> > +		local pid
> > +
> > +		evts=$(mktemp)
> > +		:> $evts
> > +		ip netns exec $ns1 ./pm_nl_ctl events >> $evts 2>&1 &
> > +		pid=$!
> > +
> >  		pm_nl_set_limits $ns1 0 1
> >  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port 10100
> >  		pm_nl_set_limits $ns2 1 1
> > @@ -2541,6 +2598,11 @@ add_addr_ports_tests()
> >  		chk_join_nr 1 1 1
> >  		chk_add_nr 1 1 1
> >  		chk_rm_nr 1 1 invert
> > +
> > +		verify_listener_events $evts 15 $AF_INET 10.0.2.1 10100
> > +		verify_listener_events $evts 16 $AF_INET 10.0.2.1 10100
> > +		kill_wait $pid
> > +		rm -rf $evts

Thanks Paolo, I just sent the squash-to patches to fix them.

> 
> is better if you make the 'evts' variable global, moving its definition
> in the initial global variable declaration and move the 'rm -rf $evts'
> in cleanup() 

Here I prefer to keep 'evts' as a local variable, since all other 'evts'
both in userspace_pm.sh and in do_transfer() in mptcp_join.sh are local
variables.

Thanks,
-Geliang

> 
> @Geliang: could you please send a squash-to follow tacking care of
> the above?
> 
> Thanks!
> 
> Paolo
>