[PATCH net-next 1/2] selftests: drv-net: adjust tests before defaulting to shell=False

[PATCH net-next 1/2] selftests: drv-net: adjust tests before defaulting to shell=False

[Jakub Kicinski]

Clean up tests which expect shell=True without explicitly passing
that param to cmd(). There seems to be only one such case, and
in fact it's better converted to a direct write.

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

diff --git a/tools/testing/selftests/drivers/net/napi_threaded.py b/tools/testing/selftests/drivers/net/napi_threaded.py
index ed66efa481b0..f4be72b2145a 100755
--- a/tools/testing/selftests/drivers/net/napi_threaded.py
+++ b/tools/testing/selftests/drivers/net/napi_threaded.py
@@ -24,7 +24,8 @@ from lib.py import cmd, defer, ethtool
 
 
 def _set_threaded_state(cfg, threaded) -> None:
-    cmd(f"echo {threaded} > /sys/class/net/{cfg.ifname}/threaded")
+    with open(f"/sys/class/net/{cfg.ifname}/threaded", "wb") as fp:
+        fp.write(str(threaded).encode('utf-8'))
 
 
 def _setup_deferred_cleanup(cfg) -> None:
-- 
2.51.0

[PATCH net-next 2/2] selftests: net: py: don't default to shell=True

[Jakub Kicinski]

Overhead of using shell=True is quite significant.
Micro-benchmark of running ethtool --help shows that
non-shell run is 2x faster.

Runtime of the XDP tests also shows improvement:
this patch: 2m34s 2m21s 2m18s 2m18s
    before:     2m54s 2m36s 2m34s

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/utils.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index b188cac49738..e7155b6db9a3 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -29,9 +29,12 @@ import time
     """
     Execute a command on local or remote host.
 
+    @shell defaults to false, and class will try to split @comm into a list
+    if it's a string with spaces.
+
     Use bkg() instead to run a command in the background.
     """
-    def __init__(self, comm, shell=True, fail=True, ns=None, background=False,
+    def __init__(self, comm, shell=None, fail=True, ns=None, background=False,
                  host=None, timeout=5, ksft_wait=None):
         if ns:
             comm = f'ip netns exec {ns} ' + comm
@@ -45,6 +48,10 @@ import time
         if host:
             self.proc = host.cmd(comm)
         else:
+            # If user doesn't explicitly request shell try to avoid it.
+            if shell is None and isinstance(comm, str) and ' ' in comm:
+                comm = comm.split()
+
             # ksft_wait lets us wait for the background process to fully start,
             # we pass an FD to the child process, and wait for it to write back.
             # Similarly term_fd tells child it's time to exit.
@@ -111,7 +118,7 @@ import time
 
         with bkg("my_binary", ksft_wait=5):
     """
-    def __init__(self, comm, shell=True, fail=None, ns=None, host=None,
+    def __init__(self, comm, shell=None, fail=None, ns=None, host=None,
                  exit_wait=False, ksft_wait=None):
         super().__init__(comm, background=True,
                          shell=shell, fail=fail, ns=ns, host=host,
-- 
2.51.0

Re: [PATCH net-next 2/2] selftests: net: py: don't default to shell=True

[Breno Leitao]

On Sat, Aug 30, 2025 at 11:43:17AM -0700, Jakub Kicinski wrote:
> @@ -45,6 +48,10 @@ import time
>          if host:
>              self.proc = host.cmd(comm)
>          else:
> +            # If user doesn't explicitly request shell try to avoid it.
> +            if shell is None and isinstance(comm, str) and ' ' in comm:
> +                comm = comm.split()

I am wondering if you can always split the string, independently if
shell is True or now. Passing comm as a list is usually recommend, even
when shell is enabled. Also, if there is no space, split() will return
the same string.

What about something as?

	if isinstance(comm, str):
		comm = comm.split()

Re: [PATCH net-next 2/2] selftests: net: py: don't default to shell=True

[Jakub Kicinski]

On Mon, 1 Sep 2025 02:34:05 -0700 Breno Leitao wrote:
> On Sat, Aug 30, 2025 at 11:43:17AM -0700, Jakub Kicinski wrote:
> > @@ -45,6 +48,10 @@ import time
> >          if host:
> >              self.proc = host.cmd(comm)
> >          else:
> > +            # If user doesn't explicitly request shell try to avoid it.
> > +            if shell is None and isinstance(comm, str) and ' ' in comm:
> > +                comm = comm.split()  
> 
> I am wondering if you can always split the string, independently if
> shell is True or now. Passing comm as a list is usually recommend, even
> when shell is enabled. Also, if there is no space, split() will return
> the same string.

Not sure how that'll interact with various shells..
I'd rather play it safe.

Re: [PATCH net-next 2/2] selftests: net: py: don't default to shell=True

[Breno Leitao]

On Sat, Aug 30, 2025 at 11:43:17AM -0700, Jakub Kicinski wrote:
> Overhead of using shell=True is quite significant.
> Micro-benchmark of running ethtool --help shows that
> non-shell run is 2x faster.
> 
> Runtime of the XDP tests also shows improvement:
> this patch: 2m34s 2m21s 2m18s 2m18s
>     before:     2m54s 2m36s 2m34s
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net-next 1/2] selftests: drv-net: adjust tests before defaulting to shell=False

[Breno Leitao]

On Sat, Aug 30, 2025 at 11:43:16AM -0700, Jakub Kicinski wrote:
> Clean up tests which expect shell=True without explicitly passing
> that param to cmd(). There seems to be only one such case, and
> in fact it's better converted to a direct write.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net-next 1/2] selftests: drv-net: adjust tests before defaulting to shell=False

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 30 Aug 2025 11:43:16 -0700 you wrote:
> Clean up tests which expect shell=True without explicitly passing
> that param to cmd(). There seems to be only one such case, and
> in fact it's better converted to a direct write.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/napi_threaded.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Here is the summary with links:
  - [net-next,1/2] selftests: drv-net: adjust tests before defaulting to shell=False
    https://git.kernel.org/netdev/net-next/c/c2e5108649ab
  - [net-next,2/2] selftests: net: py: don't default to shell=True
    https://git.kernel.org/netdev/net-next/c/bc1a767f695d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html