| Message ID | a1c56680a5041ae337a6a44a7091bd8f781c1970.1702295081.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] selftests: forwarding: Import top-level lib.sh through $lib_dir | expand |
On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: > The commit cited below moved code from net/forwarding/lib.sh to net/lib.sh, > and had the former source the latter. This causes issues with selftests > from directories other than net/forwarding/, in particular drivers/net/... > For those files, the line "source ../lib.sh" would look for lib.sh in the > directory above the script file's one, which is incorrect. The way these > selftests source net/forwarding/lib.sh is like this: > > lib_dir=$(dirname $0)/../../../net/forwarding > source $lib_dir/lib.sh > > Reuse the variable lib_dir, if set, in net/forwarding/lib.sh to source > net/lib.sh as well. > > Fixes: 25ae948b4478 ("selftests/net: add lib.sh") > Cc: Hangbin Liu <liuhangbin@gmail.com> > Signed-off-by: Petr Machata <petrm@nvidia.com> > --- > tools/testing/selftests/net/forwarding/lib.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh > index 038b7d956722..0feb4e400110 100755 > --- a/tools/testing/selftests/net/forwarding/lib.sh > +++ b/tools/testing/selftests/net/forwarding/lib.sh > @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then > source "$relative_path/forwarding.config" > fi > > -source ../lib.sh > +source ${lib_dir-.}/../lib.sh > ############################################################################## > # Sanity checks Hi Petr, Thanks for the report. However, this doesn't fix the soft link scenario. e.g. The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. So how about something like: diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index 8f6ca458af9a..7f90248e05d6 100755 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then source "$relative_path/forwarding.config" fi -source ../lib.sh +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) +source ${forwarding_dir}/../lib.sh Thanks Hangbin
Hangbin Liu <liuhangbin@gmail.com> writes: > On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: > >> @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then >> source "$relative_path/forwarding.config" >> fi >> >> -source ../lib.sh >> +source ${lib_dir-.}/../lib.sh >> ############################################################################## >> # Sanity checks > > Hi Petr, > > Thanks for the report. However, this doesn't fix the soft link scenario. e.g. > The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link > net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. I see, I didn't realize those exist. > So how about something like: > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh > index 8f6ca458af9a..7f90248e05d6 100755 > --- a/tools/testing/selftests/net/forwarding/lib.sh > +++ b/tools/testing/selftests/net/forwarding/lib.sh > @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then > source "$relative_path/forwarding.config" > fi > > -source ../lib.sh > +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) > +source ${forwarding_dir}/../lib.sh Yep, that's gonna work. I'll pass through our tests and send later this week.
On 2023-12-12 18:22 +0100, Petr Machata wrote: > > Hangbin Liu <liuhangbin@gmail.com> writes: > > > On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: > > > >> @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then > >> source "$relative_path/forwarding.config" > >> fi > >> > >> -source ../lib.sh > >> +source ${lib_dir-.}/../lib.sh > >> ############################################################################## > >> # Sanity checks > > > > Hi Petr, > > > > Thanks for the report. However, this doesn't fix the soft link scenario. e.g. > > The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link > > net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. > > I see, I didn't realize those exist. > > > So how about something like: > > > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh > > index 8f6ca458af9a..7f90248e05d6 100755 > > --- a/tools/testing/selftests/net/forwarding/lib.sh > > +++ b/tools/testing/selftests/net/forwarding/lib.sh > > @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then > > source "$relative_path/forwarding.config" > > fi > > > > -source ../lib.sh > > +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) > > +source ${forwarding_dir}/../lib.sh > > Yep, that's gonna work. > I'll pass through our tests and send later this week. > There is also another related issue which is that generating a test archive using gen_tar for the tests under drivers/net/bonding does not include the new lib.sh. This is similar to the issue reported here: https://lore.kernel.org/netdev/40f04ded-0c86-8669-24b1-9a313ca21076@redhat.com/ /tmp/x# ./run_kselftest.sh TAP version 13 [...] # timeout set to 120 # selftests: drivers/net/bonding: dev_addr_lists.sh # ./net_forwarding_lib.sh: line 41: ../lib.sh: No such file or directory # TEST: bonding cleanup mode active-backup [ OK ] # TEST: bonding cleanup mode 802.3ad [ OK ] # TEST: bonding LACPDU multicast address to slave (from bond down) [ OK ] # TEST: bonding LACPDU multicast address to slave (from bond up) [ OK ] ok 4 selftests: drivers/net/bonding: dev_addr_lists.sh [...]
On Tue, Dec 12, 2023 at 03:17:01PM -0500, Benjamin Poirier wrote: > On 2023-12-12 18:22 +0100, Petr Machata wrote: > > > > Hangbin Liu <liuhangbin@gmail.com> writes: > > > > > On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: > > > > > >> @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then > > >> source "$relative_path/forwarding.config" > > >> fi > > >> > > >> -source ../lib.sh > > >> +source ${lib_dir-.}/../lib.sh > > >> ############################################################################## > > >> # Sanity checks > > > > > > Hi Petr, > > > > > > Thanks for the report. However, this doesn't fix the soft link scenario. e.g. > > > The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link > > > net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. > > > > I see, I didn't realize those exist. > > > > > So how about something like: > > > > > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh > > > index 8f6ca458af9a..7f90248e05d6 100755 > > > --- a/tools/testing/selftests/net/forwarding/lib.sh > > > +++ b/tools/testing/selftests/net/forwarding/lib.sh > > > @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then > > > source "$relative_path/forwarding.config" > > > fi > > > > > > -source ../lib.sh > > > +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) > > > +source ${forwarding_dir}/../lib.sh > > > > Yep, that's gonna work. > > I'll pass through our tests and send later this week. > > > > There is also another related issue which is that generating a test > archive using gen_tar for the tests under drivers/net/bonding does not > include the new lib.sh. This is similar to the issue reported here: > https://lore.kernel.org/netdev/40f04ded-0c86-8669-24b1-9a313ca21076@redhat.com/ > > /tmp/x# ./run_kselftest.sh > TAP version 13 > [...] > # timeout set to 120 > # selftests: drivers/net/bonding: dev_addr_lists.sh > # ./net_forwarding_lib.sh: line 41: ../lib.sh: No such file or directory > # TEST: bonding cleanup mode active-backup [ OK ] > # TEST: bonding cleanup mode 802.3ad [ OK ] > # TEST: bonding LACPDU multicast address to slave (from bond down) [ OK ] > # TEST: bonding LACPDU multicast address to slave (from bond up) [ OK ] > ok 4 selftests: drivers/net/bonding: dev_addr_lists.sh > [...] Hmm.. Is it possible to write a rule in the Makefile to create the net/ and net/forwarding folder so we can source the relative path directly. e.g. ]# tree . ├── drivers │ └── net │ └── bonding │ ├── bond-arp-interval-causes-panic.sh │ ├── ... │ └── settings ├── kselftest │ ├── module.sh │ ├── prefix.pl │ └── runner.sh ├── kselftest-list.txt ├── net │ ├── forwarding │ │ └── lib.sh │ └── lib.sh └── run_kselftest.sh Thanks Hangbin
Benjamin Poirier <benjamin.poirier@gmail.com> writes: > On 2023-12-12 18:22 +0100, Petr Machata wrote: >> >> Hangbin Liu <liuhangbin@gmail.com> writes: >> >> > On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: >> > >> >> @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then >> >> source "$relative_path/forwarding.config" >> >> fi >> >> >> >> -source ../lib.sh >> >> +source ${lib_dir-.}/../lib.sh >> >> ############################################################################## >> >> # Sanity checks >> > >> > Hi Petr, >> > >> > Thanks for the report. However, this doesn't fix the soft link scenario. e.g. >> > The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link >> > net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. >> >> I see, I didn't realize those exist. >> >> > So how about something like: >> > >> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh >> > index 8f6ca458af9a..7f90248e05d6 100755 >> > --- a/tools/testing/selftests/net/forwarding/lib.sh >> > +++ b/tools/testing/selftests/net/forwarding/lib.sh >> > @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then >> > source "$relative_path/forwarding.config" >> > fi >> > >> > -source ../lib.sh >> > +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) >> > +source ${forwarding_dir}/../lib.sh >> >> Yep, that's gonna work. >> I'll pass through our tests and send later this week. > > There is also another related issue which is that generating a test > archive using gen_tar for the tests under drivers/net/bonding does not > include the new lib.sh. This is similar to the issue reported here: > https://lore.kernel.org/netdev/40f04ded-0c86-8669-24b1-9a313ca21076@redhat.com/ > > /tmp/x# ./run_kselftest.sh > TAP version 13 > [...] > # timeout set to 120 > # selftests: drivers/net/bonding: dev_addr_lists.sh > # ./net_forwarding_lib.sh: line 41: ../lib.sh: No such file or directory > # TEST: bonding cleanup mode active-backup [ OK ] > # TEST: bonding cleanup mode 802.3ad [ OK ] > # TEST: bonding LACPDU multicast address to slave (from bond down) [ OK ] > # TEST: bonding LACPDU multicast address to slave (from bond up) [ OK ] > ok 4 selftests: drivers/net/bonding: dev_addr_lists.sh > [...] The issue is that the symlink becomes a text file during install, and readlink on that file then becomes a nop. Maybe the bonding tests should include net/forwarding/lib.sh through a relative path like other tests in drivers/, instead of this symlink business?
Petr Machata <me@pmachata.org> writes: > Benjamin Poirier <benjamin.poirier@gmail.com> writes: > >> On 2023-12-12 18:22 +0100, Petr Machata wrote: >>> >>> Hangbin Liu <liuhangbin@gmail.com> writes: >>> >>> > On Mon, Dec 11, 2023 at 01:01:06PM +0100, Petr Machata wrote: >>> > >>> >> @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then >>> >> source "$relative_path/forwarding.config" >>> >> fi >>> >> >>> >> -source ../lib.sh >>> >> +source ${lib_dir-.}/../lib.sh >>> >> ############################################################################## >>> >> # Sanity checks >>> > >>> > Hi Petr, >>> > >>> > Thanks for the report. However, this doesn't fix the soft link scenario. e.g. >>> > The bonding tests tools/testing/selftests/drivers/net/bonding add a soft link >>> > net_forwarding_lib.sh and source it directly in dev_addr_lists.sh. >>> >>> I see, I didn't realize those exist. >>> >>> > So how about something like: >>> > >>> > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh >>> > index 8f6ca458af9a..7f90248e05d6 100755 >>> > --- a/tools/testing/selftests/net/forwarding/lib.sh >>> > +++ b/tools/testing/selftests/net/forwarding/lib.sh >>> > @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then >>> > source "$relative_path/forwarding.config" >>> > fi >>> > >>> > -source ../lib.sh >>> > +forwarding_dir=$(dirname $(readlink -f $BASH_SOURCE)) >>> > +source ${forwarding_dir}/../lib.sh >>> >>> Yep, that's gonna work. >>> I'll pass through our tests and send later this week. >> >> There is also another related issue which is that generating a test >> archive using gen_tar for the tests under drivers/net/bonding does not >> include the new lib.sh. This is similar to the issue reported here: >> https://lore.kernel.org/netdev/40f04ded-0c86-8669-24b1-9a313ca21076@redhat.com/ >> >> /tmp/x# ./run_kselftest.sh >> TAP version 13 >> [...] >> # timeout set to 120 >> # selftests: drivers/net/bonding: dev_addr_lists.sh >> # ./net_forwarding_lib.sh: line 41: ../lib.sh: No such file or directory >> # TEST: bonding cleanup mode active-backup [ OK ] >> # TEST: bonding cleanup mode 802.3ad [ OK ] >> # TEST: bonding LACPDU multicast address to slave (from bond down) [ OK ] >> # TEST: bonding LACPDU multicast address to slave (from bond up) [ OK ] >> ok 4 selftests: drivers/net/bonding: dev_addr_lists.sh >> [...] > > The issue is that the symlink becomes a text file during install, and > readlink on that file then becomes a nop. Maybe the bonding tests should > include net/forwarding/lib.sh through a relative path like other tests > in drivers/, instead of this symlink business? Or wait, the goal is for make install in the bonding directory to produce a working install, right? Hmm. This worked before exactly because the symlink became a text file. It does not work in general, but it did work for bonding. I don't have ideas how to solve this elegantly honestly. Maybe have bonding tests set $net_lib_dir and forwarding/lib.sh then source net/lib.sh through that path if set; have bonding symlink net/lib.sh in addition to forwarding/lib.sh, and set net_lib=.? It's ugly as sin, but... should work? Hmm, maybe we could side-step the issue? I suspect that vast majority of what bonding uses are just generic helpers. log_test, check_err, that sort of stuff. Unless I missed something, all of them set NUM_NETIFS=0. Those things could all be in the generic net/lib.sh. So long-term it might be possible for bonding to do the trick with symlinking, except with just net/lib.sh, not both libs. I think that most of forwarding/lib.sh actually belongs to net/lib.sh. We reinvent a lot of that functionality in various net/ tests, because, presumably, people find it odd to source forwarding/lib.sh. If it all lived in net/, we could reuse all these tools instead of cut'n'pasting them from one test to the other. Stuff like the mcast_packet_test, start/stop_traffic, etc., would probably stay in forwarding. So that's long-term. And short-term we can live with the ugly-as-sin workaround that I propose? Ideas? Comments?
On 2023-12-13 14:00 +0800, Hangbin Liu wrote: > On Tue, Dec 12, 2023 at 03:17:01PM -0500, Benjamin Poirier wrote: > > On 2023-12-12 18:22 +0100, Petr Machata wrote: [...] > > There is also another related issue which is that generating a test > > archive using gen_tar for the tests under drivers/net/bonding does not > > include the new lib.sh. This is similar to the issue reported here: > > https://lore.kernel.org/netdev/40f04ded-0c86-8669-24b1-9a313ca21076@redhat.com/ > > > > /tmp/x# ./run_kselftest.sh > > TAP version 13 > > [...] > > # timeout set to 120 > > # selftests: drivers/net/bonding: dev_addr_lists.sh > > # ./net_forwarding_lib.sh: line 41: ../lib.sh: No such file or directory > > # TEST: bonding cleanup mode active-backup [ OK ] > > # TEST: bonding cleanup mode 802.3ad [ OK ] > > # TEST: bonding LACPDU multicast address to slave (from bond down) [ OK ] > > # TEST: bonding LACPDU multicast address to slave (from bond up) [ OK ] > > ok 4 selftests: drivers/net/bonding: dev_addr_lists.sh > > [...] > > Hmm.. Is it possible to write a rule in the Makefile to create the net/ > and net/forwarding folder so we can source the relative path directly. e.g. > > ]# tree > . > ├── drivers > │ └── net > │ └── bonding > │ ├── bond-arp-interval-causes-panic.sh > │ ├── ... > │ └── settings > ├── kselftest > │ ├── module.sh > │ ├── prefix.pl > │ └── runner.sh > ├── kselftest-list.txt > ├── net > │ ├── forwarding > │ │ └── lib.sh > │ └── lib.sh > └── run_kselftest.sh That sounds like a good idea. I started to work on that approach but I'm missing recursive inclusion. For instance cd tools/testing/selftests make install TARGETS="drivers/net/bonding" ./kselftest_install/run_kselftest.sh -t drivers/net/bonding:dev_addr_lists.sh includes net/forwarding/lib.sh but is missing net/lib.sh. I feel that my 'make' skills are rusty but I guess that with enough make code, it could be done. A workaround is simply to manually list the transitive dependencies in TEST_SH_LIBS: TEST_SH_LIBS := \ - net/forwarding/lib.sh + net/forwarding/lib.sh \ + net/lib.sh I only converted a few files to validate that the approach is viable. I used the following tests: drivers/net/bonding/dev_addr_lists.sh net/test_vxlan_vnifiltering.sh net/forwarding/pedit_ip.sh Let me know what you think. --- tools/testing/selftests/Makefile | 5 ++++- tools/testing/selftests/drivers/net/bonding/Makefile | 6 ++++-- .../testing/selftests/drivers/net/bonding/dev_addr_lists.sh | 2 +- .../selftests/drivers/net/bonding/net_forwarding_lib.sh | 1 - tools/testing/selftests/lib.mk | 3 +++ tools/testing/selftests/net/forwarding/Makefile | 3 +++ tools/testing/selftests/net/forwarding/lib.sh | 3 ++- 7 files changed, 17 insertions(+), 6 deletions(-) delete mode 120000 tools/testing/selftests/drivers/net/bonding/net_forwarding_lib.sh diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 3b2061d1c1a5..0aaa7efa3015 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -256,7 +256,10 @@ ifdef INSTALL_PATH @ret=1; \ for TARGET in $(TARGETS); do \ BUILD_TARGET=$$BUILD/$$TARGET; \ - $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install \ + $(MAKE) install OUTPUT=$$BUILD_TARGET -C $$TARGET \ + INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \ + SH_LIBS_PATH=$(INSTALL_PATH) \ + BASE_PATH=$(shell pwd) \ O=$(abs_objtree) \ $(if $(FORCE_TARGETS),|| exit); \ ret=$$((ret * $$?)); \ diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile index 8a72bb7de70f..6682f8c1fe79 100644 --- a/tools/testing/selftests/drivers/net/bonding/Makefile +++ b/tools/testing/selftests/drivers/net/bonding/Makefile @@ -15,7 +15,9 @@ TEST_PROGS := \ TEST_FILES := \ lag_lib.sh \ bond_topo_2d1c.sh \ - bond_topo_3d1c.sh \ - net_forwarding_lib.sh + bond_topo_3d1c.sh + +TEST_SH_LIBS := \ + net/forwarding/lib.sh include ../../../lib.mk diff --git a/tools/testing/selftests/drivers/net/bonding/dev_addr_lists.sh b/tools/testing/selftests/drivers/net/bonding/dev_addr_lists.sh index 5cfe7d8ebc25..e6fa24eded5b 100755 --- a/tools/testing/selftests/drivers/net/bonding/dev_addr_lists.sh +++ b/tools/testing/selftests/drivers/net/bonding/dev_addr_lists.sh @@ -14,7 +14,7 @@ ALL_TESTS=" REQUIRE_MZ=no NUM_NETIFS=0 lib_dir=$(dirname "$0") -source "$lib_dir"/net_forwarding_lib.sh +source "$lib_dir"/../../../net/forwarding/lib.sh source "$lib_dir"/lag_lib.sh diff --git a/tools/testing/selftests/drivers/net/bonding/net_forwarding_lib.sh b/tools/testing/selftests/drivers/net/bonding/net_forwarding_lib.sh deleted file mode 120000 index 39c96828c5ef..000000000000 --- a/tools/testing/selftests/drivers/net/bonding/net_forwarding_lib.sh +++ /dev/null @@ -1 +0,0 @@ -../../../net/forwarding/lib.sh \ No newline at end of file diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 118e0964bda9..94b7af7d6610 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -137,6 +137,9 @@ endef install: all ifdef INSTALL_PATH $(INSTALL_RULE) + $(if $(TEST_SH_LIBS), \ + cd $(BASE_PATH) && rsync -aR $(TEST_SH_LIBS) $(SH_LIBS_PATH)/ \ + ) else $(error Error: set INSTALL_PATH to use install) endif diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile index df593b7b3e6b..4f6921638bae 100644 --- a/tools/testing/selftests/net/forwarding/Makefile +++ b/tools/testing/selftests/net/forwarding/Makefile @@ -128,4 +128,7 @@ TEST_PROGS_EXTENDED := devlink_lib.sh \ sch_tbf_etsprio.sh \ tc_common.sh +TEST_SH_LIBS := \ + net/lib.sh + include ../../lib.mk diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index 8f6ca458af9a..b99fae42e3c0 100755 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -38,7 +38,8 @@ if [[ -f $relative_path/forwarding.config ]]; then source "$relative_path/forwarding.config" fi -source ../lib.sh +libdir=$(dirname "$(readlink -f "$BASH_SOURCE")") +source "$libdir"/../lib.sh ############################################################################## # Sanity checks
On Wed, Dec 13, 2023 at 11:31:50AM +0100, Petr Machata wrote: > Hmm, maybe we could side-step the issue? I suspect that vast majority of > what bonding uses are just generic helpers. log_test, check_err, that > sort of stuff. Unless I missed something, all of them set NUM_NETIFS=0. > Those things could all be in the generic net/lib.sh. So long-term it > might be possible for bonding to do the trick with symlinking, except > with just net/lib.sh, not both libs. > > I think that most of forwarding/lib.sh actually belongs to net/lib.sh. > We reinvent a lot of that functionality in various net/ tests, because, > presumably, people find it odd to source forwarding/lib.sh. If it all > lived in net/, we could reuse all these tools instead of cut'n'pasting > them from one test to the other. Stuff like the mcast_packet_test, > start/stop_traffic, etc., would probably stay in forwarding. > > So that's long-term. And short-term we can live with the ugly-as-sin > workaround that I propose? For bonding only I think this is a good resolution. For other driver tests that may still need to include forwarding/lib.sh. I think the way Benjamin suggested is a good choice. Thanks Hangbin
Hi Benjamin, On Wed, Dec 13, 2023 at 04:40:53PM -0500, Benjamin Poirier wrote: > > Hmm.. Is it possible to write a rule in the Makefile to create the net/ > > and net/forwarding folder so we can source the relative path directly. e.g. > > > > ]# tree > > . > > ├── drivers > > │ └── net > > │ └── bonding > > │ ├── bond-arp-interval-causes-panic.sh > > │ ├── ... > > │ └── settings > > ├── kselftest > > │ ├── module.sh > > │ ├── prefix.pl > > │ └── runner.sh > > ├── kselftest-list.txt > > ├── net > > │ ├── forwarding > > │ │ └── lib.sh > > │ └── lib.sh > > └── run_kselftest.sh > > That sounds like a good idea. I started to work on that approach but I'm > missing recursive inclusion. For instance > > cd tools/testing/selftests > make install TARGETS="drivers/net/bonding" > ./kselftest_install/run_kselftest.sh -t drivers/net/bonding:dev_addr_lists.sh > > includes net/forwarding/lib.sh but is missing net/lib.sh. I feel that my > 'make' skills are rusty but I guess that with enough make code, it could > be done. A workaround is simply to manually list the transitive > dependencies in TEST_SH_LIBS: > TEST_SH_LIBS := \ > - net/forwarding/lib.sh > + net/forwarding/lib.sh \ > + net/lib.sh Yes, this makes the user need to make sure all the recursive inclusions listed here. A little inconvenient. But I "make" skill is worse than you... > > I only converted a few files to validate that the approach is viable. I > used the following tests: > drivers/net/bonding/dev_addr_lists.sh > net/test_vxlan_vnifiltering.sh > net/forwarding/pedit_ip.sh > > Let me know what you think. Thanks! This works for me. Cheers Hangbin
On 2023-12-14 15:06 +0800, Hangbin Liu wrote: > Hi Benjamin, > > On Wed, Dec 13, 2023 at 04:40:53PM -0500, Benjamin Poirier wrote: > > > Hmm.. Is it possible to write a rule in the Makefile to create the net/ > > > and net/forwarding folder so we can source the relative path directly. e.g. > > > > > > ]# tree > > > . > > > ├── drivers > > > │ └── net > > > │ └── bonding > > > │ ├── bond-arp-interval-causes-panic.sh > > > │ ├── ... > > > │ └── settings > > > ├── kselftest > > > │ ├── module.sh > > > │ ├── prefix.pl > > > │ └── runner.sh > > > ├── kselftest-list.txt > > > ├── net > > > │ ├── forwarding > > > │ │ └── lib.sh > > > │ └── lib.sh > > > └── run_kselftest.sh > > > > That sounds like a good idea. I started to work on that approach but I'm > > missing recursive inclusion. For instance > > > > cd tools/testing/selftests > > make install TARGETS="drivers/net/bonding" > > ./kselftest_install/run_kselftest.sh -t drivers/net/bonding:dev_addr_lists.sh > > > > includes net/forwarding/lib.sh but is missing net/lib.sh. I feel that my > > 'make' skills are rusty but I guess that with enough make code, it could > > be done. A workaround is simply to manually list the transitive > > dependencies in TEST_SH_LIBS: > > TEST_SH_LIBS := \ > > - net/forwarding/lib.sh > > + net/forwarding/lib.sh \ > > + net/lib.sh > > Yes, this makes the user need to make sure all the recursive inclusions listed > here. A little inconvenient. But I "make" skill is worse than you... > > > > > I only converted a few files to validate that the approach is viable. I > > used the following tests: > > drivers/net/bonding/dev_addr_lists.sh > > net/test_vxlan_vnifiltering.sh > > net/forwarding/pedit_ip.sh > > > > Let me know what you think. > > Thanks! This works for me. I started to make the adjustments to all the tests but I got stuck on the dsa tests. The problem is that the tests which are symlinked (like bridge_locked_port.sh) expect to source lib.sh (net/forwarding/lib.sh) from the same directory. That lib.sh then expects to source net/lib.sh from the parent directory. Because `rsync --copy-unsafe-links` is used, all those links become regular files after export so we can't rely on `readlink -f`. Honestly, given how the dsa tests are organized, I don't see a clean way to support these tests without error after commit 25ae948b4478 ("selftests/net: add lib.sh"). The only way that I see to run these tests via runner.sh is by using a command like: make -C tools/testing/selftests install TARGETS="drivers/net/dsa" KSELFTEST_BRIDGE_LOCKED_PORT_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_BRIDGE_MDB_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_BRIDGE_MLD_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_BRIDGE_VLAN_AWARE_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_BRIDGE_VLAN_MCAST_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_BRIDGE_VLAN_UNAWARE_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_LOCAL_TERMINATION_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_NO_FORWARDING_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_TC_ACTIONS_SH_ARGS="swp1 swp2 swp3 swp4" KSELFTEST_TEST_BRIDGE_FDB_STRESS_SH_ARGS="swp1 swp2 swp3 swp4" tools/testing/selftests/kselftest_install/run_kselftest.sh This is very cumbersome so it makes me question the value of drivers/net/dsa/Makefile. Patrice, Vladimir, Martin, how do you run the dsa tests? Could we revert 6ecf206d602f ("selftests: net: dsa: Add a Makefile which installs the selftests")? Do you have other suggestions to avoid the following error about lib.sh: tools/testing/selftests# make install TARGETS="drivers/net/dsa" [...] tools/testing/selftests# ./kselftest_install/run_kselftest.sh TAP version 13 1..10 # timeout set to 45 # selftests: drivers/net/dsa: bridge_locked_port.sh # lib.sh: line 41: ../lib.sh: No such file or directory [...]
On Thu, Dec 14, 2023 at 05:00:31PM -0500, Benjamin Poirier wrote: > I started to make the adjustments to all the tests but I got stuck on > the dsa tests. The problem is that the tests which are symlinked (like > bridge_locked_port.sh) expect to source lib.sh (net/forwarding/lib.sh) > from the same directory. That lib.sh then expects to source net/lib.sh > from the parent directory. Because `rsync --copy-unsafe-links` is used, > all those links become regular files after export so we can't rely on > `readlink -f`. > > Honestly, given how the dsa tests are organized, I don't see a clean way > to support these tests without error after commit 25ae948b4478 > ("selftests/net: add lib.sh"). No worry, the last way is just make net/forwarding/lib.sh not source net/lib.sh :) Although this would make us copy a lot functions from net/forwarding/lib.sh to source net/lib.sh. So before that, let's try if we can Move all the dsa tests back to net/forwarding (actually only needed for test_bridge_fdb_stress.sh). And add a forwarding.config.dsa.sample. Maybe a test list for dsa testing. But with this way the dsa testing will not able to run via run_kselftest.sh. Or, we can remove the symlinks, and add the dsa tests with exec the relative forwarding path's tests directly. e.g. ### in das test folder $ cat bridge_mld.sh #!/bin/bash ../../../net/forwarding/bridge_mld.sh $ cat Makefile # SPDX-License-Identifier: GPL-2.0+ OR MIT TEST_PROGS = \ bridge_mld.sh TEST_SH_LIBS := \ net/lib.sh \ net/forwarding/lib.sh \ net/forwarding/bridge_mld.sh TEST_FILES := forwarding.config include ../../../lib.mk ### for net/forwarding/lib.sh looks we need source the ${PWD}/forwarding.config if run via run_kselftest.sh if [[ -f ${PWD}/forwarding.config ]]; then source "$PWD/forwarding.config" elif [[ -f $relative_path/forwarding.config ]]; then source "$relative_path/forwarding.config" fi What do you think? Thanks Hangbin
On 2023-12-15 10:35 +0800, Hangbin Liu wrote: > On Thu, Dec 14, 2023 at 05:00:31PM -0500, Benjamin Poirier wrote: > > I started to make the adjustments to all the tests but I got stuck on > > the dsa tests. The problem is that the tests which are symlinked (like > > bridge_locked_port.sh) expect to source lib.sh (net/forwarding/lib.sh) > > from the same directory. That lib.sh then expects to source net/lib.sh > > from the parent directory. Because `rsync --copy-unsafe-links` is used, > > all those links become regular files after export so we can't rely on > > `readlink -f`. > > > > Honestly, given how the dsa tests are organized, I don't see a clean way > > to support these tests without error after commit 25ae948b4478 > > ("selftests/net: add lib.sh"). > > No worry, the last way is just make net/forwarding/lib.sh not source net/lib.sh :) > Although this would make us copy a lot functions from net/forwarding/lib.sh to > source net/lib.sh. So before that, let's try if we can > > Move all the dsa tests back to net/forwarding (actually only needed for > test_bridge_fdb_stress.sh). And add a forwarding.config.dsa.sample. Maybe > a test list for dsa testing. But with this way the dsa testing will not > able to run via run_kselftest.sh. > > Or, we can remove the symlinks, and add the dsa tests with exec the relative > forwarding path's tests directly. e.g. > > ### in das test folder > > $ cat bridge_mld.sh > #!/bin/bash > ../../../net/forwarding/bridge_mld.sh > > $ cat Makefile > # SPDX-License-Identifier: GPL-2.0+ OR MIT > > TEST_PROGS = \ > bridge_mld.sh > > TEST_SH_LIBS := \ > net/lib.sh \ > net/forwarding/lib.sh \ > net/forwarding/bridge_mld.sh > > TEST_FILES := forwarding.config > > include ../../../lib.mk > > ### for net/forwarding/lib.sh looks we need source the ${PWD}/forwarding.config if run via run_kselftest.sh > > if [[ -f ${PWD}/forwarding.config ]]; then > source "$PWD/forwarding.config" > elif [[ -f $relative_path/forwarding.config ]]; then > source "$relative_path/forwarding.config" > fi > > What do you think? That's a good idea. Thank you for your suggestion. I made a few adjustment from what you show and the result seems to work so far. I'm working on the full patchset. If there is no problem, I'll post it next week.
On Thu, Dec 14, 2023 at 05:00:31PM -0500, Benjamin Poirier wrote:
> Patrice, Vladimir, Martin, how do you run the dsa tests?
Not how they're supposed to, apparently.
I used to rsync the "selftests" folder to the device under test, go
to the drivers/net/dsa directory, and ./ the test from there. That is
absolutely sufficient without all the "make kselftest" / run_kselftest.sh
overhead which I don't need, but apparently that broken now too.
I don't have a strong objection against eliminating the symlinks.
They were just a handy way of filtering those tests from net/forwarding/
which were relevant to DSA, and just ignore the test.
What might turn out to be problematic is DSA's forwarding.config, where
we actually rely on the STABLE_MAC_ADDRS option for some tests to pass.
I'm not actually sure what is the "recommended" way of deploying a
custom forwarding.config file anyway.
On 2023-12-21 18:58 +0200, Vladimir Oltean wrote: > On Thu, Dec 14, 2023 at 05:00:31PM -0500, Benjamin Poirier wrote: > > Patrice, Vladimir, Martin, how do you run the dsa tests? > > Not how they're supposed to, apparently. > > I used to rsync the "selftests" folder to the device under test, go > to the drivers/net/dsa directory, and ./ the test from there. That is > absolutely sufficient without all the "make kselftest" / run_kselftest.sh > overhead which I don't need, but apparently that broken now too. > > I don't have a strong objection against eliminating the symlinks. > They were just a handy way of filtering those tests from net/forwarding/ > which were relevant to DSA, and just ignore the test. > > What might turn out to be problematic is DSA's forwarding.config, where > we actually rely on the STABLE_MAC_ADDRS option for some tests to pass. > I'm not actually sure what is the "recommended" way of deploying a > custom forwarding.config file anyway. Thank you for sharing that info. There are so many different ways to run the selftests, there is no "one true way". Thanks to Hangbin's suggestion earlier in the thread I found a way to make things work for the dsa tests after commit 25ae948b4478 ("selftests/net: add lib.sh") by adding a wrapper script that sources another test; see the RFC that I just posted: https://lore.kernel.org/netdev/20231222135836.992841-1-bpoirier@nvidia.com/T/#t drivers/net/dsa will still have links to the net/forwarding/ tests so today's functionality should be preserved, including forwarding.config.
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index 038b7d956722..0feb4e400110 100755 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -38,7 +38,7 @@ if [[ -f $relative_path/forwarding.config ]]; then source "$relative_path/forwarding.config" fi -source ../lib.sh +source ${lib_dir-.}/../lib.sh ############################################################################## # Sanity checks
The commit cited below moved code from net/forwarding/lib.sh to net/lib.sh, and had the former source the latter. This causes issues with selftests from directories other than net/forwarding/, in particular drivers/net/... For those files, the line "source ../lib.sh" would look for lib.sh in the directory above the script file's one, which is incorrect. The way these selftests source net/forwarding/lib.sh is like this: lib_dir=$(dirname $0)/../../../net/forwarding source $lib_dir/lib.sh Reuse the variable lib_dir, if set, in net/forwarding/lib.sh to source net/lib.sh as well. Fixes: 25ae948b4478 ("selftests/net: add lib.sh") Cc: Hangbin Liu <liuhangbin@gmail.com> Signed-off-by: Petr Machata <petrm@nvidia.com> --- tools/testing/selftests/net/forwarding/lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)