[PATCH net-next 0/4] selftests: drv-net: round some sharp edges

[PATCH net-next 0/4] selftests: drv-net: round some sharp edges

[Jakub Kicinski]

I had to explain how to run the driver tests twice already.
Improve the README so we can just point to it.
Also fix a potential problem with the SSH remote sessions.

Jakub Kicinski (4):
  selftests: drv-net: force pseudo-terminal allocation in ssh
  selftests: drv-net: extend the README with more info and example
  selftests: drv-net: reimplement the config parser
  selftests: drv-net: validate the environment

 .../testing/selftests/drivers/net/README.rst  | 95 ++++++++++++++++---
 .../selftests/drivers/net/lib/py/env.py       | 46 ++++++---
 .../drivers/net/lib/py/remote_ssh.py          |  2 +-
 3 files changed, 118 insertions(+), 25 deletions(-)

-- 
2.44.0

[PATCH net-next 1/4] selftests: drv-net: force pseudo-terminal allocation in ssh

[Jakub Kicinski]

This is not yet needed, because we don't terminate remote background
commands. But once we do, if we run ssh without -t the ssh session
may close and the program may carry on happily running.

I have hit this problem experimenting with mausezahn, let's fix
it already to avoid someone else wasting time debugging it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/lib/py/remote_ssh.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py b/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
index 924addde19a3..294a4ed8284e 100644
--- a/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
+++ b/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
@@ -20,7 +20,7 @@ from lib.py import cmd
             self._tmpdir = None
 
     def cmd(self, comm):
-        return subprocess.Popen(["ssh", "-q", self.name, comm],
+        return subprocess.Popen(["ssh", "-t", "-q", self.name, comm],
                                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
     def _mktmp(self):
-- 
2.44.0

Re: [PATCH net-next 1/4] selftests: drv-net: force pseudo-terminal allocation in ssh

[Jakub Kicinski]

On Wed, 24 Apr 2024 15:14:41 -0700 Jakub Kicinski wrote:
> This is not yet needed, because we don't terminate remote background
> commands. But once we do, if we run ssh without -t the ssh session
> may close and the program may carry on happily running.
> 
> I have hit this problem experimenting with mausezahn, let's fix
> it already to avoid someone else wasting time debugging it.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/lib/py/remote_ssh.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py b/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
> index 924addde19a3..294a4ed8284e 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/remote_ssh.py
> @@ -20,7 +20,7 @@ from lib.py import cmd
>              self._tmpdir = None
>  
>      def cmd(self, comm):
> -        return subprocess.Popen(["ssh", "-q", self.name, comm],
> +        return subprocess.Popen(["ssh", "-t", "-q", self.name, comm],
>                                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)

This seems to mess up the local terminal. I guess we'll cross that
bridge when we get there... I'll drop this patch when applying.

[PATCH net-next 2/4] selftests: drv-net: extend the README with more info and example

[Jakub Kicinski]

Add more info to the README. It's also now copied to GitHub for
increased visibility:

 https://github.com/linux-netdev/nipa/wiki/Running-driver-tests

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../testing/selftests/drivers/net/README.rst  | 97 ++++++++++++++++---
 1 file changed, 85 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/README.rst b/tools/testing/selftests/drivers/net/README.rst
index 0cbab33dad1f..3b6a29e6564b 100644
--- a/tools/testing/selftests/drivers/net/README.rst
+++ b/tools/testing/selftests/drivers/net/README.rst
@@ -1,18 +1,42 @@
-Running tests
-=============
+.. SPDX-License-Identifier: GPL-2.0
 
-Tests are executed within kselftest framework like any other tests.
-By default tests execute against software drivers such as netdevsim.
-All tests must support running against a real device (SW-only tests
-should instead be placed in net/ or drivers/net/netdevsim, HW-only
-tests in drivers/net/hw).
+Running driver tests
+====================
 
-Set appropriate variables to point the tests at a real device.
+Networking driver tests are executed within kselftest framework like any
+other tests. They support testing both real device drivers and emulated /
+software drivers (latter mostly to test the core parts of the stack).
+
+SW mode
+~~~~~~~
+
+By default, when no extra parameters are set or exported, tests execute
+against software drivers such as netdevsim. No extra preparation is required
+the software devices are created and destroyed as part of the test.
+In this mode the tests are indistinguishable from other selftests and
+(for example) can be run under ``virtme-ng`` like the core networking selftests.
+
+HW mode
+~~~~~~~
+
+Executing tests against a real device requires external preparation.
+The netdevice against which tests will be run must exist, be running
+(in UP state) and be configured with an IP address.
+
+Refer to list of :ref:`Variables` later in this file to set up running
+the tests against a real device.
+
+Both modes required
+~~~~~~~~~~~~~~~~~~~
+
+All tests in drivers/net must support running both against a software device
+and a real device. SW-only tests should instead be placed in net/ or
+drivers/net/netdevsim, HW-only tests in drivers/net/hw.
 
 Variables
 =========
 
-Variables can be set in the environment or by creating a net.config
+The variables can be set in the environment or by creating a net.config
 file in the same directory as this README file. Example::
 
   $ NETIF=eth0 ./some_test.sh
@@ -23,9 +47,9 @@ Variables can be set in the environment or by creating a net.config
   # Variable set in a file
   NETIF=eth0
 
-Please note that the config parser is very simple, if there are
-any non-alphanumeric characters in the value it needs to be in
-double quotes.
+Local test (which don't require endpoint for sending / receiving traffic)
+need only the ``NETIF`` variable. Remaining variables define the endpoint
+and communication method.
 
 NETIF
 ~~~~~
@@ -61,3 +85,52 @@ Arguments used to construct the communication channel.
 
   for netns - name of the "remote" namespace
   for ssh - name/address of the remote host
+
+Example
+=======
+
+Build the selftests::
+
+  # make -C tools/testing/selftests/ TARGETS="drivers/net drivers/net/hw"
+
+"Install" the tests and copy them over to the target machine::
+
+  # make -C tools/testing/selftests/ TARGETS="drivers/net drivers/net/hw" \
+     install INSTALL_PATH=/tmp/ksft-net-drv
+
+  # rsync -ra --delete /tmp/ksft-net-drv root@192.168.1.1:/root/
+
+On the target machine, running the tests will use netdevsim by default::
+
+  [/root] # ./ksft-net-drv/run_kselftest.sh -t drivers/net:ping.py
+  TAP version 13
+  1..1
+  # timeout set to 45
+  # selftests: drivers/net: ping.py
+  # KTAP version 1
+  # 1..3
+  # ok 1 ping.test_v4
+  # ok 2 ping.test_v6
+  # ok 3 ping.test_tcp
+  # # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0
+  ok 1 selftests: drivers/net: ping.py
+
+Create a config with remote info::
+
+  [/root] # cat > ./ksft-net-drv/drivers/net/net.config <<EOF
+  NETIF=eth0
+  LOCAL_V4=192.168.1.1
+  REMOTE_V4=192.168.1.2
+  REMOTE_TYPE=ssh
+  REMOTE_ARGS=root@192.168.1.2
+  EOF
+
+Run the test::
+
+  [/root] # ./ksft-net-drv/drivers/net/ping.py
+  KTAP version 1
+  1..3
+  ok 1 ping.test_v4
+  ok 2 ping.test_v6 # SKIP Test requires IPv6 connectivity
+  ok 3 ping.test_tcp
+  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:1 error:0
-- 
2.44.0

[PATCH net-next 3/4] selftests: drv-net: reimplement the config parser

[Jakub Kicinski]

The shell lexer is not helping much, do very basic parsing
manually.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/lib/py/env.py       | 26 ++++++++++---------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index a3db1bb1afeb..a6a5a5f9c6db 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -1,7 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
 import os
-import shlex
 from pathlib import Path
 from lib.py import KsftSkipEx
 from lib.py import cmd, ip
@@ -16,17 +15,20 @@ from .remote import Remote
     if not (src_dir / "net.config").exists():
         return env
 
-    lexer = shlex.shlex(open((src_dir / "net.config").as_posix(), 'r').read())
-    k = None
-    for token in lexer:
-        if k is None:
-            k = token
-            env[k] = ""
-        elif token == "=":
-            pass
-        else:
-            env[k] = token
-            k = None
+    with open((src_dir / "net.config").as_posix(), 'r') as fp:
+        for line in fp.readlines():
+            full_file = line
+            # Strip comments
+            pos = line.find("#")
+            if pos >= 0:
+                line = line[:pos]
+            line = line.strip()
+            if not line:
+                continue
+            pos = line.find("=")
+            if pos <= 0:
+                raise Exception("Can't parse configuration line:", full_file)
+            env[line[:pos]] = line[pos+1:]
     return env
 
 
-- 
2.44.0

Re: [PATCH net-next 3/4] selftests: drv-net: reimplement the config parser

[Willem de Bruijn]

Jakub Kicinski wrote:
> The shell lexer is not helping much, do very basic parsing
> manually.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/lib/py/env.py       | 26 ++++++++++---------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index a3db1bb1afeb..a6a5a5f9c6db 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -1,7 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  import os
> -import shlex
>  from pathlib import Path
>  from lib.py import KsftSkipEx
>  from lib.py import cmd, ip
> @@ -16,17 +15,20 @@ from .remote import Remote
>      if not (src_dir / "net.config").exists():
>          return env
>  
> -    lexer = shlex.shlex(open((src_dir / "net.config").as_posix(), 'r').read())
> -    k = None
> -    for token in lexer:
> -        if k is None:
> -            k = token
> -            env[k] = ""
> -        elif token == "=":
> -            pass
> -        else:
> -            env[k] = token
> -            k = None
> +    with open((src_dir / "net.config").as_posix(), 'r') as fp:
> +        for line in fp.readlines():
> +            full_file = line
> +            # Strip comments
> +            pos = line.find("#")
> +            if pos >= 0:
> +                line = line[:pos]
> +            line = line.strip()
> +            if not line:
> +                continue
> +            pos = line.find("=")
> +            if pos <= 0:
> +                raise Exception("Can't parse configuration line:", full_file)
> +            env[line[:pos]] = line[pos+1:]

Or line.split. But bikeshedding python is a deep rabbit hole :)

[PATCH net-next 4/4] selftests: drv-net: validate the environment

[Jakub Kicinski]

Throw a slightly more helpful exception when env variables
are partially populated. Prior to this change we'd get
a dictionary key exception somewhere later on.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/lib/py/env.py       | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index a6a5a5f9c6db..fda4967503de 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -88,6 +88,7 @@ from .remote import Remote
         self._ns_peer = None
 
         if "NETIF" in self.env:
+            self._check_env()
             self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
 
             self.v4 = self.env.get("LOCAL_V4")
@@ -143,6 +144,25 @@ from .remote import Remote
         ip(f"-6 addr add dev {self._ns_peer.nsims[0].ifname} {self.nsim_v6_pfx}2/64 nodad", ns=self._netns)
         ip(f"   link set dev {self._ns_peer.nsims[0].ifname} up", ns=self._netns)
 
+    def _check_env(self):
+        vars_needed = [
+            ["LOCAL_V4", "LOCAL_V6"],
+            ["REMOTE_V4", "REMOTE_V6"],
+            ["REMOTE_TYPE"],
+            ["REMOTE_ARGS"]
+        ]
+        missing = []
+
+        for choice in vars_needed:
+            for entry in choice:
+                if entry in self.env:
+                    break
+            else:
+                missing.append(choice)
+        if missing:
+            raise Exception("Invalid environment, missing configuration:", missing,
+                            "Please see tools/testing/selftests/drivers/net/README.rst")
+
     def __enter__(self):
         return self
 
-- 
2.44.0

Re: [PATCH net-next 4/4] selftests: drv-net: validate the environment

[Willem de Bruijn]

Jakub Kicinski wrote:
> Throw a slightly more helpful exception when env variables
> are partially populated. Prior to this change we'd get
> a dictionary key exception somewhere later on.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/lib/py/env.py       | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index a6a5a5f9c6db..fda4967503de 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -88,6 +88,7 @@ from .remote import Remote
>          self._ns_peer = None
>  
>          if "NETIF" in self.env:
> +            self._check_env()
>              self.dev = ip("link show dev " + self.env['NETIF'], json=True)[0]
>  
>              self.v4 = self.env.get("LOCAL_V4")
> @@ -143,6 +144,25 @@ from .remote import Remote
>          ip(f"-6 addr add dev {self._ns_peer.nsims[0].ifname} {self.nsim_v6_pfx}2/64 nodad", ns=self._netns)
>          ip(f"   link set dev {self._ns_peer.nsims[0].ifname} up", ns=self._netns)
>  
> +    def _check_env(self):
> +        vars_needed = [
> +            ["LOCAL_V4", "LOCAL_V6"],
> +            ["REMOTE_V4", "REMOTE_V6"],
> +            ["REMOTE_TYPE"],
> +            ["REMOTE_ARGS"]
> +        ]
> +        missing = []
> +
> +        for choice in vars_needed:
> +            for entry in choice:
> +                if entry in self.env:
> +                    break
> +            else:
> +                missing.append(choice)
> +        if missing:
> +            raise Exception("Invalid environment, missing configuration:", missing,
> +                            "Please see tools/testing/selftests/drivers/net/README.rst")
> +

I suppose this could still reach the exception if a file contains
LOCAL_V4 and REMOTE_V6 or vice versa.

But this is best effort anyway.

Re: [PATCH net-next 0/4] selftests: drv-net: round some sharp edges

[Willem de Bruijn]

Jakub Kicinski wrote:
> I had to explain how to run the driver tests twice already.
> Improve the README so we can just point to it.
> Also fix a potential problem with the SSH remote sessions.
> 
> Jakub Kicinski (4):
>   selftests: drv-net: force pseudo-terminal allocation in ssh
>   selftests: drv-net: extend the README with more info and example
>   selftests: drv-net: reimplement the config parser
>   selftests: drv-net: validate the environment
> 
>  .../testing/selftests/drivers/net/README.rst  | 95 ++++++++++++++++---
>  .../selftests/drivers/net/lib/py/env.py       | 46 ++++++---
>  .../drivers/net/lib/py/remote_ssh.py          |  2 +-
>  3 files changed, 118 insertions(+), 25 deletions(-)

Left two minor points comments inline, but nothing important.
The clear documentation is really helpful, thanks.

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next 0/4] selftests: drv-net: round some sharp edges

[Jakub Kicinski]

On Wed, 24 Apr 2024 21:45:49 -0400 Willem de Bruijn wrote:
> Left two minor points comments inline, but nothing important.

I'll respin, apply your suggestions and drop the unfortunate patch.