From: Petr Machata <petrm@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Petr Machata <petrm@nvidia.com>, <davem@davemloft.net>,
<netdev@vger.kernel.org>, <edumazet@google.com>,
<pabeni@redhat.com>, <vladimir.oltean@nxp.com>,
<shuah@kernel.org>, <liuhangbin@gmail.com>, <bpoirier@nvidia.com>,
<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures
Date: Thu, 16 May 2024 10:42:31 +0200 [thread overview]
Message-ID: <87ikzevyl7.fsf@nvidia.com> (raw)
In-Reply-To: <20240515162132.476a6b43@kernel.org>
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)" \
prev parent reply other threads:[~2024-05-16 10:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 23:55 [PATCH net-next] selftests: net: local_termination: annotate the expected failures Jakub Kicinski
2024-05-10 3:15 ` Hangbin Liu
2024-05-10 3:47 ` Jakub Kicinski
2024-05-10 10:58 ` Vladimir Oltean
2024-05-13 13:25 ` Petr Machata
2024-05-15 0:43 ` Jakub Kicinski
2024-05-15 9:02 ` Petr Machata
2024-05-15 23:21 ` Jakub Kicinski
2024-05-16 8:42 ` Petr Machata [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ikzevyl7.fsf@nvidia.com \
--to=petrm@nvidia.com \
--cc=bpoirier@nvidia.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=liuhangbin@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=shuah@kernel.org \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).