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

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

[Petr Machata]

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
 
-- 
2.41.0

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

[Hangbin Liu]

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

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

[Petr Machata]

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.

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

[Benjamin Poirier]

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

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

[Hangbin Liu]

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

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

[Benjamin Poirier]

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
 
-- 
2.43.0

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

[Hangbin Liu]

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

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

[Benjamin Poirier]

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

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

[Hangbin Liu]

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

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

[Benjamin Poirier]

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.

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

[Vladimir Oltean]

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.

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

[Benjamin Poirier]

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.

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

[Petr Machata]

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?

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

[Petr Machata]

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?

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

[Hangbin Liu]

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