Re: [PATCH net-next v4 03/10] selftests: net: extend lib.sh to parse drivers/net/net.config

[Petr Machata <petrm@nvidia.com>]

Ioana Ciornei <ioana.ciornei@nxp.com> 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.
>
> 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@nxp.com>
> ---
> Changes in v4:
> - reword the entry in README.rst to mention that the different interface
>   names is only a bash restriction and the python infrastructure does
>   not have the same problem.
> - only declare the TARGETS array when necessary

I guess you mean at the point where it's necessary, instead of it being
a user API. Right now TARGETS is defined always, and I think that's
better than having it only defined sometimes.

> - add a new flags - DRIVER_TEST_CONFORMANT - that needs to be set by the
>   test
> - rework the check_env() function so that its logic is simpler
> - source drivers/net/net.config only if DRIVER_TEST_CONFORMANT == yes
> - check that NETIF and the remote netif have different names and abort
>   test is not
>
> 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  |   4 +
>  tools/testing/selftests/net/forwarding/lib.sh | 106 ++++++++++++++++--
>  2 files changed, 101 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
> index c94992acf10b..1897aa1583ec 100644
> --- a/tools/testing/selftests/drivers/net/README.rst
> +++ b/tools/testing/selftests/drivers/net/README.rst
> @@ -26,6 +26,10 @@ 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.
>  
> +The current support for bash tests restricts the use of the same interface name
> +on the local system and the remote one and will bail if this case is
> +encountered.
> +
>  Both modes required
>  ~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 3009ce00c5dc..93b865681840 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

The shellcheck line should be moved elsewhere. The comment is related to
NETIFS, and it's weird there's a shellcheck declaration between a
comment and the thing it's commenting.

>  declare -A NETIFS=(
>      [p1]=veth0
> @@ -85,6 +86,9 @@ declare -A NETIFS=(
>  # e.g. a low-power board.
>  : "${KSFT_MACHINE_SLOW:=no}"
>  
> +# Whether the test is conforming to the requirements and usage described in
> +# drivers/net/README.rst.
> +: "${DRIVER_TEST_CONFORMANT:=no}"

Not sure this makes sense as user API either honestly. Given the
differences between requirements of the two test types I can't imagine a
non-conformant test would be runnable like that. The actual line is OK,
but it should be moved probably above the if check, so as not to
indicate it's usable as a user API.

>  ##############################################################################
>  # Find netifs by test-specified driver name
>  
> @@ -340,17 +344,101 @@ fi
>  ##############################################################################
>  # Command line options handling
>  
> -count=0
> +check_env() {
> +	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"
> +		echo "Please see tools/testing/selftests/drivers/net/README.rst"
> +		exit "$ksft_skip"
> +	fi
>  
> -while [[ $# -gt 0 ]]; do
> -	if [[ "$count" -eq "0" ]]; then
> -		unset NETIFS
> -		declare -A NETIFS
> +	if [[ -z "$REMOTE_TYPE" ]]; then
> +		echo "SKIP: Invalid environment, missing REMOTE_TYPE"
> +		exit "$ksft_skip"
>  	fi
> -	count=$((count + 1))
> -	NETIFS[p$count]="$1"
> -	shift
> -done
> +
> +	if [[ -z "$REMOTE_ARGS" ]]; then
> +		echo "SKIP: Invalid environment, missing REMOTE_ARGS"
> +		exit "$ksft_skip"
> +	fi
> +}
> +
> +get_ifname_by_ip()
> +{
> +	local target=$1; shift
> +	local ip_addr=$1; shift
> +
> +	__run_on "$target" ip -j addr show to "$ip_addr" | jq -r '.[].ifname'
> +}
> +
> +# Based on DRIVER_TEST_CONFORMANT, decide if to source drivers/net/net.config
> +# or not. In the "yes" case, 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 [ "${DRIVER_TEST_CONFORMANT}" = "yes" ]; then
> +	if [[ -f $net_forwarding_dir/../../drivers/net/net.config ]]; then
> +		source "$net_forwarding_dir/../../drivers/net/net.config"
> +	fi
> +
> +	if (( NUM_NETIFS > 2)); then
> +		echo "SKIP: DRIVER_TEST_CONFORMANT=yes and NUM_NETIFS is bigger than 2"
> +		exit "$ksft_skip"
> +	fi
> +
> +	check_env
> +
> +	# Populate the NETIFS and TARGETS arrays automatically based on the
> +	# environment variables. The TARGETS array is 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.
> +
> +	unset NETIFS
> +	declare -A NETIFS
> +	declare -A TARGETS
> +
> +	NETIFS[p1]="$NETIF"
> +	TARGETS[$NETIF]="local:"
> +
> +	# Locate the name of the remote interface
> +	remote_target="$REMOTE_TYPE:$REMOTE_ARGS"
> +	if [[ -v REMOTE_V4 ]]; then
> +		remote_netif=$(get_ifname_by_ip "$remote_target" "$REMOTE_V4")
> +	else
> +		remote_netif=$(get_ifname_by_ip "$remote_target" "$REMOTE_V6")
> +	fi
> +	if [[ ! -n "$remote_netif" ]]; then
> +		echo "SKIP: cannot find remote interface"
> +		exit "$ksft_skip"
> +	fi
> +
> +	if [[ "$NETIF" == "$remote_netif" ]]; then
> +		echo "SKIP: local and remote interfaces cannot have the same name"
> +		exit "$ksft_skip"
> +	fi
> +
> +	NETIFS[p2]="$remote_netif"
> +	TARGETS[$remote_netif]="$REMOTE_TYPE:$REMOTE_ARGS"
> +else
> +	count=0

This leg is missing declare -A TARGETS.

Since both legs need to declare it, why not move it up above the if?

> +	while [[ $# -gt 0 ]]; do
> +		if [[ "$count" -eq "0" ]]; then
> +			unset NETIFS
> +			declare -A NETIFS
> +		fi
> +		count=$((count + 1))
> +		NETIFS[p$count]="$1"
> +		TARGETS[$1]="local:"
> +		shift
> +	done
> +fi
>  
>  ##############################################################################
>  # Network interfaces configuration