diff mbox series

[net,1/2] selftests: net: lib: fix broken ping with coreutils ping util

Message ID 20241130113314.6488-1-ansuelsmth@gmail.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [net,1/2] selftests: net: lib: fix broken ping with coreutils ping util | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success net selftest script(s) already in Makefile
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 304 this patch: 304
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest warning net-next-2024-11-30--12-00 (tests: 759)

Commit Message

Christian Marangi Nov. 30, 2024, 11:33 a.m. UTC
If the coreutils variant of ping is used instead of the busybox one, the
ping_do() command is broken. This comes by the fact that for coreutils
ping, the ping IP needs to be the very last elements.

To handle this, reorder the ping args and make $dip last element.

The use of coreutils ping might be useful for case where busybox is not
compiled with float interval support and ping command doesn't support
0.1 interval. (in such case a dedicated ping utility is installed
instead)

Cc: stable@vger.kernel.org
Fixes: 73bae6736b6b ("selftests: forwarding: Add initial testing framework")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Vladimir Oltean Nov. 30, 2024, 3:43 p.m. UTC | #1
On Sat, Nov 30, 2024 at 12:33:09PM +0100, Christian Marangi wrote:
> If the coreutils variant of ping is used instead of the busybox one, the
> ping_do() command is broken. This comes by the fact that for coreutils
> ping, the ping IP needs to be the very last elements.
> 
> To handle this, reorder the ping args and make $dip last element.
> 
> The use of coreutils ping might be useful for case where busybox is not
> compiled with float interval support and ping command doesn't support
> 0.1 interval. (in such case a dedicated ping utility is installed
> instead)
> 
> Cc: stable@vger.kernel.org
> Fixes: 73bae6736b6b ("selftests: forwarding: Add initial testing framework")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---

Do you mean the other way around? that the busybox ping is the broken one?
And by coreutils ping, you actually mean iputils ping, right?
Christian Marangi Nov. 30, 2024, 3:46 p.m. UTC | #2
On Sat, Nov 30, 2024 at 05:43:07PM +0200, Vladimir Oltean wrote:
> On Sat, Nov 30, 2024 at 12:33:09PM +0100, Christian Marangi wrote:
> > If the coreutils variant of ping is used instead of the busybox one, the
> > ping_do() command is broken. This comes by the fact that for coreutils
> > ping, the ping IP needs to be the very last elements.
> > 
> > To handle this, reorder the ping args and make $dip last element.
> > 
> > The use of coreutils ping might be useful for case where busybox is not
> > compiled with float interval support and ping command doesn't support
> > 0.1 interval. (in such case a dedicated ping utility is installed
> > instead)
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 73bae6736b6b ("selftests: forwarding: Add initial testing framework")
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> 
> Do you mean the other way around? that the busybox ping is the broken one?
> And by coreutils ping, you actually mean iputils ping, right?

Mhh no busybox ping utility is problematic only if FLOAT INTERVAL is not
enabled (aka 0.1 interval are not supported)

Yes I'm referring to iputils ping. With that I notice args are wrongly
parsed... especially with the -c option.
Vladimir Oltean Nov. 30, 2024, 3:48 p.m. UTC | #3
On Sat, Nov 30, 2024 at 04:46:14PM +0100, Christian Marangi wrote:
> On Sat, Nov 30, 2024 at 05:43:07PM +0200, Vladimir Oltean wrote:
> > On Sat, Nov 30, 2024 at 12:33:09PM +0100, Christian Marangi wrote:
> > > If the coreutils variant of ping is used instead of the busybox one, the
> > > ping_do() command is broken. This comes by the fact that for coreutils
> > > ping, the ping IP needs to be the very last elements.
> > > 
> > > To handle this, reorder the ping args and make $dip last element.
> > > 
> > > The use of coreutils ping might be useful for case where busybox is not
> > > compiled with float interval support and ping command doesn't support
> > > 0.1 interval. (in such case a dedicated ping utility is installed
> > > instead)
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 73bae6736b6b ("selftests: forwarding: Add initial testing framework")
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > 
> > Do you mean the other way around? that the busybox ping is the broken one?
> > And by coreutils ping, you actually mean iputils ping, right?
> 
> Mhh no busybox ping utility is problematic only if FLOAT INTERVAL is not
> enabled (aka 0.1 interval are not supported)
> 
> Yes I'm referring to iputils ping. With that I notice args are wrongly
> parsed... especially with the -c option.

But isn't iputils ping what everybody else uses? I'm confused. I have
this version and the current syntax is not problematic for me.

$ ping -V
ping from iputils 20240905
libcap: yes, IDN: yes, NLS: no, error.h: yes, getrandom(): yes, __fpending(): yes
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index c992e385159c..2060f95d5c62 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1473,8 +1473,8 @@  ping_do()
 
 	vrf_name=$(master_name_get $if_name)
 	ip vrf exec $vrf_name \
-		$PING $args $dip -c $PING_COUNT -i 0.1 \
-		-w $PING_TIMEOUT &> /dev/null
+		$PING $args -c $PING_COUNT -i 0.1 \
+		-w $PING_TIMEOUT $dip &> /dev/null
 }
 
 ping_test()
@@ -1504,8 +1504,8 @@  ping6_do()
 
 	vrf_name=$(master_name_get $if_name)
 	ip vrf exec $vrf_name \
-		$PING6 $args $dip -c $PING_COUNT -i 0.1 \
-		-w $PING_TIMEOUT &> /dev/null
+		$PING6 $args -c $PING_COUNT -i 0.1 \
+		-w $PING_TIMEOUT $dip &> /dev/null
 }
 
 ping6_test()