Re: [PATCH net-next 01/38] selftests/net: add lib.sh

[Petr Machata <petrm@nvidia.com>]

Hangbin Liu <liuhangbin@gmail.com> writes:

> +cleanup_ns()
> +{
> +	local ns=""
> +	local errexit=0
> +
> +	# disable errexit temporary
> +	if [[ $- =~ "e" ]]; then
> +		errexit=1
> +		set +e
> +	fi
> +
> +	for ns in "$@"; do
> +		ip netns delete "${ns}" &> /dev/null
> +		busywait 2 "ip netns list | grep -vq $1" &> /dev/null

The grep would get confused by substrings of other names.
This should be grep -vq "^$ns$".

> +		if ip netns list | grep -q $1; then

Busywait returns != 0 when the wait condition is not reached within a
given time. So it should be possible to roll the duplicated if-grep into
the busywait line like so:

		if ! busywait 2 "ip netns etc."; then

> +			echo "Failed to remove namespace $1"
> +			return $ksft_skip

This does not restore the errexit.

I think it might be clearest to have this function as a helper, say
__cleanup_ns, and then have a wrapper that does the errexit management:

cleanup_ns()
{
	local errexit
	local rc

	# disable errexit temporarily
	if [[ $- =~ "e" ]]; then
		errexit=1
		set +e
	fi

	__cleanup_ns "$@"
	rc=$?

	[ $errexit -eq 1 ] && set -e
	return $rc
}

If this comes up more often, we can have a helper like
with_disabled_errexit or whatever, that does this management and
dispatches to "$@", so cleanup_ns() would become:

cleanup_ns()
{
	with_disabled_errexit __cleanup_ns "$@"
}

> +		fi
> +	done
> +
> +	[ $errexit -eq 1 ] && set -e
> +	return 0
> +}
> +
> +# By default, remove all netns before EXIT.
> +cleanup_all_ns()
> +{
> +	cleanup_ns $NS_LIST
> +}
> +trap cleanup_all_ns EXIT

Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
because basically all users of forwarding/lib.sh use the EXIT trap.

I wonder if we need something like these push_cleanup / on_exit helpers:

	https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15

But I don't want to force this on your already large patchset :)
So just ignore the bit about including from forwarding/lib.sh.

> +# setup netns with given names as prefix. e.g
> +# setup_ns local remote
> +setup_ns()
> +{
> +	local ns=""
> +	# the ns list we created in this call
> +	local ns_list=""
> +	while [ -n "$1" ]; do

I would find it more readable if this used the same iteration approach
as the 'for ns in "$@"' above. The $1/shift approach used here is
somewhat confusing.

> +		# Some test may setup/remove same netns multi times
> +		if unset $1 2> /dev/null; then
> +			ns="${1,,}-$(mktemp -u XXXXXX)"
> +			eval readonly $1=$ns
> +		else
> +			eval ns='$'$1
> +			cleanup_ns $ns
> +
> +		fi
> +
> +		ip netns add $ns
> +		if ! ip netns list | grep -q $ns; then

As above, the grep could get confused. But in fact wouldn't just
checking the exit code of ip netns add be enough?

> +			echo "Failed to create namespace $1"
> +			cleanup_ns $ns_list
> +			return $ksft_skip
> +		fi
> +		ip -n $ns link set lo up
> +		ns_list="$ns_list $ns"
> +
> +		shift
> +	done
> +	NS_LIST="$NS_LIST $ns_list"
> +}