On Mon, 5 Mar 2018 22:26:42 -0700
David Ahern wrote:
> On 3/5/18 3:45 PM, Stefano Brivio wrote:
> > diff --git a/tools/testing/selftests/net/pmtu.sh
> > b/tools/testing/selftests/net/pmtu.sh
> > new file mode 100755
> > index ..eb186ca3e5e4
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/pmtu.sh
> > @@ -0,0 +1,159 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Check that route PMTU values match expectations
> > +#
> > +# Tests currently implemented:
> > +#
> > +# - test_pmtu_vti6_exception
> > +# Set up vti6 tunnel on top of veth, with xfrm states and policies, in two
> > +# namespaces with matching endpoints. Check that route exception is
> > +# created by exceeding link layer MTU with ping to other endpoint. Then
> > +# decrease and increase MTU of tunnel, checking that route exception PMTU
> > +# changes accordingly
> > +
> > +NS_A="ns-$(mktemp -u XX)"
> > +NS_B="ns-$(mktemp -u XX)"
> > +ns_a="ip netns exec ${NS_A}"
> > +ns_b="ip netns exec ${NS_B}"
> > +
> > +veth6_a_addr="fd00:1::a"
> > +veth6_b_addr="fd00:1::b"
> > +veth6_mask="64"
> > +
> > +vti6_a_addr="fd00:2::a"
> > +vti6_b_addr="fd00:2::b"
> > +vti6_mask="64"
> > +
> > +setup_namespaces() {
> > + ip netns add ${NS_A} || return 1
> > + ip netns add ${NS_B}
>
> for basic config commands that should work every encasing in
> set -e
> ...
> set +e
>
> simplifies error handling. IMO, it is relevant for netns and veth config
> commands. Not so much for the xfrm commands which need to load modules
> or depend on features that need config options.
Still I would prefer in that case to handle the error explicitly and be
verbose about what went wrong, because also netns and veth might be
missing. Sure, USER_NS is in 'config', but one might actually not use
that file, or try to run this as a single test.
> > +
> > + return 0
> > +}
> > +
> > +setup_veth() {
> > + ${ns_a} ip link add veth_a type veth peer name veth_b || return 1
> > + ${ns_a} ip link set veth_b netns ${NS_B}
> > +
> > + ${ns_a} ip link set veth_a up
> > + ${ns_b} ip link set veth_b up
> > +
> > + ${ns_a} ip addr add ${veth6_a_addr}/${veth6_mask} dev veth_a
> > + ${ns_b} ip addr add ${veth6_b_addr}/${veth6_mask} dev veth_b
> > +
> > + return 0
> > +}
> > +
> > +setup_vti6() {
> > + ${ns_a} ip link add vti_a type vti6 local ${veth6_a_addr} remote
> > ${veth6_b_addr} key 10 || return 1
> > + ${ns_b} ip link add vti_b type vti6 local ${veth6_b_addr} remote
> > ${veth6_a_addr} key 10
> > +
> > + ${ns_a} ip link set vti_a up
> > + ${ns_b} ip link set vti_b up
> > +
> > + ${ns_a} ip addr add ${vti6_a_addr}/${vti6_mask} dev vti_a
> > + ${ns_b} ip addr add ${vti6_b_addr}/${vti6_mask} dev vti_b
> > +
> > + return 0
> > +}
> > +
> > +setup_xfrm() {
> > + ${ns_a} ip -6 xfrm state add src ${veth6_a_addr} dst ${veth6_b_addr}
> > spi 0x1000 proto esp aead "rfc4106(gcm(aes))"
> > 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel || return 1
> > + ${ns_a} ip -6 xfrm state add src ${veth6_b_addr} dst ${veth6_a_addr}
> > spi 0x1001 proto esp aead "rfc4106(gcm(aes))"
> > 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > + ${ns_a} ip -6 xfrm policy add dir out mark 10 tmpl src ${veth6_a_addr}
> > dst ${veth6_b_addr} proto esp mode tunnel
> > + ${ns_a} ip -6 xfrm policy add dir in mark 10 tmpl src ${veth6_b_addr}
> > dst ${veth6_a_addr} proto esp mode tunnel
> > +
> > + ${ns_b} ip -6 xfrm state add src ${veth6_a_addr} dst ${veth6_b_addr}
> > spi 0x1000 proto esp aead "rfc4106(gcm(aes))"
> > 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > + ${ns_b} ip -6 xfrm state add src ${veth6_b_addr} dst ${veth6_a_addr}
> > spi 0x1001 proto esp aead "rfc4106(gcm(aes))"
> > 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f 128 mode tunnel
> > + ${ns_b} ip -6 xfrm policy add dir out mark 10 tmpl src ${veth6_b_addr}
> > dst ${veth6_a_addr} proto esp mode tunnel
> > + ${ns_b} ip -6 xfrm policy add dir in mark 10 tmpl src ${veth6_a_addr}
> > dst ${veth6_b_addr} proto esp mode tunnel
> > +
> > + return 0
> > +}
> > +
> > +setup() {
> > + tunnel_type="$1"
> > +
> > + [ "$(id -u)" -ne 0 ] && (echo "SKIP: need to run as root" && exit 0)
> > +
> > + setup_namespaces || (echo "SKIP: namespaces not supported" && exit 0)
> > + setup_veth || (echo "SKIP: veth not supported" && exit 0)
>
> You use this style (' || (...)' or ' && (...)') a lot
> and it does not actually exit the script. You can verify by adding
> 'return 1' to either function.
I forgot that runs in a subshell, will fix in v2 by reversing the
return values, so that I can just have function && echo ... && exit 0,
which is not ambiguous on any shell.
> > +
> > + case ${tunnel_type} in
> > + "vti6")
> > + setup_vti6 && (echo "SKIP: vti6 not supported" && exit 0)
> > + setup_xfrm && (echo "SKIP: xfrm not supported" && exit 0)
> > + ;;
> > + *)
> > +