Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/3] selftests: Add VRF icmp error route lookup test
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        David Ahern <dsahern@kernel.org>
Cc:     linux-kernel@vger.kernel.org,
        Michael Jeanson <mjeanson@efficios.com>,
        "David S . Miller" <davem@davemloft.net>, netdev@vger.kernel.org
References: <20200811195003.1812-1-mathieu.desnoyers@efficios.com>
 <20200811195003.1812-2-mathieu.desnoyers@efficios.com>
From:   David Ahern <dsahern@gmail.com>
Message-ID: <49f26c7d-b8aa-57e7-02c6-424bec9c3845@gmail.com>
Date:   Thu, 13 Aug 2020 17:13:55 -0600
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0)
 Gecko/20100101 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200811195003.1812-2-mathieu.desnoyers@efficios.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 8/11/20 1:50 PM, Mathieu Desnoyers wrote:
> +run_cmd()
> +{
> +	local cmd="$*"
> +	local out
> +	local rc
> +
> +	if [ "$VERBOSE" = "1" ]; then
> +		echo "COMMAND: $cmd"
> +	fi
> +
> +	out=$(eval $cmd 2>&1)
> +	rc=$?
> +	if [ "$VERBOSE" = "1" ] && [ -n "$out" ]; then
> +		echo "$out"
> +	fi
> +
> +	[ "$VERBOSE" = "1" ] && echo
> +
> +	return $rc
> +}
> +

...

> +ipv6_ping()
> +{
> +	log_section "IPv6: VRF ICMP error route lookup ping"
> +
> +	setup
> +
> +	# verify connectivity
> +	if ! check_connectivity6; then
> +		echo "Error: Basic connectivity is broken"
> +		ret=1
> +		return
> +	fi
> +
> +	if [ "$VERBOSE" = "1" ]; then
> +		echo "Command to check for ICMP ttl exceeded:"
> +		run_cmd ip netns exec h1 "${ping6}" -t1 -c1 -W2 ${H2_N2_IP6}
> +	fi
> +
> +	ip netns exec h1 "${ping6}" -t1 -c1 -W2 ${H2_N2_IP6} | grep -q "Time exceeded: Hop limit"

run_cmd runs the command and if VERBOSE is set to 1 shows the command to
the user. Something is off with this script and passing the -v arg -- I
do not get a command list. This applies to the whole script.

Since you need to check for output, I suggest modifying run_cmd to
search the output for the given string.


> +	log_test $? 0 "Ping received ICMP ttl exceeded"
> +}
> +################################################################################

missing newline between '}' and '####'

> +# usage
> +
> +usage()
> +{
> +        cat <<EOF
> +usage: ${0##*/} OPTS
> +
> +	-4          IPv4 tests only
> +	-6          IPv6 tests only
> +	-p          Pause on fail
> +	-v          verbose mode (show commands and output)
> +EOF
> +}
> +
> +################################################################################
> +# main
> +
> +# Some systems don't have a ping6 binary anymore
> +command -v ping6 > /dev/null 2>&1 && ping6=$(command -v ping6) || ping6=$(command -v ping)
> +
> +TESTS_IPV4="ipv4_ping ipv4_traceroute"
> +TESTS_IPV6="ipv6_ping ipv6_traceroute"
> +
> +ret=0
> +nsuccess=0
> +nfail=0
> +setup=0
> +
> +while getopts :46pvh o
> +do
> +	case $o in
> +		4) TESTS=ipv4;;
> +		6) TESTS=ipv6;;
> +                p) PAUSE_ON_FAIL=yes;;
> +                v) VERBOSE=1;;
> +		h) usage; exit 0;;
> +                *) usage; exit 1;;

indentation issues; not using tabs

> +	esac
> +done
> +
> +#
> +# show user test config
> +#
> +if [ -z "$TESTS" ]; then
> +        TESTS="$TESTS_IPV4 $TESTS_IPV6"
> +elif [ "$TESTS" = "ipv4" ]; then
> +        TESTS="$TESTS_IPV4"
> +elif [ "$TESTS" = "ipv6" ]; then
> +        TESTS="$TESTS_IPV6"
> +fi
> +
> +for t in $TESTS
> +do
> +	case $t in
> +	ipv4_ping|ping)             ipv4_ping;;
> +	ipv4_traceroute|traceroute) ipv4_traceroute;;
> +
> +	ipv6_ping|ping)             ipv6_ping;;
> +	ipv6_traceroute|traceroute) ipv6_traceroute;;
> +
> +	# setup namespaces and config, but do not run any tests
> +	setup)                      setup; exit 0;;

you don't allow '-t setup' so you can remove this part

> +
> +	help)                       echo "Test names: $TESTS"; exit 0;;
> +	esac
> +done
> +
> +cleanup
> +
> +printf "\nTests passed: %3d\n" ${nsuccess}
> +printf "Tests failed: %3d\n"   ${nfail}
> +
> +exit $ret
>