Re: [PATCH mptcp-next v2 1/4] selftests: mptcp: make evts global in userspace_pm

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

On Mon, Nov 28, 2022 at 09:04:44AM +0100, Paolo Abeni wrote:
> Hi,
> 
> On Fri, 2022-11-25 at 21:40 +0800, Geliang Tang wrote:
> > @@ -134,18 +140,25 @@ make_connection()
> >  
> >  	# Capture netlink events over the two network namespaces running
> >  	# the MPTCP client and server
> > -	local client_evts
> > -	client_evts=$(mktemp)
> > +	if [ -z "$client_evts" ]; then
> > +		client_evts=$(mktemp)
> > +	fi
> >  	:>"$client_evts"
> > +	if [ $client_evts_pid -ne 0 ]; then
> > +		kill_wait $client_evts_pid
> > +	fi
> >  	ip netns exec "$ns2" ./pm_nl_ctl events >> "$client_evts" 2>&1 &
> > -	local client_evts_pid=$!
> > -	local server_evts
> > -	server_evts=$(mktemp)
> > +	client_evts_pid=$!
> > +	if [ -z "$server_evts" ]; then
> > +		server_evts=$(mktemp)
> > +	fi
> >  	:>"$server_evts"
> > +	if [ $server_evts_pid -ne 0 ]; then
> > +		kill_wait $server_evts_pid
> > +	fi
> >  	ip netns exec "$ns1" ./pm_nl_ctl events >> "$server_evts" 2>&1 &
> > -	local server_evts_pid=$!
> > +	server_evts_pid=$!
> >  	sleep 0.5
> > -
> 
> Minor nit: I would not remove this empty line here.

This has been removed in v3 in the ML.

> 
> >  	# Run the server
> >  	ip netns exec "$ns1" \
> >  	   ./mptcp_connect -s MPTCP -w 300 -p $app_port -l $listen_addr > /dev/null 2>&1 &
> > @@ -161,7 +174,6 @@ make_connection()
> >  	sleep 1
> >  
> >  	# Capture client/server attributes from MPTCP connection netlink events
> > -	kill_wait $client_evts_pid
> >  
> >  	local client_token
> >  	local client_port
> > @@ -173,12 +185,11 @@ make_connection()
> >  	client_port=$(sed --unbuffered -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$client_evts")
> >  	client_serverside=$(sed --unbuffered -n 's/.*\(server_side:\)\([[:digit:]]*\).*$/\2/p;q'\
> >  				      "$client_evts")
> > -	kill_wait $server_evts_pid
> >  	server_token=$(grep "type:1," "$server_evts" |
> >  		       sed --unbuffered -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
> >  	server_serverside=$(grep "type:1," "$server_evts" |
> >  			    sed --unbuffered -n 's/.*\(server_side:\)\([[:digit:]]*\).*$/\2/p;q')
> > -	rm -f "$client_evts" "$server_evts" "$file"
> > +	rm -f "$file"
> 
> At this point is better to move even 'file' as a global variable, for
> the same reason as 'evts'

I just sent a squash-to patch against v3 to fix this.

Thanks,
-Geliang

> 
> >  
> >  	if [ "$client_token" != "" ] && [ "$server_token" != "" ] && [ "$client_serverside" = 0 ] &&
> >  		   [ "$server_serverside" = 1 ]
> 
> Thanks!
> 
> Paolo
>