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

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

[Jakub Kicinski]

Vladimir said when adding this test:

  The bridge driver fares particularly badly [...] mainly because
  it does not implement IFF_UNICAST_FLT.

See commit 90b9566aa5cd ("selftests: forwarding: add a test for
local_termination.sh").

We don't want to hide the known gaps, but having a test which
always fails prevents us from catching regressions. Report
the cases we know may fail as XFAIL.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: vladimir.oltean@nxp.com
CC: shuah@kernel.org
CC: petrm@nvidia.com
CC: liuhangbin@gmail.com
CC: bpoirier@nvidia.com
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/forwarding/lib.sh |  9 ++++++++
 .../net/forwarding/local_termination.sh       | 21 ++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 3353a1745946..4fe28ab5d8b9 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -605,6 +605,15 @@ log_test_xfail()
 	RET=$ksft_xfail retmsg= log_test "$@"
 }
 
+log_test_xfail()
+{
+	local test_name=$1
+	local opt_str=$2
+
+	printf "TEST: %-60s  [XFAIL]\n" "$test_name $opt_str"
+	return 0
+}
+
 log_info()
 {
 	local msg=$1
diff --git a/tools/testing/selftests/net/forwarding/local_termination.sh b/tools/testing/selftests/net/forwarding/local_termination.sh
index c5b0cbc85b3e..4bba9c78db3e 100755
--- a/tools/testing/selftests/net/forwarding/local_termination.sh
+++ b/tools/testing/selftests/net/forwarding/local_termination.sh
@@ -73,6 +73,10 @@ check_rcv()
 	local pattern=$3
 	local should_receive=$4
 	local should_fail=
+	local xfail_sw=$5
+
+	local kind=$(ip -j -d link show dev $if_name |
+			 jq -r '.[].linkinfo.info_kind')
 
 	[ $should_receive = true ] && should_fail=0 || should_fail=1
 	RET=0
@@ -81,7 +85,14 @@ check_rcv()
 
 	check_err_fail "$should_fail" "$?" "reception"
 
-	log_test "$if_name: $type"
+	# If not a SW interface, ignore the XFAIL allowance
+	[ "$kind" != veth ] && [ "$kind" != bridge ] && xfail_sw=
+
+	if [ $RET -ne 0 ] && [ "$xfail_sw" == true ]; then
+	    log_test_xfail "$if_name: $type"
+	else
+	    log_test "$if_name: $type"
+	fi
 }
 
 mc_route_prepare()
@@ -157,7 +168,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 +176,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 +184,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 +200,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)" \
-- 
2.45.0

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

[Hangbin Liu]

Hi Jakub,

On Thu, May 09, 2024 at 04:55:53PM -0700, Jakub Kicinski wrote:
> Vladimir said when adding this test:
> 
>   The bridge driver fares particularly badly [...] mainly because
>   it does not implement IFF_UNICAST_FLT.
> 
> See commit 90b9566aa5cd ("selftests: forwarding: add a test for
> local_termination.sh").
> 
> We don't want to hide the known gaps, but having a test which
> always fails prevents us from catching regressions. Report
> the cases we know may fail as XFAIL.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: vladimir.oltean@nxp.com
> CC: shuah@kernel.org
> CC: petrm@nvidia.com
> CC: liuhangbin@gmail.com
> CC: bpoirier@nvidia.com
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/net/forwarding/lib.sh |  9 ++++++++
>  .../net/forwarding/local_termination.sh       | 21 ++++++++++++++-----
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 3353a1745946..4fe28ab5d8b9 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -605,6 +605,15 @@ log_test_xfail()
>  	RET=$ksft_xfail retmsg= log_test "$@"
>  }
>  
> +log_test_xfail()
> +{
> +	local test_name=$1
> +	local opt_str=$2
> +
> +	printf "TEST: %-60s  [XFAIL]\n" "$test_name $opt_str"
> +	return 0
> +}
> +

I may missed something, I saw there is already a log_test_xfail() in lib.sh

log_test_skip()
{
        RET=$ksft_skip retmsg= log_test "$@"
}

log_test_xfail()
{
        RET=$ksft_xfail retmsg= log_test "$@"
}

log_info()
{
...
}

Added by a923af1ceee7 ("selftests: forwarding: Convert log_test() to recognize RET values")

Thanks
Hangbin

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

[Jakub Kicinski]

On Fri, 10 May 2024 11:15:58 +0800 Hangbin Liu wrote:
> I may missed something, I saw there is already a log_test_xfail() in lib.sh
> 
> log_test_skip()
> {
>         RET=$ksft_skip retmsg= log_test "$@"
> }
> 
> log_test_xfail()
> {
>         RET=$ksft_xfail retmsg= log_test "$@"
> }
> 
> log_info()
> {
> ...
> }
> 
> Added by a923af1ceee7 ("selftests: forwarding: Convert log_test() to recognize RET values")

Good catch, sorry. Must have had net checked out when I wrote it :(

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

[Vladimir Oltean]

On Thu, May 09, 2024 at 04:55:53PM -0700, Jakub Kicinski wrote:
> diff --git a/tools/testing/selftests/net/forwarding/local_termination.sh b/tools/testing/selftests/net/forwarding/local_termination.sh
> index c5b0cbc85b3e..4bba9c78db3e 100755
> --- a/tools/testing/selftests/net/forwarding/local_termination.sh
> +++ b/tools/testing/selftests/net/forwarding/local_termination.sh
> @@ -73,6 +73,10 @@ check_rcv()
>  	local pattern=$3
>  	local should_receive=$4
>  	local should_fail=
> +	local xfail_sw=$5
> +
> +	local kind=$(ip -j -d link show dev $if_name |
> +			 jq -r '.[].linkinfo.info_kind')
>  
>  	[ $should_receive = true ] && should_fail=0 || should_fail=1
>  	RET=0
> @@ -81,7 +85,14 @@ check_rcv()
>  
>  	check_err_fail "$should_fail" "$?" "reception"
>  
> -	log_test "$if_name: $type"
> +	# If not a SW interface, ignore the XFAIL allowance
> +	[ "$kind" != veth ] && [ "$kind" != bridge ] && xfail_sw=
> +
> +	if [ $RET -ne 0 ] && [ "$xfail_sw" == true ]; then
> +	    log_test_xfail "$if_name: $type"
> +	else
> +	    log_test "$if_name: $type"
> +	fi
>  }

I just wish IFF_UNICAST_FLT was available to user space.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> @@ -157,7 +168,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)" \

For veth specifically there is xfail_on_veth:

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

Which is IMHO clearer than passing an extra boolean.

Not sure what to do about the bridge bit though. In principle the
various xfail_on_'s can be chained, so e.g. this should work:

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

I find this preferable to adding these ad-hoc tweaks to each test
individually. Maybe it would make sense to have:

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

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.

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

[Jakub Kicinski]

On Mon, 13 May 2024 15:25:38 +0200 Petr Machata wrote:
> For veth specifically there is xfail_on_veth:
> 
> xfail_on_veth $rcv_if_name \
> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> 		  false
> 
> Which is IMHO clearer than passing an extra boolean.

The extra boolean is because we want to only fail particular subcases.
I think that's legit. If the other cases regress we want to know.
Otherwise running the test on SW bridge is only useful for catching
crashes (so less useful).

So we probably need to reset FAIL_TO_XFAIL in this case.
Any preference on how to go about that? I'm tempted to:

diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh index
112c85c35092..79aa3c8bc288 100644 ---
a/tools/testing/selftests/net/forwarding/lib.sh +++
b/tools/testing/selftests/net/forwarding/lib.sh @@ -473,6 +473,13 @@
ret_set_ksft_status() # Whether FAILs should be interpreted as XFAILs.
Internal. FAIL_TO_XFAIL=
 
+# 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
65694cdf2dbb..0781ceba1348 100755 ---
a/tools/testing/selftests/net/forwarding/local_termination.sh +++
b/tools/testing/selftests/net/forwarding/local_termination.sh @@ -76,7
+76,7 @@ check_rcv() local xfail_sw=$5 
 	[ $should_receive = true ] && should_fail=0 || should_fail=1
-	RET=0
+	begin_test
 
 	tcpdump_show $if_name | grep -q "$pattern"
 

> Not sure what to do about the bridge bit though. In principle the
> various xfail_on_'s can be chained, so e.g. this should work:
> 
> xfail_on_bridge $rcv_if_name \
> xfail_on_veth $rcv_if_name \
> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> 		  false
> 
> I find this preferable to adding these ad-hoc tweaks to each test
> individually. Maybe it would make sense to have:
> 
> xfail_on_kind $rcv_if_name veth bridge \
> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> 		  false
> 
> 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.

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

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 13 May 2024 15:25:38 +0200 Petr Machata wrote:
>> For veth specifically there is xfail_on_veth:
>> 
>> xfail_on_veth $rcv_if_name \
>> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>> 		  false
>> 
>> Which is IMHO clearer than passing an extra boolean.
>
> The extra boolean is because we want to only fail particular subcases.
> I think that's legit. If the other cases regress we want to know.
> Otherwise running the test on SW bridge is only useful for catching
> crashes (so less useful).

Likewise you only annotate with xfail_on_* the testcases that you want
to xfail. The FAIL_TO_XFAIL ought to only be set for that one subcase
and then revert to its former value. (That's the intention anyway.)

> So we probably need to reset FAIL_TO_XFAIL in this case.
> Any preference on how to go about that? I'm tempted to:
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh
> b/tools/testing/selftests/net/forwarding/lib.sh index
> 112c85c35092..79aa3c8bc288 100644 ---
> a/tools/testing/selftests/net/forwarding/lib.sh +++
> b/tools/testing/selftests/net/forwarding/lib.sh @@ -473,6 +473,13 @@
> ret_set_ksft_status() # Whether FAILs should be interpreted as XFAILs.
> Internal. FAIL_TO_XFAIL=
>  
> +# 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
> 65694cdf2dbb..0781ceba1348 100755 ---
> a/tools/testing/selftests/net/forwarding/local_termination.sh +++
> b/tools/testing/selftests/net/forwarding/local_termination.sh @@ -76,7
> +76,7 @@ check_rcv() local xfail_sw=$5 
>  	[ $should_receive = true ] && should_fail=0 || should_fail=1
> -	RET=0
> +	begin_test
>  
>  	tcpdump_show $if_name | grep -q "$pattern"
>  
>
>> Not sure what to do about the bridge bit though. In principle the
>> various xfail_on_'s can be chained, so e.g. this should work:
>> 
>> xfail_on_bridge $rcv_if_name \
>> xfail_on_veth $rcv_if_name \
>> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>> 		  false
>> 
>> I find this preferable to adding these ad-hoc tweaks to each test
>> individually. Maybe it would make sense to have:
>> 
>> xfail_on_kind $rcv_if_name veth bridge \
>> 	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>> 		  "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>> 		  false
>> 
>> 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.

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

[Jakub Kicinski]

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 :)

--->8------

From 7a645eb425530f647e88590ba7ba01681e73812e Mon Sep 17 00:00:00 2001
From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 9 May 2024 16:28:41 -0700
Subject: selftests: net: local_termination: annotate the expected failures

Vladimir said when adding this test:

  The bridge driver fares particularly badly [...] mainly because
  it does not implement IFF_UNICAST_FLT.

See commit 90b9566aa5cd ("selftests: forwarding: add a test for
local_termination.sh").

We don't want to hide the known gaps, but having a test which
always fails prevents us from catching regressions. Report
the cases we know may fail as XFAIL.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: liuhangbin@gmail.com
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org

v2:
 - remove duplicated log_test_xfail
v1: https://lore.kernel.org/all/20240509235553.5740-1-kuba@kernel.org/
---
 tools/testing/selftests/net/forwarding/lib.sh       |  7 +++++++
 .../selftests/net/forwarding/local_termination.sh   | 13 ++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 112c85c35092..79aa3c8bc288 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -473,6 +473,13 @@ ret_set_ksft_status()
 # Whether FAILs should be interpreted as XFAILs. Internal.
 FAIL_TO_XFAIL=
 
+# 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
 
 	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)" \
-- 
2.45.0

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

[Petr Machata]

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)" \