* [PATCH net v4] selftests: net: add test for destination in broadcast packets
@ 2025-08-28 11:42 Oscar Maes
2025-08-28 12:16 ` Brett A C Sheffield
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Oscar Maes @ 2025-08-28 11:42 UTC (permalink / raw)
To: netdev, bacs, brett, kuba, pabeni; +Cc: davem, dsahern, stable, Oscar Maes
Add test to check the broadcast ethernet destination field is set
correctly.
This test sends a broadcast ping, captures it using tcpdump and
ensures that all bits of the 6 octet ethernet destination address
are correctly set by examining the output capture file.
Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
---
v3 -> v4:
- Added Brett as co-author
- Wait for tcpdump to bind using slowwait
Links:
- Discussion: https://lore.kernel.org/netdev/20250822165231.4353-4-bacs@librecast.net/
- Previous version: https://lore.kernel.org/netdev/20250827062322.4807-2-oscmaes92@gmail.com/
Thanks to Brett Sheffield for writing the initial version of this
selftest!
tools/testing/selftests/net/Makefile | 1 +
.../selftests/net/broadcast_ether_dst.sh | 83 +++++++++++++++++++
2 files changed, 84 insertions(+)
create mode 100755 tools/testing/selftests/net/broadcast_ether_dst.sh
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index b31a71f2b372..56ad10ea6628 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -115,6 +115,7 @@ TEST_PROGS += skf_net_off.sh
TEST_GEN_FILES += skf_net_off
TEST_GEN_FILES += tfo
TEST_PROGS += tfo_passive.sh
+TEST_PROGS += broadcast_ether_dst.sh
TEST_PROGS += broadcast_pmtu.sh
TEST_PROGS += ipv6_force_forwarding.sh
diff --git a/tools/testing/selftests/net/broadcast_ether_dst.sh b/tools/testing/selftests/net/broadcast_ether_dst.sh
new file mode 100755
index 000000000000..334a7eca8a80
--- /dev/null
+++ b/tools/testing/selftests/net/broadcast_ether_dst.sh
@@ -0,0 +1,83 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Author: Brett A C Sheffield <bacs@librecast.net>
+# Author: Oscar Maes <oscmaes92@gmail.com>
+#
+# Ensure destination ethernet field is correctly set for
+# broadcast packets
+
+source lib.sh
+
+CLIENT_IP4="192.168.0.1"
+GW_IP4="192.168.0.2"
+
+setup() {
+ setup_ns CLIENT_NS SERVER_NS
+
+ ip -net "${SERVER_NS}" link add link1 type veth \
+ peer name link0 netns "${CLIENT_NS}"
+
+ ip -net "${CLIENT_NS}" link set link0 up
+ ip -net "${CLIENT_NS}" addr add "${CLIENT_IP4}"/24 dev link0
+
+ ip -net "${SERVER_NS}" link set link1 up
+
+ ip -net "${CLIENT_NS}" route add default via "${GW_IP4}"
+ ip netns exec "${CLIENT_NS}" arp -s "${GW_IP4}" 00:11:22:33:44:55
+}
+
+cleanup() {
+ rm -f "${CAPFILE}" "${OUTPUT}"
+ ip -net "${SERVER_NS}" link del link1
+ cleanup_ns "${CLIENT_NS}" "${SERVER_NS}"
+}
+
+test_broadcast_ether_dst() {
+ local rc=0
+ CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
+ OUTPUT=$(mktemp -u out.XXXXXXXXXX)
+
+ echo "Testing ethernet broadcast destination"
+
+ # start tcpdump listening for icmp
+ # tcpdump will exit after receiving a single packet
+ # timeout will kill tcpdump if it is still running after 2s
+ timeout 2s ip netns exec "${CLIENT_NS}" \
+ tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
+ pid=$!
+ slowwait 1 grep -qs "listening" "${OUTPUT}"
+
+ # send broadcast ping
+ ip netns exec "${CLIENT_NS}" \
+ ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
+
+ # wait for tcpdump for exit after receiving packet
+ wait "${pid}"
+
+ # compare ethernet destination field to ff:ff:ff:ff:ff:ff
+ ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
+ awk '{sub(/,/,"",$3); print $3}')
+ if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
+ echo "[ OK ]"
+ rc="${ksft_pass}"
+ else
+ echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
+ "got ${ether_dst}"
+ rc="${ksft_fail}"
+ fi
+
+ return "${rc}"
+}
+
+if [ ! -x "$(command -v tcpdump)" ]; then
+ echo "SKIP: Could not run test without tcpdump tool"
+ exit "${ksft_skip}"
+fi
+
+trap cleanup EXIT
+
+setup
+test_broadcast_ether_dst
+
+exit $?
--
2.39.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-28 11:42 [PATCH net v4] selftests: net: add test for destination in broadcast packets Oscar Maes
@ 2025-08-28 12:16 ` Brett A C Sheffield
2025-08-29 11:19 ` Simon Horman
2025-09-02 8:49 ` Paolo Abeni
2 siblings, 0 replies; 10+ messages in thread
From: Brett A C Sheffield @ 2025-08-28 12:16 UTC (permalink / raw)
To: Oscar Maes; +Cc: netdev, kuba, pabeni, davem, dsahern, stable
On 2025-08-28 13:42, Oscar Maes wrote:
> Add test to check the broadcast ethernet destination field is set
> correctly.
>
> This test sends a broadcast ping, captures it using tcpdump and
> ensures that all bits of the 6 octet ethernet destination address
> are correctly set by examining the output capture file.
>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
> ---
> v3 -> v4:
> - Added Brett as co-author
> - Wait for tcpdump to bind using slowwait
Thanks Oscar.
I've tested the v4 selftest on a kernel with the regression and one without and
it looks good.
6.17.0-rc3
Testing ethernet broadcast destination
[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff, got 00:11:22:33:44:55
6.17.0-rc3-00002-g329af5eb13d7 (with v3 patch applied)
Testing ethernet broadcast destination
[ OK ]
> Links:
> - Discussion: https://lore.kernel.org/netdev/20250822165231.4353-4-bacs@librecast.net/
> - Previous version: https://lore.kernel.org/netdev/20250827062322.4807-2-oscmaes92@gmail.com/
>
> Thanks to Brett Sheffield for writing the initial version of this
> selftest!
>
> tools/testing/selftests/net/Makefile | 1 +
> .../selftests/net/broadcast_ether_dst.sh | 83 +++++++++++++++++++
> 2 files changed, 84 insertions(+)
> create mode 100755 tools/testing/selftests/net/broadcast_ether_dst.sh
>
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index b31a71f2b372..56ad10ea6628 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -115,6 +115,7 @@ TEST_PROGS += skf_net_off.sh
> TEST_GEN_FILES += skf_net_off
> TEST_GEN_FILES += tfo
> TEST_PROGS += tfo_passive.sh
> +TEST_PROGS += broadcast_ether_dst.sh
> TEST_PROGS += broadcast_pmtu.sh
> TEST_PROGS += ipv6_force_forwarding.sh
>
> diff --git a/tools/testing/selftests/net/broadcast_ether_dst.sh b/tools/testing/selftests/net/broadcast_ether_dst.sh
> new file mode 100755
> index 000000000000..334a7eca8a80
> --- /dev/null
> +++ b/tools/testing/selftests/net/broadcast_ether_dst.sh
> @@ -0,0 +1,83 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Author: Brett A C Sheffield <bacs@librecast.net>
> +# Author: Oscar Maes <oscmaes92@gmail.com>
> +#
> +# Ensure destination ethernet field is correctly set for
> +# broadcast packets
> +
> +source lib.sh
> +
> +CLIENT_IP4="192.168.0.1"
> +GW_IP4="192.168.0.2"
> +
> +setup() {
> + setup_ns CLIENT_NS SERVER_NS
> +
> + ip -net "${SERVER_NS}" link add link1 type veth \
> + peer name link0 netns "${CLIENT_NS}"
> +
> + ip -net "${CLIENT_NS}" link set link0 up
> + ip -net "${CLIENT_NS}" addr add "${CLIENT_IP4}"/24 dev link0
> +
> + ip -net "${SERVER_NS}" link set link1 up
> +
> + ip -net "${CLIENT_NS}" route add default via "${GW_IP4}"
> + ip netns exec "${CLIENT_NS}" arp -s "${GW_IP4}" 00:11:22:33:44:55
> +}
> +
> +cleanup() {
> + rm -f "${CAPFILE}" "${OUTPUT}"
> + ip -net "${SERVER_NS}" link del link1
> + cleanup_ns "${CLIENT_NS}" "${SERVER_NS}"
> +}
> +
> +test_broadcast_ether_dst() {
> + local rc=0
> + CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> + OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> +
> + echo "Testing ethernet broadcast destination"
> +
> + # start tcpdump listening for icmp
> + # tcpdump will exit after receiving a single packet
> + # timeout will kill tcpdump if it is still running after 2s
> + timeout 2s ip netns exec "${CLIENT_NS}" \
> + tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> + pid=$!
> + slowwait 1 grep -qs "listening" "${OUTPUT}"
> +
> + # send broadcast ping
> + ip netns exec "${CLIENT_NS}" \
> + ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> +
> + # wait for tcpdump for exit after receiving packet
> + wait "${pid}"
> +
> + # compare ethernet destination field to ff:ff:ff:ff:ff:ff
> + ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> + awk '{sub(/,/,"",$3); print $3}')
> + if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> + echo "[ OK ]"
> + rc="${ksft_pass}"
> + else
> + echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> + "got ${ether_dst}"
> + rc="${ksft_fail}"
> + fi
> +
> + return "${rc}"
> +}
> +
> +if [ ! -x "$(command -v tcpdump)" ]; then
> + echo "SKIP: Could not run test without tcpdump tool"
> + exit "${ksft_skip}"
> +fi
> +
> +trap cleanup EXIT
> +
> +setup
> +test_broadcast_ether_dst
> +
> +exit $?
> --
> 2.39.5
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-28 11:42 [PATCH net v4] selftests: net: add test for destination in broadcast packets Oscar Maes
2025-08-28 12:16 ` Brett A C Sheffield
@ 2025-08-29 11:19 ` Simon Horman
2025-08-29 11:44 ` Brett Sheffield
2025-08-29 13:11 ` Brett Sheffield
2025-09-02 8:49 ` Paolo Abeni
2 siblings, 2 replies; 10+ messages in thread
From: Simon Horman @ 2025-08-29 11:19 UTC (permalink / raw)
To: Oscar Maes; +Cc: netdev, bacs, brett, kuba, pabeni, davem, dsahern, stable
On Thu, Aug 28, 2025 at 01:42:42PM +0200, Oscar Maes wrote:
> Add test to check the broadcast ethernet destination field is set
> correctly.
>
> This test sends a broadcast ping, captures it using tcpdump and
> ensures that all bits of the 6 octet ethernet destination address
> are correctly set by examining the output capture file.
>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
...
> +test_broadcast_ether_dst() {
> + local rc=0
> + CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> + OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> +
> + echo "Testing ethernet broadcast destination"
> +
> + # start tcpdump listening for icmp
> + # tcpdump will exit after receiving a single packet
> + # timeout will kill tcpdump if it is still running after 2s
> + timeout 2s ip netns exec "${CLIENT_NS}" \
> + tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> + pid=$!
> + slowwait 1 grep -qs "listening" "${OUTPUT}"
> +
> + # send broadcast ping
> + ip netns exec "${CLIENT_NS}" \
> + ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> +
> + # wait for tcpdump for exit after receiving packet
> + wait "${pid}"
Hi Oscar and Brett,
I am concerned that if something goes wrong this may block forever.
Also, I'm wondering if this test could make use of the tcpdump helpers
provided in tools/testing/selftests/net/forwarding/lib.sh
> +
> + # compare ethernet destination field to ff:ff:ff:ff:ff:ff
> + ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> + awk '{sub(/,/,"",$3); print $3}')
> + if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> + echo "[ OK ]"
> + rc="${ksft_pass}"
> + else
> + echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> + "got ${ether_dst}"
> + rc="${ksft_fail}"
> + fi
> +
> + return "${rc}"
> +}
...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-29 11:19 ` Simon Horman
@ 2025-08-29 11:44 ` Brett Sheffield
2025-08-29 13:11 ` Brett Sheffield
1 sibling, 0 replies; 10+ messages in thread
From: Brett Sheffield @ 2025-08-29 11:44 UTC (permalink / raw)
To: Simon Horman; +Cc: Oscar Maes, netdev, kuba, pabeni, davem, dsahern, stable
On 2025-08-29 12:19, Simon Horman wrote:
> On Thu, Aug 28, 2025 at 01:42:42PM +0200, Oscar Maes wrote:
> > Add test to check the broadcast ethernet destination field is set
> > correctly.
> >
> > This test sends a broadcast ping, captures it using tcpdump and
> > ensures that all bits of the 6 octet ethernet destination address
> > are correctly set by examining the output capture file.
> >
> > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> > Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
>
> ...
>
> > +test_broadcast_ether_dst() {
> > + local rc=0
> > + CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> > + OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> > +
> > + echo "Testing ethernet broadcast destination"
> > +
> > + # start tcpdump listening for icmp
> > + # tcpdump will exit after receiving a single packet
> > + # timeout will kill tcpdump if it is still running after 2s
> > + timeout 2s ip netns exec "${CLIENT_NS}" \
> > + tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> > + pid=$!
> > + slowwait 1 grep -qs "listening" "${OUTPUT}"
> > +
> > + # send broadcast ping
> > + ip netns exec "${CLIENT_NS}" \
> > + ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> > +
> > + # wait for tcpdump for exit after receiving packet
> > + wait "${pid}"
>
> Hi Oscar and Brett,
>
> I am concerned that if something goes wrong this may block forever.
> Also, I'm wondering if this test could make use of the tcpdump helpers
> provided in tools/testing/selftests/net/forwarding/lib.sh
tcpdump is started with `timeout 2s`, which will kill it if the 2s timeout is
exceeded. Is that not sufficient here?
> > +
> > + # compare ethernet destination field to ff:ff:ff:ff:ff:ff
> > + ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> > + awk '{sub(/,/,"",$3); print $3}')
> > + if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> > + echo "[ OK ]"
> > + rc="${ksft_pass}"
> > + else
> > + echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> > + "got ${ether_dst}"
> > + rc="${ksft_fail}"
> > + fi
> > +
> > + return "${rc}"
> > +}
>
> ...
bacs
--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-29 11:19 ` Simon Horman
2025-08-29 11:44 ` Brett Sheffield
@ 2025-08-29 13:11 ` Brett Sheffield
2025-09-01 12:25 ` Oscar Maes
1 sibling, 1 reply; 10+ messages in thread
From: Brett Sheffield @ 2025-08-29 13:11 UTC (permalink / raw)
To: Simon Horman; +Cc: Oscar Maes, netdev, kuba, pabeni, davem, dsahern, stable
On 2025-08-29 12:19, Simon Horman wrote:
> On Thu, Aug 28, 2025 at 01:42:42PM +0200, Oscar Maes wrote:
> > Add test to check the broadcast ethernet destination field is set
> > correctly.
> >
> > This test sends a broadcast ping, captures it using tcpdump and
> > ensures that all bits of the 6 octet ethernet destination address
> > are correctly set by examining the output capture file.
> >
> > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> > Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
>
> ...
>
> > +test_broadcast_ether_dst() {
> > + local rc=0
> > + CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> > + OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> > +
> > + echo "Testing ethernet broadcast destination"
> > +
> > + # start tcpdump listening for icmp
> > + # tcpdump will exit after receiving a single packet
> > + # timeout will kill tcpdump if it is still running after 2s
> > + timeout 2s ip netns exec "${CLIENT_NS}" \
> > + tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> > + pid=$!
> > + slowwait 1 grep -qs "listening" "${OUTPUT}"
> > +
> > + # send broadcast ping
> > + ip netns exec "${CLIENT_NS}" \
> > + ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> > +
> > + # wait for tcpdump for exit after receiving packet
> > + wait "${pid}"
>
> Hi Oscar and Brett,
>
> I am concerned that if something goes wrong this may block forever.
> Also, I'm wondering if this test could make use of the tcpdump helpers
> provided in tools/testing/selftests/net/forwarding/lib.sh
Thanks for the review Simon. Further to previous email after some more thought
and poking at lib.sh
We're starting tcpdump with -c1 so that it exits immediately when the packet is
received, and we catch this with the wait() so that, in the best case, we
continue immediately, and in the worse case the `timeout 2s` kills tcpdump and
we move on to cleanup. I *think* this is pretty safe.
Taking a look at the forwarding/lib.sh it looks like we could use
tcpdump_start() and pass in $TCPDUMP_EXTRA_FLAGS but I don't think this buys us
much here, as we'd still need to wait() or a sleep() or otherwise detect that
tcpdump is finished so we can continue. I don't see anything in lib.sh to aid us
with that?
That said, it might be good to use the helper function anyway and keep the
wait() for consistency. There don't seem to be many tests using the tcpdump
helper functions yet, but it's probably the right way to move.
What do you think, Oscar? It looks like lib.sh tcpdump_start() takes all the
arguments, including for your namespaces. Up to you if you want to call that
instead.
Now I know it's there, I'll try to use that for future tests.
I don't *think* there's anything here that needs a v4, unless the timeout() call
is thought to be insufficient to kill tcpdump. There's a -k switch if we want
to SIGKILL it :-)
> > +
> > + # compare ethernet destination field to ff:ff:ff:ff:ff:ff
> > + ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> > + awk '{sub(/,/,"",$3); print $3}')
> > + if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> > + echo "[ OK ]"
> > + rc="${ksft_pass}"
> > + else
> > + echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> > + "got ${ether_dst}"
> > + rc="${ksft_fail}"
> > + fi
> > +
> > + return "${rc}"
> > +}
>
> ...
--
Brett Sheffield (he/him)
Librecast - Decentralising the Internet with Multicast
https://librecast.net/
https://blog.brettsheffield.com/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-29 13:11 ` Brett Sheffield
@ 2025-09-01 12:25 ` Oscar Maes
0 siblings, 0 replies; 10+ messages in thread
From: Oscar Maes @ 2025-09-01 12:25 UTC (permalink / raw)
To: Brett Sheffield, Simon Horman
Cc: netdev, kuba, pabeni, davem, dsahern, stable
On Fri, Aug 29, 2025 at 03:11:20PM +0200, Brett Sheffield wrote:
> On 2025-08-29 12:19, Simon Horman wrote:
> > On Thu, Aug 28, 2025 at 01:42:42PM +0200, Oscar Maes wrote:
> > > Add test to check the broadcast ethernet destination field is set
> > > correctly.
> > >
> > > This test sends a broadcast ping, captures it using tcpdump and
> > > ensures that all bits of the 6 octet ethernet destination address
> > > are correctly set by examining the output capture file.
> > >
> > > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> > > Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
> >
> > ...
> >
> > > +test_broadcast_ether_dst() {
> > > + local rc=0
> > > + CAPFILE=$(mktemp -u cap.XXXXXXXXXX)
> > > + OUTPUT=$(mktemp -u out.XXXXXXXXXX)
> > > +
> > > + echo "Testing ethernet broadcast destination"
> > > +
> > > + # start tcpdump listening for icmp
> > > + # tcpdump will exit after receiving a single packet
> > > + # timeout will kill tcpdump if it is still running after 2s
> > > + timeout 2s ip netns exec "${CLIENT_NS}" \
> > > + tcpdump -i link0 -c 1 -w "${CAPFILE}" icmp &> "${OUTPUT}" &
> > > + pid=$!
> > > + slowwait 1 grep -qs "listening" "${OUTPUT}"
> > > +
> > > + # send broadcast ping
> > > + ip netns exec "${CLIENT_NS}" \
> > > + ping -W0.01 -c1 -b 255.255.255.255 &> /dev/null
> > > +
> > > + # wait for tcpdump for exit after receiving packet
> > > + wait "${pid}"
> >
> > Hi Oscar and Brett,
> >
> > I am concerned that if something goes wrong this may block forever.
> > Also, I'm wondering if this test could make use of the tcpdump helpers
> > provided in tools/testing/selftests/net/forwarding/lib.sh
>
> Thanks for the review Simon. Further to previous email after some more thought
> and poking at lib.sh
>
> We're starting tcpdump with -c1 so that it exits immediately when the packet is
> received, and we catch this with the wait() so that, in the best case, we
> continue immediately, and in the worse case the `timeout 2s` kills tcpdump and
> we move on to cleanup. I *think* this is pretty safe.
>
> Taking a look at the forwarding/lib.sh it looks like we could use
> tcpdump_start() and pass in $TCPDUMP_EXTRA_FLAGS but I don't think this buys us
> much here, as we'd still need to wait() or a sleep() or otherwise detect that
> tcpdump is finished so we can continue. I don't see anything in lib.sh to aid us
> with that?
>
> That said, it might be good to use the helper function anyway and keep the
> wait() for consistency. There don't seem to be many tests using the tcpdump
> helper functions yet, but it's probably the right way to move.
>
> What do you think, Oscar? It looks like lib.sh tcpdump_start() takes all the
> arguments, including for your namespaces. Up to you if you want to call that
> instead.
>
> Now I know it's there, I'll try to use that for future tests.
>
> I don't *think* there's anything here that needs a v4, unless the timeout() call
> is thought to be insufficient to kill tcpdump. There's a -k switch if we want
> to SIGKILL it :-)
>
I agree with Brett here.
I tried using forwarding/lib.sh but it made the test unnecessarily complex
and difficult to read/debug. I suggest we keep it as-is.
Simon - what do you think?
> > > +
> > > + # compare ethernet destination field to ff:ff:ff:ff:ff:ff
> > > + ether_dst=$(tcpdump -r "${CAPFILE}" -tnne 2>/dev/null | \
> > > + awk '{sub(/,/,"",$3); print $3}')
> > > + if [[ "${ether_dst}" == "ff:ff:ff:ff:ff:ff" ]]; then
> > > + echo "[ OK ]"
> > > + rc="${ksft_pass}"
> > > + else
> > > + echo "[FAIL] expected dst ether addr to be ff:ff:ff:ff:ff:ff," \
> > > + "got ${ether_dst}"
> > > + rc="${ksft_fail}"
> > > + fi
> > > +
> > > + return "${rc}"
> > > +}
> >
> > ...
>
> --
> Brett Sheffield (he/him)
> Librecast - Decentralising the Internet with Multicast
> https://librecast.net/
> https://blog.brettsheffield.com/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-08-28 11:42 [PATCH net v4] selftests: net: add test for destination in broadcast packets Oscar Maes
2025-08-28 12:16 ` Brett A C Sheffield
2025-08-29 11:19 ` Simon Horman
@ 2025-09-02 8:49 ` Paolo Abeni
2025-09-02 9:33 ` Brett Sheffield
2 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2025-09-02 8:49 UTC (permalink / raw)
To: Oscar Maes, netdev, bacs, brett, kuba; +Cc: davem, dsahern, stable
On 8/28/25 1:42 PM, Oscar Maes wrote:
> Add test to check the broadcast ethernet destination field is set
> correctly.
>
> This test sends a broadcast ping, captures it using tcpdump and
> ensures that all bits of the 6 octet ethernet destination address
> are correctly set by examining the output capture file.
>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
I'm sorry for nit-picking, but the sob/tag-chain is wrong, please have a
look at:
https://elixir.bootlin.com/linux/v6.16.4/source/Documentation/process/submitting-patches.rst#L516
Thanks,
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-09-02 8:49 ` Paolo Abeni
@ 2025-09-02 9:33 ` Brett Sheffield
2025-09-02 9:57 ` Paolo Abeni
0 siblings, 1 reply; 10+ messages in thread
From: Brett Sheffield @ 2025-09-02 9:33 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Oscar Maes, netdev, bacs, kuba, davem, dsahern, stable
On 2025-09-02 10:49, Paolo Abeni wrote:
> On 8/28/25 1:42 PM, Oscar Maes wrote:
> > Add test to check the broadcast ethernet destination field is set
> > correctly.
> >
> > This test sends a broadcast ping, captures it using tcpdump and
> > ensures that all bits of the 6 octet ethernet destination address
> > are correctly set by examining the output capture file.
> >
> > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> > Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
>
> I'm sorry for nit-picking, but the sob/tag-chain is wrong, please have a
> look at:
>
> https://elixir.bootlin.com/linux/v6.16.4/source/Documentation/process/submitting-patches.rst#L516
Thanks Paolo. So, something like:
Co-developed-by: Brett A C Sheffield <bacs@librecast.net>
Signed-off-by: Brett A C Sheffield <bacs@librecast.net>
Co-developed-by: Oscar Maes <oscmaes92@gmail.com>
Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
with the last sign-off by Oscar because he is submitting?
Brett
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-09-02 9:33 ` Brett Sheffield
@ 2025-09-02 9:57 ` Paolo Abeni
2025-09-02 10:25 ` Brett Sheffield
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2025-09-02 9:57 UTC (permalink / raw)
To: Brett Sheffield; +Cc: Oscar Maes, netdev, kuba, davem, dsahern, stable
On 9/2/25 11:33 AM, Brett Sheffield wrote:
> On 2025-09-02 10:49, Paolo Abeni wrote:
>> On 8/28/25 1:42 PM, Oscar Maes wrote:
>>> Add test to check the broadcast ethernet destination field is set
>>> correctly.
>>>
>>> This test sends a broadcast ping, captures it using tcpdump and
>>> ensures that all bits of the 6 octet ethernet destination address
>>> are correctly set by examining the output capture file.
>>>
>>> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
>>> Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
>>
>> I'm sorry for nit-picking, but the sob/tag-chain is wrong, please have a
>> look at:
>>
>> https://elixir.bootlin.com/linux/v6.16.4/source/Documentation/process/submitting-patches.rst#L516
>
> Thanks Paolo. So, something like:
>
> Co-developed-by: Brett A C Sheffield <bacs@librecast.net>
> Signed-off-by: Brett A C Sheffield <bacs@librecast.net>
> Co-developed-by: Oscar Maes <oscmaes92@gmail.com>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
>
> with the last sign-off by Oscar because he is submitting?
Actually my understanding is:
Co-developed-by: Brett A C Sheffield <bacs@librecast.net>
Signed-off-by: Brett A C Sheffield <bacs@librecast.net>
Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
(if the patch is submitted by Oscar.) Basically the first examples in
the doc, with the only differences that such examples lists 3 co-developers.
/P
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v4] selftests: net: add test for destination in broadcast packets
2025-09-02 9:57 ` Paolo Abeni
@ 2025-09-02 10:25 ` Brett Sheffield
0 siblings, 0 replies; 10+ messages in thread
From: Brett Sheffield @ 2025-09-02 10:25 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Oscar Maes, netdev, kuba, davem, dsahern, stable
On 2025-09-02 11:57, Paolo Abeni wrote:
>
>
> On 9/2/25 11:33 AM, Brett Sheffield wrote:
> > On 2025-09-02 10:49, Paolo Abeni wrote:
> >> On 8/28/25 1:42 PM, Oscar Maes wrote:
> >>> Add test to check the broadcast ethernet destination field is set
> >>> correctly.
> >>>
> >>> This test sends a broadcast ping, captures it using tcpdump and
> >>> ensures that all bits of the 6 octet ethernet destination address
> >>> are correctly set by examining the output capture file.
> >>>
> >>> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> >>> Co-authored-by: Brett A C Sheffield <bacs@librecast.net>
> >>
> >> I'm sorry for nit-picking, but the sob/tag-chain is wrong, please have a
> >> look at:
> >>
> >> https://elixir.bootlin.com/linux/v6.16.4/source/Documentation/process/submitting-patches.rst#L516
> >
> > Thanks Paolo. So, something like:
> >
> > Co-developed-by: Brett A C Sheffield <bacs@librecast.net>
> > Signed-off-by: Brett A C Sheffield <bacs@librecast.net>
> > Co-developed-by: Oscar Maes <oscmaes92@gmail.com>
> > Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
> >
> > with the last sign-off by Oscar because he is submitting?
>
> Actually my understanding is:
>
> Co-developed-by: Brett A C Sheffield <bacs@librecast.net>
> Signed-off-by: Brett A C Sheffield <bacs@librecast.net>
> Signed-off-by: Oscar Maes <oscmaes92@gmail.com>
>
> (if the patch is submitted by Oscar.) Basically the first examples in
> the doc, with the only differences that such examples lists 3 co-developers.
Ah yes, you are correct. I missed the "in addition to the author attributed by
the From: tag" bit.
Thanks again.
Brett
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-09-02 10:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-28 11:42 [PATCH net v4] selftests: net: add test for destination in broadcast packets Oscar Maes
2025-08-28 12:16 ` Brett A C Sheffield
2025-08-29 11:19 ` Simon Horman
2025-08-29 11:44 ` Brett Sheffield
2025-08-29 13:11 ` Brett Sheffield
2025-09-01 12:25 ` Oscar Maes
2025-09-02 8:49 ` Paolo Abeni
2025-09-02 9:33 ` Brett Sheffield
2025-09-02 9:57 ` Paolo Abeni
2025-09-02 10:25 ` Brett Sheffield
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 NNTP newsgroup(s).