[v1] selftests: forwarding: lib: rewrite processing of command line arguments

[PATCH net-next] selftests: forwarding: lib: rewrite processing of command line arguments

Posted by Ioana Ciornei 3 days, 20 hours ago

The piece of code which processes the command line arguments and
populates NETIFS based on them is really unobvious. Rewrite it so that
the intention is clear and the code is easy to follow.

Suggested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index d8cc4c64148d..922cdaf2ceb9 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -466,18 +466,18 @@ if [ "${DRIVER_TEST_CONFORMANT}" = "yes" ]; then
 	NETIFS[p2]="$remote_netif"
 	TARGETS[$remote_netif]="$REMOTE_TYPE:$REMOTE_ARGS"
 else
-	count=0
+	# Prime NETIFS from the command line, but retain if none given.
+	if [[ $# -gt 0 ]]; then
+		unset NETIFS
+		declare -A NETIFS
 
-	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
+		while [[ $# -gt 0 ]]; do
+			count=$((count + 1))
+			NETIFS[p$count]="$1"
+			TARGETS[$1]="local:"
+			shift
+		done
+	fi
 fi
 
 ##############################################################################
-- 
2.25.1

Re: [PATCH net-next] selftests: forwarding: lib: rewrite processing of command line arguments

Posted by Simon Horman 19 hours ago

On Fri, Apr 03, 2026 at 05:19:12PM +0300, Ioana Ciornei wrote:
> The piece of code which processes the command line arguments and
> populates NETIFS based on them is really unobvious. Rewrite it so that
> the intention is clear and the code is easy to follow.
> 
> Suggested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 22 +++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index d8cc4c64148d..922cdaf2ceb9 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -466,18 +466,18 @@ if [ "${DRIVER_TEST_CONFORMANT}" = "yes" ]; then
>  	NETIFS[p2]="$remote_netif"
>  	TARGETS[$remote_netif]="$REMOTE_TYPE:$REMOTE_ARGS"
>  else
> -	count=0

AI generated review on Sashiko.dev suggests that initialising count should
be retained to ensure correct behaviour in the (unlikely) case that count
is already set (to a non-zero value) in the environment.

And this seems like a good idea to me.

> +	# Prime NETIFS from the command line, but retain if none given.
> +	if [[ $# -gt 0 ]]; then
> +		unset NETIFS
> +		declare -A NETIFS
>  
> -	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
> +		while [[ $# -gt 0 ]]; do
> +			count=$((count + 1))
> +			NETIFS[p$count]="$1"
> +			TARGETS[$1]="local:"
> +			shift
> +		done
> +	fi
>  fi
>  
>  ##############################################################################
> -- 
> 2.25.1
>

Re: [PATCH net-next] selftests: forwarding: lib: rewrite processing of command line arguments

Posted by Ioana Ciornei an hour ago

On Mon, Apr 06, 2026 at 04:31:50PM +0100, Simon Horman wrote:
> On Fri, Apr 03, 2026 at 05:19:12PM +0300, Ioana Ciornei wrote:
> > The piece of code which processes the command line arguments and
> > populates NETIFS based on them is really unobvious. Rewrite it so that
> > the intention is clear and the code is easy to follow.
> > 
> > Suggested-by: Petr Machata <petrm@nvidia.com>
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  tools/testing/selftests/net/forwarding/lib.sh | 22 +++++++++----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> > index d8cc4c64148d..922cdaf2ceb9 100644
> > --- a/tools/testing/selftests/net/forwarding/lib.sh
> > +++ b/tools/testing/selftests/net/forwarding/lib.sh
> > @@ -466,18 +466,18 @@ if [ "${DRIVER_TEST_CONFORMANT}" = "yes" ]; then
> >  	NETIFS[p2]="$remote_netif"
> >  	TARGETS[$remote_netif]="$REMOTE_TYPE:$REMOTE_ARGS"
> >  else
> > -	count=0
> 
> AI generated review on Sashiko.dev suggests that initialising count should
> be retained to ensure correct behaviour in the (unlikely) case that count
> is already set (to a non-zero value) in the environment.
> 
> And this seems like a good idea to me.

Ok. Will add this in v2.