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:
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,
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()
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.
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
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
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!
linux-kselftest-mirror@lists.linaro.org
-
Breno Leitao -
Jakub Kicinski -
patchwork-bot+netdevbpf＠kernel.org