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;
Date:   Fri, 14 Aug 2020 10:05:20 -0400 (EDT)
From:   Michael Jeanson <mjeanson@efficios.com>
To:     David Ahern <dsahern@gmail.com>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        David Ahern <dsahern@kernel.org>, linux-kernel@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>, netdev@vger.kernel.org
Message-ID: <217122726.8258.1597413920624.JavaMail.zimbra@efficios.com>
In-Reply-To: <49f26c7d-b8aa-57e7-02c6-424bec9c3845@gmail.com>
References: <20200811195003.1812-1-mathieu.desnoyers@efficios.com> <20200811195003.1812-2-mathieu.desnoyers@efficios.com> <49f26c7d-b8aa-57e7-02c6-424bec9c3845@gmail.com>
Subject: Re: [PATCH 1/3] selftests: Add VRF icmp error route lookup test
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: selftests: Add VRF icmp error route lookup test
Thread-Index: L7PbwFLrYb4o0H7ABrtySfFz4apuVQ==
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

----- On 13 Aug, 2020, at 19:13, David Ahern dsahern@gmail.com wrote:

...

>> +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.

Hum, I have no issues here with '-v', you get no output at all from run_cmd?

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

I took this pattern of executing commands twice when running with verbose and
grepping the output from icmp_redirect.sh. I'll see if I can come up with
something fancier in run_cmd.


>> +	log_test $? 0 "Ping received ICMP ttl exceeded"
>> +}
>> +################################################################################
> 
> missing newline between '}' and '####'

Ack

...

>> +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
> 

Ack

>> +	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

I'l add the missing '-t' option to getopts.

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