| Message ID | 20231229131931.3961150-1-yujie.liu@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 05d92cb0e919239c29b3a26da1f76f1e18fed7d3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] selftests/net: change shebang to bash to support "source" | expand |
On 12/29/23 14:19, Yujie Liu wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > Refer to other scripts that were converted together, simply change the > shebang to bash to fix the following issues when the default /bin/sh > points to other shells. > (snip) > > Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] > Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Yujie Liu <yujie.liu@intel.com> I would recommend use of shellcheck in the future, it will catch this particular bug, with following warning: SC3046: In POSIX sh, 'source' in place of '.' is undefined. Being specific, and requiring bash looks fine for me. Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > Refer to other scripts that were converted together, simply change the > shebang to bash to fix the following issues when the default /bin/sh > points to other shells. > > # selftests: net: unicast_extensions.sh > # ./unicast_extensions.sh: 31: source: not found > # ########################################################################### > # Unicast address extensions tests (behavior of reserved IPv4 addresses) > # ########################################################################### > # TEST: assign and ping within 240/4 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 240/4 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping inside 255.255/16 (is allowed) [FAIL] > # TEST: assign and ping inside 255.255.255/24 (is allowed) [FAIL] > # TEST: route between 240.5.6/24 and 255.1.2/24 (is allowed) [FAIL] > # TEST: route between 0.200/16 and 245.99/16 (is allowed) [FAIL] > # TEST: assign and ping lowest address (/24) [FAIL] > # TEST: assign and ping lowest address (/26) [FAIL] > # TEST: routing using lowest address [FAIL] > # TEST: assigning 0.0.0.0 (is forbidden) [ OK ] > # TEST: assigning 255.255.255.255 (is forbidden) [ OK ] > # TEST: assign and ping inside 127/8 (is forbidden) [ OK ] > # TEST: assign and ping class D address (is forbidden) [ OK ] > # TEST: routing using class D (is forbidden) [ OK ] > # TEST: routing using 127/8 (is forbidden) [ OK ] > not ok 51 selftests: net: unicast_extensions.sh # exit=1 > > v1 -> v2: > - Fix pmtu.sh which has the same issue as unicast_extensions.sh, > suggested by Hangbin > - Change the style of the "source" line to be consistent with other > tests, suggested by Hangbin > > Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] > Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Yujie Liu <yujie.liu@intel.com> > --- > tools/testing/selftests/net/pmtu.sh | 4 ++-- > tools/testing/selftests/net/unicast_extensions.sh | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh > index 175d3d1d773b..f10879788f61 100755 > --- a/tools/testing/selftests/net/pmtu.sh > +++ b/tools/testing/selftests/net/pmtu.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # Check that route PMTU values match expectations, and that initial device MTU > @@ -198,7 +198,7 @@ > # - pmtu_ipv6_route_change > # Same as above but with IPv6 > > -source ./lib.sh > +source lib.sh > > PAUSE_ON_FAIL=no > VERBOSE=0 > diff --git a/tools/testing/selftests/net/unicast_extensions.sh b/tools/testing/selftests/net/unicast_extensions.sh > index b7a2cb9e7477..f52aa5f7da52 100755 > --- a/tools/testing/selftests/net/unicast_extensions.sh > +++ b/tools/testing/selftests/net/unicast_extensions.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # By Seth Schoen (c) 2021, for the IPv4 Unicast Extensions Project > @@ -28,7 +28,7 @@ > # These tests provide an easy way to flip the expected result of any > # of these behaviors for testing kernel patches that change them. > > -source ./lib.sh > +source lib.sh > > # nettest can be run from PATH or from same directory as this selftest > if ! which nettest >/dev/null; then > > base-commit: cd4d7263d58ab98fd4dee876776e4da6c328faa3 > -- > 2.34.1 > Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
On 12/29/23 6:19 PM, Yujie Liu wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > Refer to other scripts that were converted together, simply change the > shebang to bash to fix the following issues when the default /bin/sh > points to other shells. > > # selftests: net: unicast_extensions.sh > # ./unicast_extensions.sh: 31: source: not found > # ########################################################################### > # Unicast address extensions tests (behavior of reserved IPv4 addresses) > # ########################################################################### > # TEST: assign and ping within 240/4 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 240/4 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping inside 255.255/16 (is allowed) [FAIL] > # TEST: assign and ping inside 255.255.255/24 (is allowed) [FAIL] > # TEST: route between 240.5.6/24 and 255.1.2/24 (is allowed) [FAIL] > # TEST: route between 0.200/16 and 245.99/16 (is allowed) [FAIL] > # TEST: assign and ping lowest address (/24) [FAIL] > # TEST: assign and ping lowest address (/26) [FAIL] > # TEST: routing using lowest address [FAIL] > # TEST: assigning 0.0.0.0 (is forbidden) [ OK ] > # TEST: assigning 255.255.255.255 (is forbidden) [ OK ] > # TEST: assign and ping inside 127/8 (is forbidden) [ OK ] > # TEST: assign and ping class D address (is forbidden) [ OK ] > # TEST: routing using class D (is forbidden) [ OK ] > # TEST: routing using 127/8 (is forbidden) [ OK ] > not ok 51 selftests: net: unicast_extensions.sh # exit=1 > > v1 -> v2: > - Fix pmtu.sh which has the same issue as unicast_extensions.sh, > suggested by Hangbin > - Change the style of the "source" line to be consistent with other > tests, suggested by Hangbin > > Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] > Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Yujie Liu <yujie.liu@intel.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > tools/testing/selftests/net/pmtu.sh | 4 ++-- > tools/testing/selftests/net/unicast_extensions.sh | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh > index 175d3d1d773b..f10879788f61 100755 > --- a/tools/testing/selftests/net/pmtu.sh > +++ b/tools/testing/selftests/net/pmtu.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # Check that route PMTU values match expectations, and that initial device MTU > @@ -198,7 +198,7 @@ > # - pmtu_ipv6_route_change > # Same as above but with IPv6 > > -source ./lib.sh > +source lib.sh > > PAUSE_ON_FAIL=no > VERBOSE=0 > diff --git a/tools/testing/selftests/net/unicast_extensions.sh b/tools/testing/selftests/net/unicast_extensions.sh > index b7a2cb9e7477..f52aa5f7da52 100755 > --- a/tools/testing/selftests/net/unicast_extensions.sh > +++ b/tools/testing/selftests/net/unicast_extensions.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # By Seth Schoen (c) 2021, for the IPv4 Unicast Extensions Project > @@ -28,7 +28,7 @@ > # These tests provide an easy way to flip the expected result of any > # of these behaviors for testing kernel patches that change them. > > -source ./lib.sh > +source lib.sh > > # nettest can be run from PATH or from same directory as this selftest > if ! which nettest >/dev/null; then > > base-commit: cd4d7263d58ab98fd4dee876776e4da6c328faa3
On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > Refer to other scripts that were converted together, simply change the > shebang to bash to fix the following issues when the default /bin/sh > points to other shells. Looks like it'd be simpler to just replace the "source" commands with "." and leave the shebang as is (unless there are other bash-specific constructs in these scripts of course). Generally speaking, I think we should avoid madating a specific shell, unless that really simplifies the test script (which is not the case here). > # selftests: net: unicast_extensions.sh > # ./unicast_extensions.sh: 31: source: not found > # ########################################################################### > # Unicast address extensions tests (behavior of reserved IPv4 addresses) > # ########################################################################### > # TEST: assign and ping within 240/4 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 240/4 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping inside 255.255/16 (is allowed) [FAIL] > # TEST: assign and ping inside 255.255.255/24 (is allowed) [FAIL] > # TEST: route between 240.5.6/24 and 255.1.2/24 (is allowed) [FAIL] > # TEST: route between 0.200/16 and 245.99/16 (is allowed) [FAIL] > # TEST: assign and ping lowest address (/24) [FAIL] > # TEST: assign and ping lowest address (/26) [FAIL] > # TEST: routing using lowest address [FAIL] > # TEST: assigning 0.0.0.0 (is forbidden) [ OK ] > # TEST: assigning 255.255.255.255 (is forbidden) [ OK ] > # TEST: assign and ping inside 127/8 (is forbidden) [ OK ] > # TEST: assign and ping class D address (is forbidden) [ OK ] > # TEST: routing using class D (is forbidden) [ OK ] > # TEST: routing using 127/8 (is forbidden) [ OK ] > not ok 51 selftests: net: unicast_extensions.sh # exit=1 > > v1 -> v2: > - Fix pmtu.sh which has the same issue as unicast_extensions.sh, > suggested by Hangbin > - Change the style of the "source" line to be consistent with other > tests, suggested by Hangbin > > Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] > Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Yujie Liu <yujie.liu@intel.com> > --- > tools/testing/selftests/net/pmtu.sh | 4 ++-- > tools/testing/selftests/net/unicast_extensions.sh | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh > index 175d3d1d773b..f10879788f61 100755 > --- a/tools/testing/selftests/net/pmtu.sh > +++ b/tools/testing/selftests/net/pmtu.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # Check that route PMTU values match expectations, and that initial device MTU > @@ -198,7 +198,7 @@ > # - pmtu_ipv6_route_change > # Same as above but with IPv6 > > -source ./lib.sh > +source lib.sh > > PAUSE_ON_FAIL=no > VERBOSE=0 > diff --git a/tools/testing/selftests/net/unicast_extensions.sh b/tools/testing/selftests/net/unicast_extensions.sh > index b7a2cb9e7477..f52aa5f7da52 100755 > --- a/tools/testing/selftests/net/unicast_extensions.sh > +++ b/tools/testing/selftests/net/unicast_extensions.sh > @@ -1,4 +1,4 @@ > -#!/bin/sh > +#!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > # > # By Seth Schoen (c) 2021, for the IPv4 Unicast Extensions Project > @@ -28,7 +28,7 @@ > # These tests provide an easy way to flip the expected result of any > # of these behaviors for testing kernel patches that change them. > > -source ./lib.sh > +source lib.sh > > # nettest can be run from PATH or from same directory as this selftest > if ! which nettest >/dev/null; then > > base-commit: cd4d7263d58ab98fd4dee876776e4da6c328faa3 > -- > 2.34.1 > >
On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > Refer to other scripts that were converted together, simply change the > shebang to bash to fix the following issues when the default /bin/sh > points to other shells. > > # selftests: net: unicast_extensions.sh > # ./unicast_extensions.sh: 31: source: not found > # ########################################################################### > # Unicast address extensions tests (behavior of reserved IPv4 addresses) > # ########################################################################### > # TEST: assign and ping within 240/4 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 240/4 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (1 of 2) (is allowed) [FAIL] > # TEST: assign and ping within 0/8 (2 of 2) (is allowed) [FAIL] > # TEST: assign and ping inside 255.255/16 (is allowed) [FAIL] > # TEST: assign and ping inside 255.255.255/24 (is allowed) [FAIL] > # TEST: route between 240.5.6/24 and 255.1.2/24 (is allowed) [FAIL] > # TEST: route between 0.200/16 and 245.99/16 (is allowed) [FAIL] > # TEST: assign and ping lowest address (/24) [FAIL] > # TEST: assign and ping lowest address (/26) [FAIL] > # TEST: routing using lowest address [FAIL] > # TEST: assigning 0.0.0.0 (is forbidden) [ OK ] > # TEST: assigning 255.255.255.255 (is forbidden) [ OK ] > # TEST: assign and ping inside 127/8 (is forbidden) [ OK ] > # TEST: assign and ping class D address (is forbidden) [ OK ] > # TEST: routing using class D (is forbidden) [ OK ] > # TEST: routing using 127/8 (is forbidden) [ OK ] > not ok 51 selftests: net: unicast_extensions.sh # exit=1 > > v1 -> v2: > - Fix pmtu.sh which has the same issue as unicast_extensions.sh, > suggested by Hangbin > - Change the style of the "source" line to be consistent with other > tests, suggested by Hangbin > > Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] > Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] Also, please add the missing Fixes tags. > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Yujie Liu <yujie.liu@intel.com>
On Sun, Dec 31, 2023 at 01:17:11PM +0100, Guillaume Nault wrote: > On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > > The patch set [1] added a general lib.sh in net selftests, and converted > > several test scripts to source the lib.sh. > > > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > > have a /bin/sh shebang which may point to various shells in different > > distributions, but "source" is only available in some of them. For > > example, "source" is a built-it function in bash, but it cannot be > > used in dash. > > > > Refer to other scripts that were converted together, simply change the > > shebang to bash to fix the following issues when the default /bin/sh > > points to other shells. > > Looks like it'd be simpler to just replace the "source" commands with > "." and leave the shebang as is (unless there are other bash-specific > constructs in these scripts of course). > > Generally speaking, I think we should avoid madating a specific shell, > unless that really simplifies the test script (which is not the case > here). Hi Guillaume, Thanks for the comments. As this is related with a large patch series from Hangbin, and other scripts use "source" during the conversion, so we may need some input from Hangbin. Hi Hangbin, Could you please share your comments on this? Would you like to replace "source" with "." for these two specific scripts as Guillaume suggested, or change the shebang from "sh" to "bash"? Best Regards, Yujie
On Tue, Jan 02, 2024 at 01:51:56PM +0800, Yujie Liu wrote: > > Looks like it'd be simpler to just replace the "source" commands with > > "." and leave the shebang as is (unless there are other bash-specific > > constructs in these scripts of course). > > > > Generally speaking, I think we should avoid madating a specific shell, > > unless that really simplifies the test script (which is not the case > > here). > > Hi Guillaume, > > Thanks for the comments. As this is related with a large patch series from > Hangbin, and other scripts use "source" during the conversion, so we may > need some input from Hangbin. > > Hi Hangbin, > > Could you please share your comments on this? Would you like to replace > "source" with "." for these two specific scripts as Guillaume suggested, > or change the shebang from "sh" to "bash"? Both works for me :) Thanks Hangbin
On Sun, Dec 31, 2023 at 01:17:11PM +0100, Guillaume Nault wrote: > On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > > The patch set [1] added a general lib.sh in net selftests, and converted > > several test scripts to source the lib.sh. > > > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > > have a /bin/sh shebang which may point to various shells in different > > distributions, but "source" is only available in some of them. For > > example, "source" is a built-it function in bash, but it cannot be > > used in dash. > > > > Refer to other scripts that were converted together, simply change the > > shebang to bash to fix the following issues when the default /bin/sh > > points to other shells. > > Looks like it'd be simpler to just replace the "source" commands with > "." and leave the shebang as is (unless there are other bash-specific > constructs in these scripts of course). > > Generally speaking, I think we should avoid madating a specific shell, > unless that really simplifies the test script (which is not the case > here). Hi Guillaume, Thanks for the comments. Actually I also considered replacing "source" with "." at first, but finally decided to change the shebang in consideration of being consistent with other scripts. We can see that there are 140+ scripts in net selftests that have "source lib.sh" and "bash" shebang, but none of the selftests has ". lib.sh". If we replace "source" with "." and keep the "sh" shebang specifically for unicast_extensions.sh and pmtu.sh, we will get only 2 scripts using "sh and ." while most other scripts using "bash and source". Maybe it would be nice to keep the consistency by changing the shebang as well? What do you think? :) linux/tools/testing/selftests/net$ grep -rl "source lib.sh" . | xargs grep -F '#!/bin/' ./test_vxlan_under_vrf.sh:#!/bin/bash ./test_vxlan_nolocalbypass.sh:#!/bin/bash ./xfrm_policy.sh:#!/bin/bash ./test_vxlan_mdb.sh:#!/bin/bash ./test_bridge_backup_port.sh:#!/bin/bash ./vrf_route_leaking.sh:#!/bin/bash ./l2tp.sh:#!/bin/bash ./netns-name.sh:#!/bin/bash ./rtnetlink.sh:#!/bin/bash ./ioam6.sh:#!/bin/bash ./drop_monitor_tests.sh:#!/bin/bash ./test_vxlan_vnifiltering.sh:#!/bin/bash ./icmp.sh:#!/bin/bash ./gre_gso.sh:#!/bin/bash ./fib_nexthop_multiprefix.sh:#!/bin/bash ./icmp_redirect.sh:#!/bin/bash ./vrf-xfrm-tests.sh:#!/bin/bash ./vrf_strict_mode_test.sh:#!/bin/bash ./fcnal-test.sh:#!/bin/bash ./stress_reuseport_listen.sh:#!/bin/bash ./srv6_end_dt4_l3vpn_test.sh:#!/bin/bash ./test_bridge_neigh_suppress.sh:#!/bin/bash ./cmsg_ipv6.sh:#!/bin/bash ./arp_ndisc_evict_nocarrier.sh:#!/bin/bash ./fib_rule_tests.sh:#!/bin/bash ./srv6_end_dt6_l3vpn_test.sh:#!/bin/bash ./forwarding/custom_multipath_hash.sh:#!/bin/bash ./forwarding/gre_inner_v4_multipath.sh:#!/bin/bash ./forwarding/tc_tunnel_key.sh:#!/bin/bash ./forwarding/tc_shblocks.sh:#!/bin/bash ./forwarding/router_nh.sh:#!/bin/bash ./forwarding/skbedit_priority.sh:#!/bin/bash ./forwarding/tc_mpls_l2vpn.sh:#!/bin/bash ./forwarding/gre_inner_v6_multipath.sh:#!/bin/bash ./forwarding/vxlan_symmetric.sh:#!/bin/bash ./forwarding/bridge_mdb.sh:#!/bin/bash ./forwarding/no_forwarding.sh:#!/bin/bash ./forwarding/router_bridge_1d.sh:#!/bin/bash ./forwarding/tc_flower_port_range.sh:#!/bin/bash ./forwarding/router_multicast.sh:#!/bin/bash ./forwarding/bridge_locked_port.sh:#!/bin/bash ./forwarding/vxlan_asymmetric_ipv6.sh:#!/bin/bash ./forwarding/dual_vxlan_bridge.sh:#!/bin/bash ./forwarding/bridge_port_isolation.sh:#!/bin/bash ./forwarding/local_termination.sh:#!/bin/bash ./forwarding/ipip_flat_gre_keys.sh:#!/bin/bash ./forwarding/gre_multipath_nh_res.sh:#!/bin/bash ./forwarding/gre_multipath.sh:#!/bin/bash ./forwarding/vxlan_bridge_1d_ipv6.sh:#!/bin/bash ./forwarding/ip6gre_flat_keys.sh:#!/bin/bash ./forwarding/gre_multipath_nh.sh:#!/bin/bash ./forwarding/bridge_mld.sh:#!/bin/bash ./forwarding/ip6gre_inner_v6_multipath.sh:#!/bin/bash ./forwarding/ip6gre_flat_key.sh:#!/bin/bash ./forwarding/vxlan_asymmetric.sh:#!/bin/bash ./forwarding/tc_flower_router.sh:#!/bin/bash ./forwarding/router_bridge_vlan_upper_pvid.sh:#!/bin/bash ./forwarding/mirror_gre_vlan_bridge_1q.sh:#!/bin/bash ./forwarding/q_in_vni_ipv6.sh:#!/bin/bash ./forwarding/mirror_gre_lag_lacp.sh:#!/bin/bash ./forwarding/ip6gre_custom_multipath_hash.sh:#!/bin/bash ./forwarding/vxlan_bridge_1d.sh:#!/bin/bash ./forwarding/ip6gre_hier_key.sh:#!/bin/bash ./forwarding/gre_custom_multipath_hash.sh:#!/bin/bash ./forwarding/ipip_flat_gre_key.sh:#!/bin/bash ./forwarding/mirror_gre_flower.sh:#!/bin/bash ./forwarding/router_bridge.sh:#!/bin/bash ./forwarding/vxlan_symmetric_ipv6.sh:#!/bin/bash ./forwarding/mirror_gre_bridge_1q.sh:#!/bin/bash ./forwarding/router_multipath.sh:#!/bin/bash ./forwarding/tc_vlan_modify.sh:#!/bin/bash ./forwarding/vxlan_bridge_1q.sh:#!/bin/bash ./forwarding/bridge_mdb_port_down.sh:#!/bin/bash ./forwarding/tc_flower.sh:#!/bin/bash ./forwarding/tc_flower_cfm.sh:#!/bin/bash ./forwarding/mirror_gre_neigh.sh:#!/bin/bash ./forwarding/ethtool_rmon.sh:#!/bin/bash ./forwarding/hw_stats_l3_gre.sh:#!/bin/bash ./forwarding/router.sh:#!/bin/bash ./forwarding/ipip_hier_gre_key.sh:#!/bin/bash ./forwarding/tc_police.sh:#!/bin/bash ./forwarding/pedit_ip.sh:#!/bin/bash ./forwarding/ip6_forward_instats_vrf.sh:#!/bin/bash ./forwarding/router_mpath_nh_res.sh:#!/bin/bash ./forwarding/mirror_gre_changes.sh:#!/bin/bash ./forwarding/hw_stats_l3.sh:#!/bin/bash ./forwarding/ipip_hier_gre.sh:#!/bin/bash ./forwarding/q_in_vni.sh:#!/bin/bash ./forwarding/ip6gre_flat.sh:#!/bin/bash ./forwarding/router_bridge_vlan_upper.sh:#!/bin/bash ./forwarding/bridge_igmp.sh:#!/bin/bash ./forwarding/mirror_gre_nh.sh:#!/bin/bash ./forwarding/bridge_mdb_host.sh:#!/bin/bash ./forwarding/ipip_hier_gre_keys.sh:#!/bin/bash ./forwarding/pedit_dsfield.sh:#!/bin/bash ./forwarding/bridge_vlan_mcast.sh:#!/bin/bash ./forwarding/mirror_gre_bridge_1d_vlan.sh:#!/bin/bash ./forwarding/router_bridge_1d_lag.sh:#!/bin/bash ./forwarding/router_bridge_pvid_vlan_upper.sh:#!/bin/bash ./forwarding/mirror_gre_bound.sh:#!/bin/bash ./forwarding/ip6gre_hier.sh:#!/bin/bash ./forwarding/ip6gre_hier_keys.sh:#!/bin/bash ./forwarding/ethtool_extended_state.sh:#!/bin/bash ./forwarding/router_mpath_nh.sh:#!/bin/bash ./forwarding/tc_flower_l2_miss.sh:#!/bin/bash ./forwarding/bridge_vlan_unaware.sh:#!/bin/bash ./forwarding/router_broadcast.sh:#!/bin/bash ./forwarding/bridge_fdb_learning_limit.sh:#!/bin/bash ./forwarding/ipip_lib.sh:#!/bin/bash ./forwarding/ip6gre_inner_v4_multipath.sh:#!/bin/bash ./forwarding/router_vid_1.sh:#!/bin/bash ./forwarding/mirror_gre.sh:#!/bin/bash ./forwarding/router_bridge_vlan.sh:#!/bin/bash ./forwarding/bridge_vlan_aware.sh:#!/bin/bash ./forwarding/ethtool.sh:#!/bin/bash ./forwarding/loopback.sh:#!/bin/bash ./forwarding/bridge_sticky_fdb.sh:#!/bin/bash ./forwarding/bridge_mdb_max.sh:#!/bin/bash ./forwarding/pedit_l4port.sh:#!/bin/bash ./forwarding/tc_actions.sh:#!/bin/bash ./forwarding/mirror_vlan.sh:#!/bin/bash ./forwarding/sch_red.sh:#!/bin/bash ./forwarding/ipip_flat_gre.sh:#!/bin/bash ./forwarding/mirror_gre_bridge_1d.sh:#!/bin/bash ./forwarding/lib.sh:#!/bin/bash ./forwarding/mirror_gre_vlan.sh:#!/bin/bash ./forwarding/mirror_gre_bridge_1q_lag.sh:#!/bin/bash ./forwarding/ethtool_mm.sh:#!/bin/bash ./forwarding/vxlan_bridge_1q_ipv6.sh:#!/bin/bash ./forwarding/tc_chains.sh:#!/bin/bash ./forwarding/ip6gre_lib.sh:#!/bin/bash ./fib_nexthop_nongw.sh:#!/bin/bash ./srv6_end_dt46_l3vpn_test.sh:#!/bin/bash ./cmsg_so_mark.sh:#!/bin/bash ./sctp_vrf.sh:#!/bin/bash ./fdb_flush.sh:#!/bin/bash ./ndisc_unsolicited_na_test.sh:#!/bin/bash ./traceroute.sh:#!/bin/bash ./fib-onlink-tests.sh:#!/bin/bash ./fib_tests.sh:#!/bin/bash ./cmsg_time.sh:#!/bin/bash ./arp_ndisc_untracked_subnets.sh:#!/bin/bash ./fib_nexthops.sh:#!/bin/bash linux/tools/testing/selftests/net$ grep -rF ". lib.sh" <-- nothing Thanks, Yujie
On Tue, Jan 02, 2024 at 04:20:16PM +0800, Yujie Liu wrote: > On Sun, Dec 31, 2023 at 01:17:11PM +0100, Guillaume Nault wrote: > > On Fri, Dec 29, 2023 at 09:19:31PM +0800, Yujie Liu wrote: > > > The patch set [1] added a general lib.sh in net selftests, and converted > > > several test scripts to source the lib.sh. > > > > > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > > > have a /bin/sh shebang which may point to various shells in different > > > distributions, but "source" is only available in some of them. For > > > example, "source" is a built-it function in bash, but it cannot be > > > used in dash. > > > > > > Refer to other scripts that were converted together, simply change the > > > shebang to bash to fix the following issues when the default /bin/sh > > > points to other shells. > > > > Looks like it'd be simpler to just replace the "source" commands with > > "." and leave the shebang as is (unless there are other bash-specific > > constructs in these scripts of course). > > > > Generally speaking, I think we should avoid madating a specific shell, > > unless that really simplifies the test script (which is not the case > > here). > > Hi Guillaume, > > Thanks for the comments. Actually I also considered replacing "source" > with "." at first, but finally decided to change the shebang in > consideration of being consistent with other scripts. We can see that > there are 140+ scripts in net selftests that have "source lib.sh" and > "bash" shebang, but none of the selftests has ". lib.sh". If we replace > "source" with "." and keep the "sh" shebang specifically for > unicast_extensions.sh and pmtu.sh, we will get only 2 scripts using > "sh and ." while most other scripts using "bash and source". Maybe it > would be nice to keep the consistency by changing the shebang as well? > What do you think? :) The use of "source" instead of "." is clearly an overlook. Consistency is desirable only when it brings better quality code. And it should be easy enough to convert the remaining "source lib.sh" in a followup patch to make the other shell scripts consistent. > linux/tools/testing/selftests/net$ grep -rl "source lib.sh" . | xargs grep -F '#!/bin/' > ./test_vxlan_under_vrf.sh:#!/bin/bash > ./test_vxlan_nolocalbypass.sh:#!/bin/bash > ./xfrm_policy.sh:#!/bin/bash > ./test_vxlan_mdb.sh:#!/bin/bash > ./test_bridge_backup_port.sh:#!/bin/bash > ./vrf_route_leaking.sh:#!/bin/bash > ./l2tp.sh:#!/bin/bash > ./netns-name.sh:#!/bin/bash > ./rtnetlink.sh:#!/bin/bash > ./ioam6.sh:#!/bin/bash > ./drop_monitor_tests.sh:#!/bin/bash > ./test_vxlan_vnifiltering.sh:#!/bin/bash > ./icmp.sh:#!/bin/bash > ./gre_gso.sh:#!/bin/bash > ./fib_nexthop_multiprefix.sh:#!/bin/bash > ./icmp_redirect.sh:#!/bin/bash > ./vrf-xfrm-tests.sh:#!/bin/bash > ./vrf_strict_mode_test.sh:#!/bin/bash > ./fcnal-test.sh:#!/bin/bash > ./stress_reuseport_listen.sh:#!/bin/bash > ./srv6_end_dt4_l3vpn_test.sh:#!/bin/bash > ./test_bridge_neigh_suppress.sh:#!/bin/bash > ./cmsg_ipv6.sh:#!/bin/bash > ./arp_ndisc_evict_nocarrier.sh:#!/bin/bash > ./fib_rule_tests.sh:#!/bin/bash > ./srv6_end_dt6_l3vpn_test.sh:#!/bin/bash > ./forwarding/custom_multipath_hash.sh:#!/bin/bash > ./forwarding/gre_inner_v4_multipath.sh:#!/bin/bash > ./forwarding/tc_tunnel_key.sh:#!/bin/bash > ./forwarding/tc_shblocks.sh:#!/bin/bash > ./forwarding/router_nh.sh:#!/bin/bash > ./forwarding/skbedit_priority.sh:#!/bin/bash > ./forwarding/tc_mpls_l2vpn.sh:#!/bin/bash > ./forwarding/gre_inner_v6_multipath.sh:#!/bin/bash > ./forwarding/vxlan_symmetric.sh:#!/bin/bash > ./forwarding/bridge_mdb.sh:#!/bin/bash > ./forwarding/no_forwarding.sh:#!/bin/bash > ./forwarding/router_bridge_1d.sh:#!/bin/bash > ./forwarding/tc_flower_port_range.sh:#!/bin/bash > ./forwarding/router_multicast.sh:#!/bin/bash > ./forwarding/bridge_locked_port.sh:#!/bin/bash > ./forwarding/vxlan_asymmetric_ipv6.sh:#!/bin/bash > ./forwarding/dual_vxlan_bridge.sh:#!/bin/bash > ./forwarding/bridge_port_isolation.sh:#!/bin/bash > ./forwarding/local_termination.sh:#!/bin/bash > ./forwarding/ipip_flat_gre_keys.sh:#!/bin/bash > ./forwarding/gre_multipath_nh_res.sh:#!/bin/bash > ./forwarding/gre_multipath.sh:#!/bin/bash > ./forwarding/vxlan_bridge_1d_ipv6.sh:#!/bin/bash > ./forwarding/ip6gre_flat_keys.sh:#!/bin/bash > ./forwarding/gre_multipath_nh.sh:#!/bin/bash > ./forwarding/bridge_mld.sh:#!/bin/bash > ./forwarding/ip6gre_inner_v6_multipath.sh:#!/bin/bash > ./forwarding/ip6gre_flat_key.sh:#!/bin/bash > ./forwarding/vxlan_asymmetric.sh:#!/bin/bash > ./forwarding/tc_flower_router.sh:#!/bin/bash > ./forwarding/router_bridge_vlan_upper_pvid.sh:#!/bin/bash > ./forwarding/mirror_gre_vlan_bridge_1q.sh:#!/bin/bash > ./forwarding/q_in_vni_ipv6.sh:#!/bin/bash > ./forwarding/mirror_gre_lag_lacp.sh:#!/bin/bash > ./forwarding/ip6gre_custom_multipath_hash.sh:#!/bin/bash > ./forwarding/vxlan_bridge_1d.sh:#!/bin/bash > ./forwarding/ip6gre_hier_key.sh:#!/bin/bash > ./forwarding/gre_custom_multipath_hash.sh:#!/bin/bash > ./forwarding/ipip_flat_gre_key.sh:#!/bin/bash > ./forwarding/mirror_gre_flower.sh:#!/bin/bash > ./forwarding/router_bridge.sh:#!/bin/bash > ./forwarding/vxlan_symmetric_ipv6.sh:#!/bin/bash > ./forwarding/mirror_gre_bridge_1q.sh:#!/bin/bash > ./forwarding/router_multipath.sh:#!/bin/bash > ./forwarding/tc_vlan_modify.sh:#!/bin/bash > ./forwarding/vxlan_bridge_1q.sh:#!/bin/bash > ./forwarding/bridge_mdb_port_down.sh:#!/bin/bash > ./forwarding/tc_flower.sh:#!/bin/bash > ./forwarding/tc_flower_cfm.sh:#!/bin/bash > ./forwarding/mirror_gre_neigh.sh:#!/bin/bash > ./forwarding/ethtool_rmon.sh:#!/bin/bash > ./forwarding/hw_stats_l3_gre.sh:#!/bin/bash > ./forwarding/router.sh:#!/bin/bash > ./forwarding/ipip_hier_gre_key.sh:#!/bin/bash > ./forwarding/tc_police.sh:#!/bin/bash > ./forwarding/pedit_ip.sh:#!/bin/bash > ./forwarding/ip6_forward_instats_vrf.sh:#!/bin/bash > ./forwarding/router_mpath_nh_res.sh:#!/bin/bash > ./forwarding/mirror_gre_changes.sh:#!/bin/bash > ./forwarding/hw_stats_l3.sh:#!/bin/bash > ./forwarding/ipip_hier_gre.sh:#!/bin/bash > ./forwarding/q_in_vni.sh:#!/bin/bash > ./forwarding/ip6gre_flat.sh:#!/bin/bash > ./forwarding/router_bridge_vlan_upper.sh:#!/bin/bash > ./forwarding/bridge_igmp.sh:#!/bin/bash > ./forwarding/mirror_gre_nh.sh:#!/bin/bash > ./forwarding/bridge_mdb_host.sh:#!/bin/bash > ./forwarding/ipip_hier_gre_keys.sh:#!/bin/bash > ./forwarding/pedit_dsfield.sh:#!/bin/bash > ./forwarding/bridge_vlan_mcast.sh:#!/bin/bash > ./forwarding/mirror_gre_bridge_1d_vlan.sh:#!/bin/bash > ./forwarding/router_bridge_1d_lag.sh:#!/bin/bash > ./forwarding/router_bridge_pvid_vlan_upper.sh:#!/bin/bash > ./forwarding/mirror_gre_bound.sh:#!/bin/bash > ./forwarding/ip6gre_hier.sh:#!/bin/bash > ./forwarding/ip6gre_hier_keys.sh:#!/bin/bash > ./forwarding/ethtool_extended_state.sh:#!/bin/bash > ./forwarding/router_mpath_nh.sh:#!/bin/bash > ./forwarding/tc_flower_l2_miss.sh:#!/bin/bash > ./forwarding/bridge_vlan_unaware.sh:#!/bin/bash > ./forwarding/router_broadcast.sh:#!/bin/bash > ./forwarding/bridge_fdb_learning_limit.sh:#!/bin/bash > ./forwarding/ipip_lib.sh:#!/bin/bash > ./forwarding/ip6gre_inner_v4_multipath.sh:#!/bin/bash > ./forwarding/router_vid_1.sh:#!/bin/bash > ./forwarding/mirror_gre.sh:#!/bin/bash > ./forwarding/router_bridge_vlan.sh:#!/bin/bash > ./forwarding/bridge_vlan_aware.sh:#!/bin/bash > ./forwarding/ethtool.sh:#!/bin/bash > ./forwarding/loopback.sh:#!/bin/bash > ./forwarding/bridge_sticky_fdb.sh:#!/bin/bash > ./forwarding/bridge_mdb_max.sh:#!/bin/bash > ./forwarding/pedit_l4port.sh:#!/bin/bash > ./forwarding/tc_actions.sh:#!/bin/bash > ./forwarding/mirror_vlan.sh:#!/bin/bash > ./forwarding/sch_red.sh:#!/bin/bash > ./forwarding/ipip_flat_gre.sh:#!/bin/bash > ./forwarding/mirror_gre_bridge_1d.sh:#!/bin/bash > ./forwarding/lib.sh:#!/bin/bash > ./forwarding/mirror_gre_vlan.sh:#!/bin/bash > ./forwarding/mirror_gre_bridge_1q_lag.sh:#!/bin/bash > ./forwarding/ethtool_mm.sh:#!/bin/bash > ./forwarding/vxlan_bridge_1q_ipv6.sh:#!/bin/bash > ./forwarding/tc_chains.sh:#!/bin/bash > ./forwarding/ip6gre_lib.sh:#!/bin/bash > ./fib_nexthop_nongw.sh:#!/bin/bash > ./srv6_end_dt46_l3vpn_test.sh:#!/bin/bash > ./cmsg_so_mark.sh:#!/bin/bash > ./sctp_vrf.sh:#!/bin/bash > ./fdb_flush.sh:#!/bin/bash > ./ndisc_unsolicited_na_test.sh:#!/bin/bash > ./traceroute.sh:#!/bin/bash > ./fib-onlink-tests.sh:#!/bin/bash > ./fib_tests.sh:#!/bin/bash > ./cmsg_time.sh:#!/bin/bash > ./arp_ndisc_untracked_subnets.sh:#!/bin/bash > ./fib_nexthops.sh:#!/bin/bash > > linux/tools/testing/selftests/net$ grep -rF ". lib.sh" > <-- nothing > > Thanks, > Yujie >
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 29 Dec 2023 21:19:31 +0800 you wrote: > The patch set [1] added a general lib.sh in net selftests, and converted > several test scripts to source the lib.sh. > > unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) > have a /bin/sh shebang which may point to various shells in different > distributions, but "source" is only available in some of them. For > example, "source" is a built-it function in bash, but it cannot be > used in dash. > > [...] Here is the summary with links: - [v2,net-next] selftests/net: change shebang to bash to support "source" https://git.kernel.org/netdev/net-next/c/05d92cb0e919 You are awesome, thank you!
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh index 175d3d1d773b..f10879788f61 100755 --- a/tools/testing/selftests/net/pmtu.sh +++ b/tools/testing/selftests/net/pmtu.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # SPDX-License-Identifier: GPL-2.0 # # Check that route PMTU values match expectations, and that initial device MTU @@ -198,7 +198,7 @@ # - pmtu_ipv6_route_change # Same as above but with IPv6 -source ./lib.sh +source lib.sh PAUSE_ON_FAIL=no VERBOSE=0 diff --git a/tools/testing/selftests/net/unicast_extensions.sh b/tools/testing/selftests/net/unicast_extensions.sh index b7a2cb9e7477..f52aa5f7da52 100755 --- a/tools/testing/selftests/net/unicast_extensions.sh +++ b/tools/testing/selftests/net/unicast_extensions.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # SPDX-License-Identifier: GPL-2.0 # # By Seth Schoen (c) 2021, for the IPv4 Unicast Extensions Project @@ -28,7 +28,7 @@ # These tests provide an easy way to flip the expected result of any # of these behaviors for testing kernel patches that change them. -source ./lib.sh +source lib.sh # nettest can be run from PATH or from same directory as this selftest if ! which nettest >/dev/null; then
The patch set [1] added a general lib.sh in net selftests, and converted several test scripts to source the lib.sh. unicast_extensions.sh (converted in [1]) and pmtu.sh (converted in [2]) have a /bin/sh shebang which may point to various shells in different distributions, but "source" is only available in some of them. For example, "source" is a built-it function in bash, but it cannot be used in dash. Refer to other scripts that were converted together, simply change the shebang to bash to fix the following issues when the default /bin/sh points to other shells. # selftests: net: unicast_extensions.sh # ./unicast_extensions.sh: 31: source: not found # ########################################################################### # Unicast address extensions tests (behavior of reserved IPv4 addresses) # ########################################################################### # TEST: assign and ping within 240/4 (1 of 2) (is allowed) [FAIL] # TEST: assign and ping within 240/4 (2 of 2) (is allowed) [FAIL] # TEST: assign and ping within 0/8 (1 of 2) (is allowed) [FAIL] # TEST: assign and ping within 0/8 (2 of 2) (is allowed) [FAIL] # TEST: assign and ping inside 255.255/16 (is allowed) [FAIL] # TEST: assign and ping inside 255.255.255/24 (is allowed) [FAIL] # TEST: route between 240.5.6/24 and 255.1.2/24 (is allowed) [FAIL] # TEST: route between 0.200/16 and 245.99/16 (is allowed) [FAIL] # TEST: assign and ping lowest address (/24) [FAIL] # TEST: assign and ping lowest address (/26) [FAIL] # TEST: routing using lowest address [FAIL] # TEST: assigning 0.0.0.0 (is forbidden) [ OK ] # TEST: assigning 255.255.255.255 (is forbidden) [ OK ] # TEST: assign and ping inside 127/8 (is forbidden) [ OK ] # TEST: assign and ping class D address (is forbidden) [ OK ] # TEST: routing using class D (is forbidden) [ OK ] # TEST: routing using 127/8 (is forbidden) [ OK ] not ok 51 selftests: net: unicast_extensions.sh # exit=1 v1 -> v2: - Fix pmtu.sh which has the same issue as unicast_extensions.sh, suggested by Hangbin - Change the style of the "source" line to be consistent with other tests, suggested by Hangbin Link: https://lore.kernel.org/all/20231202020110.362433-1-liuhangbin@gmail.com/ [1] Link: https://lore.kernel.org/all/20231219094856.1740079-1-liuhangbin@gmail.com/ [2] Reported-by: kernel test robot <oliver.sang@intel.com> Signed-off-by: Yujie Liu <yujie.liu@intel.com> --- tools/testing/selftests/net/pmtu.sh | 4 ++-- tools/testing/selftests/net/unicast_extensions.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) base-commit: cd4d7263d58ab98fd4dee876776e4da6c328faa3