Hangbin Liu <liuhang...@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"
> +}
