Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures

[Petr Machata <petrm@nvidia.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 15 May 2024 11:02:28 +0200 Petr Machata wrote:
>> >> And then either replace the existing xfail_on_veth's (there are just a
>> >> handful) or convert xfail_on_veth to a wrapper around xfail_on_kind.  
>> >
>> > I think the bridge thing we can workaround by just checking
>> > if ${NETIFS[p1]} is veth, rather than $rcv_if_name.
>> > Since the two behave the same.  
>> 
>> I don't follow. The test has two legs, one creates a VRF and attaches
>> p2, the other creates a bridge and attaches p2. Whether p1 and p2 are
>> veth or HW seems orthogonal to whether $rcv_if_name is a bridge or a
>> veth.
>
> Right, my superficial understanding was that the main distinction is
> whether p2/h2 can do the filtering (or possibly some offload happens).
> So if p1,p2 are veths we know to XFAIL, doesn't matter if we're in 
> the vrf or bridge configuration, cause these construct will not filter
> either.
>
> If I'm not making sense - I'm probably confused, I can code up what you
> suggested, it will work, just more LoC :)

I'm not sure myself, but from the commit message it looks like the issue
is with $rcv_if_name being the bridge.

But the patch that you inline is R-b'd and T-b'd by Vladimir, so I'm
going to assume it's doing the right thing.

> +# Clear internal failure tracking for the next test case
> +begin_test()
> +{
> +    RET=0
> +    FAIL_TO_XFAIL=
> +}
> +
>  check_err()
>  {
>  	local err=$1
> diff --git a/tools/testing/selftests/net/forwarding/local_termination.sh b/tools/testing/selftests/net/forwarding/local_termination.sh
> index c5b0cbc85b3e..a241acc02498 100755
> --- a/tools/testing/selftests/net/forwarding/local_termination.sh
> +++ b/tools/testing/selftests/net/forwarding/local_termination.sh
> @@ -73,9 +73,12 @@ check_rcv()
>  	local pattern=$3
>  	local should_receive=$4
>  	local should_fail=
> +	local xfail_sw=$5
>  
>  	[ $should_receive = true ] && should_fail=0 || should_fail=1
> -	RET=0
> +	begin_test
> +	# check if main interface is veth
> +	[ "$xfail_sw" == true ] && xfail_on_veth $h1

If xfail_on_veth $h1 is all that's needed, then I really don't see a
reason why not just do this:

	check_rcv $rcv_if_name "Unicast IPv4 to primary MAC address" \
		"$smac > $rcv_dmac, ethertype IPv4 (0x0800)" \
		true

	check_rcv $rcv_if_name "Unicast IPv4 to macvlan MAC address" \
		"$smac > $MACVLAN_ADDR, ethertype IPv4 (0x0800)" \
		true

	xfail_on_veth $h1 \
		check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
			"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
			false

This should work now, in much the same way as this patch, but the intent
is IMHO clearer (vs. passing a mystery true), and FAIL_TO_XFAIL is
cleanly scoped and doesn't run the risk of leaking out of the test.

>  	tcpdump_show $if_name | grep -q "$pattern"
>  
> @@ -157,7 +160,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>  		"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, promisc" \
>  		"$smac > $UNKNOWN_UC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -165,7 +168,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, allmulti" \
>  		"$smac > $UNKNOWN_UC_ADDR3, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to joined group" \
>  		"$smac > $JOINED_MACV4_MC_ADDR, ethertype IPv4 (0x0800)" \
> @@ -173,7 +176,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to unknown group" \
>  		"$smac > $UNKNOWN_MACV4_MC_ADDR1, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to unknown group, promisc" \
>  		"$smac > $UNKNOWN_MACV4_MC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -189,7 +192,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Multicast IPv6 to unknown group" \
>  		"$smac > $UNKNOWN_MACV6_MC_ADDR1, ethertype IPv6 (0x86dd)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv6 to unknown group, promisc" \
>  		"$smac > $UNKNOWN_MACV6_MC_ADDR2, ethertype IPv6 (0x86dd)" \