Re: [patch net-next 3/6] selftests: forwarding: add ability to assemble NETIFS array by driver name

[Benjamin Poirier <benjamin.poirier@gmail.com>]

On 2024-04-13 15:27 +0200, Jiri Pirko wrote:
> Fri, Apr 12, 2024 at 10:38:30PM CEST, benjamin.poirier@gmail.com wrote:
> >On 2024-04-12 17:13 +0200, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Allow driver tests to work without specifying the netdevice names.
> >> Introduce a possibility to search for available netdevices according to
> >> set driver name. Allow test to specify the name by setting
> >> NETIF_FIND_DRIVER variable.
> >> 
> >> Note that user overrides this either by passing netdevice names on the
> >> command line or by declaring NETIFS array in custom forwarding.config
> >> configuration file.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> ---
> >>  tools/testing/selftests/net/forwarding/lib.sh | 39 +++++++++++++++++++
> >>  1 file changed, 39 insertions(+)
> >> 
> >> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> >> index 6f6a0f13465f..06633518b3aa 100644
> >> --- a/tools/testing/selftests/net/forwarding/lib.sh
> >> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> >> @@ -55,6 +55,9 @@ declare -A NETIFS=(
> >>  : "${NETIF_CREATE:=yes}"
> >>  : "${NETIF_TYPE:=veth}"
> >>  
> >> +# Whether to find netdevice according to the specified driver.
> >> +: "${NETIF_FIND_DRIVER:=}"
> >> +
> >
> >This section of the file sets default values for variables that can be
> >set by users in forwarding.config. NETIF_FIND_DRIVER is more like
> >NUM_NETIFS, it is set by tests, so I don't think it should be listed
> >there.
> 
> Well, currently there is a mixture of config variables and test
> definitions/requirements. For example REQUIRE_JQ, REQUIRE_MZ, REQUIRE_MTOOLS
> are not forwarding.config configurable (they are, they should not be ;))

Yes, that's true. If you prefer to leave that statement there, go ahead.

> Where do you suggest to move NETIF_FIND_DRIVER?

I would make NETIF_FIND_DRIVER like NUM_NETIFS, ie. there's no statement
setting a default value for it. And I would move the comment describing
its purpose above this new part:

> +
> +if [[ ! -z $NETIF_FIND_DRIVER ]]; then
> +	unset NETIFS
> +	declare -A NETIFS
> +	find_netif
> +fi
> +

BTW, '! -z' can be removed from that test. It's equivalent to:
if [[ $NETIF_FIND_DRIVER ]]; then