Re: [PATCH net-next v3 3/9] selftests: net: extend lib.sh to parse drivers/net/net.config

[Ioana Ciornei]

On Fri, Mar 20, 2026 at 11:19:18AM +0100, Petr Machata wrote:
> 
> Ioana Ciornei <ioana.ciornei@xxxxxxx> writes:
> 
> > Extend lib.sh so that it's able to parse driver/net/net.config and
> > environment variables such as NETIF, REMOTE_TYPE, LOCAL_V4 etc described
> > in drivers/net/README.rst.
> >
> > In order to make the transition towards running with a single local
> > interface smoother for the bash networking driver tests, beside sourcing
> > the net.config file also translate the new env variables into the old
> > style based on the NETIFS array. Since the NETIFS array only holds the
> > network interface names, also add a new array - TARGETS - which keeps
> > track of the target on which a specific interfaces resides - local,
> > netns or accesible through an ssh command.
> >
> > For example, a net.config which looks like below:
> >
> > 	NETIF=eth0
> > 	LOCAL_V4=192.168.1.1
> > 	REMOTE_V4=192.168.1.2
> > 	REMOTE_TYPE=ssh
> > 	REMOTE_ARGS=root@192.168.1.2
> >
> > will generate the NETIFS and TARGETS arrays with the following data.
> >
> > 	NETIFS[p1]="eth0"
> > 	NETIFS[p2]="eth2"
> >
> > 	TARGETS[eth0]="local:"
> > 	TARGETS[eth2]="ssh:root@192.168.1.2"
> >
> > The above will be true if on the remote target, the interface which has
> > the 192.168.1.2 address is named eth2.
> >
> > Since the TARGETS array is indexed by the network interface name,
> > document a new restriction README.rst which states that the remote
> > interface cannot have the same name as the local one.
> 
> Isn't this going to be a somewhat common scenario though?

It could be if you the remote target is another system just like the
DUT. But I couldn't find an easy representation for the relationship
between interfaces and their targets that also removes this restriction.

> 
> > Also keep the old way of populating the NETIFS variable based on the
> > command line arguments. This will be invoked in case NETIF is not
> > defined.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > ---
> > Changes in v3:
> > - s/TARGET/CUR_TARGET
> > - this used to be patch #2/9 in v2. Swapped the two patches so that the
> >   run_cmd used in this patch is defined earlier, not later.
> > Changes in v2:
> > - patch is new
> >
> >  .../testing/selftests/drivers/net/README.rst  |   3 +
> >  tools/testing/selftests/net/forwarding/lib.sh | 130 ++++++++++++++++--
> >  2 files changed, 124 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> > index c94992acf10b..8d8d9d62e763 100644
> > --- a/tools/testing/selftests/drivers/net/README.rst
> > +++ b/tools/testing/selftests/drivers/net/README.rst
> > @@ -26,6 +26,9 @@ The netdevice against which tests will be run must exist, be running
> >  Refer to list of :ref:`Variables` later in this file to set up running
> >  the tests against a real device.
> >  
> > +Also, make sure that if you are using a remote machine for traffic injection,
> > +the local and remote interfaces have different names.
> > +
> >  Both modes required
> >  ~~~~~~~~~~~~~~~~~~~
> >  
> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > index cf40cb766c68..fb5aa56343e1 100644
> > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > @@ -3,6 +3,7 @@
> >  
> >  ##############################################################################
> >  # Topology description. p1 looped back to p2, p3 to p4 and so on.
> > +#shellcheck disable=SC2034 # SC doesn't see our uses of global variables
> >  
> >  declare -A NETIFS=(
> >      [p1]=veth0
> > @@ -17,6 +18,26 @@ declare -A NETIFS=(
> >      [p10]=veth9
> >  )
> >  
> > +# Array indexed by the network interface name keeping track of the target on
> > +# which the interface resides. Values will be strings of the following format -
> > +# <type>:<args>.
> > +# TARGETS[eth0]="local:" - meaning that the eth0 interface is accessible locally
> > +# TARGETS[eth1]="netns:foo" - eth1 is in the foo netns
> > +# TARGETS[eth2]="ssh:root@10.0.0.2" - eth2 is accessible through running the
> > +# 'ssh root@10.0.0.2' command.
> > +declare -A TARGETS=(
> 
> This is a helper array for internal use only, not a user interface. It
> should just be declared suitably close to where it's actually
> initialized. It doesn't even need its own initialization, there's code
> down there to initialize it accordingly.

Ok. In v3 I added the fallback to run on the local system in case either
TARGETS is not declared or the entry is not valid so I can safely remove
this now. Thanks!

> 
> > +    [veth0]="local:"
> > +    [veth1]="local:"
> > +    [veth2]="local:"
> > +    [veth3]="local:"
> > +    [veth4]="local:"
> > +    [veth5]="local:"
> > +    [veth6]="local:"
> > +    [veth7]="local:"
> > +    [veth8]="local:"
> > +    [veth9]="local:"
> > +)
> > +
> >  # Port that does not have a cable connected.
> >  : "${NETIF_NO_CABLE:=eth8}"
> >  
> > @@ -340,17 +361,108 @@ fi
> >  ##############################################################################
> >  # Command line options handling
> >  
> > -count=0
> > +check_env() {
> > +	local vars_needed=("LOCAL_V4,LOCAL_V6"
> > +			   "REMOTE_V4,REMOTE_V6"
> > +			   "REMOTE_TYPE"
> > +			   "REMOTE_ARGS")
> 
> Given how much hand-rolled code is needed for cross-checking local x
> remote config symmetry anyway, I wonder if this whole thing should just
> be very simply:
> 
> 	if [[ ! (( -n "$LOCAL_V4" && -n "$REMOTE_V4") ||
> 		 ( -n "$LOCAL_V6" && -n "$REMOTE_V6" )) ]]; then
> 		echo "SKIP: Invalid environment, missing or inconsistent LOCAL_V4/REMOTE_V4/LOCAL_V6/REMOTE_V6"
>                 exit "$ksft_skip"
>         fi
> 
> Then you still need a manual check for REMOTE_TYPE and REMOTE_ARGS, but
> that's still just three checks all told like the current code, and we
> get rid of the double nested loop that's kinda tricky to understand. It
> doesn't look like the generalization really pays off here.

I took as an example the _check_env in env.py and it got to look way
worse in bash than necessary, agreed. Will simplify it, thanks!

> 
> > +	local missing=()
> > +	local choice
> > +
> > +	# If a choice has multiple comma separated options, at least one must
> > +	# exist
> > +	for choice in "${vars_needed[@]}"; do
> > +		IFS=',' read -ra entries <<< "$choice"
> 
> Entries should be declared as local.

Will do.

> > +
> > +		local found=0
> > +		for entry in "${entries[@]}"; do
> > +			if [[ -n "${!entry}" ]]; then
> > +				found=1
> > +				break
> > +			fi
> > +		done
> >  
> > -while [[ $# -gt 0 ]]; do
> > -	if [[ "$count" -eq "0" ]]; then
> > -		unset NETIFS
> > -		declare -A NETIFS
> > +		if [[ $found -eq 0 ]]; then
> > +			missing+=("$choice")
> > +		fi
> > +	done
> > +
> > +	# Make sure v4 / v6 configs are symmetric
> > +	if [[ (-n "${LOCAL_V6}" && -z "${REMOTE_V6}") || \
> > +	      (-z "${LOCAL_V6}" && -n "${REMOTE_V6}") ]]; then
> > +		missing+=("LOCAL_V6,REMOTE_V6")
> >  	fi
> > -	count=$((count + 1))
> > -	NETIFS[p$count]="$1"
> > -	shift
> > -done
> > +
> > +	if [[ (-n "${LOCAL_V4}" && -z "${REMOTE_V4}") || \
> > +	      (-z "${LOCAL_V4}" && -n "${REMOTE_V4}") ]]; then
> > +		missing+=("LOCAL_V4,REMOTE_V4")
> > +	fi
> > +
> > +	if [[ ${#missing[@]} -gt 0 ]]; then
> > +		echo "SKIP: Invalid environment, missing configuration: ${missing[*]}"
> > +		echo "Please see tools/testing/selftests/drivers/net/README.rst"
> > +		exit "$ksft_skip"
> > +	fi
> > +}
> > +
> > +get_ifname_by_ip()
> > +{
> > +	local ip_addr=$1; shift
> > +
> > +	run_cmd ip -j addr show to "$ip_addr" | jq -r '.[].ifname'
> > +}
> > +
> > +# If there is a configuration file, source it
> > +if [[ -f $net_forwarding_dir/../../drivers/net/net.config ]]; then
> > +	source "$net_forwarding_dir/../../drivers/net/net.config"
> > +fi
> 
> This is going to impact all tests though, not just those that are meant
> to work with this. If I have this file, it sets NETIF, and all my
> forwarding tests (presumably) break, because NETIFS doesn't get
> configured correctly.
> 
> The requirements for driver tests are different from forwarding tests.
> In particular, interfaces do not belong to driver tests, the test can't
> bring them up or down, reassign addresses etc., merely to use them for
> traffic. (Which you know, I'm just stating it for context.) So I think
> there needs to be an opt-in variable for this stuff. Like setting
> DRIVER_TEST_CONFORMANT=yes before sourcing the library or whatever.

I had this kind of approach initially but thought that it might be
desirable to also maintain the possibility to run the driver tests as
./test.sh eth0 eth1 as before, so I changed it.
I can move back to that approach.

> 
> Then NETIF ceases to be a tell and becomes an environment requirement
> for check_env to validate like the others.
> 
> (Another alternative is to extract a common core from lib.sh and have
> both lib.sh and net/driver/lib.sh use that. But that's more of a change
> and I'm not even sure it's the right way to slice it.)
> 
> > +# In case NETIF is specified, then the test expects to pass the arguments
> > +# through the variables specified in drivers/net/README.rst file. If not,
> > +# fallback on parsing the script arguments for interface names.
> > +if [[ -v NETIF ]]; then
> > +	if (( NUM_NETIFS > 2)); then
> > +		echo "SKIP: NETIF defined and NUM_NETIFS is bigger than 2"
> > +		exit "$ksft_skip"
> > +	fi
> > +
> > +	check_env
> > +
> > +	# Populate the NETIF and TARGETS arrays automatically based on the
> > +	# environment variables
> > +	unset NETIFS
> > +	declare -A NETIFS
> > +
> > +	NETIFS[p1]="$NETIF"
> > +	TARGETS[$NETIF]="local:"
> > +
> > +	# Locate the name of the remote interface
> > +	if [[ -v REMOTE_V4 ]]; then
> > +		remote_netif=$(CUR_TARGET="$REMOTE_TYPE:$REMOTE_ARGS" get_ifname_by_ip "$REMOTE_V4")
> > +	else
> > +		remote_netif=$(CUR_TARGET="$REMOTE_TYPE:$REMOTE_ARGS" get_ifname_by_ip "$REMOTE_V6")
> > +	fi
> > +	if [[ ! -n "$remote_netif" ]]; then
> > +		echo "SKIP: cannot find remote interface"
> > +		exit "$ksft_skip"
> > +	fi
> 
> This should check that $NETIF != $remote_netif.

Ok.