Re: [PATCH net-next] selftests: forwarding: Import top-level lib.sh through $lib_dir

[Benjamin Poirier <benjamin.poirier@gmail.com>]

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
[...]