Note: this potentially breaks custom qemu_configs if people are using them! But the fix for them is simple, don't specify multiple arguments in one string and don't add on a redundant ''.
It feels a bit iffy to be using a shell in the first place.
There's the usual shenanigans where people could pass in arbitrary shell commands via --kernel_arg (since we're just adding '' around the kernel_cmdline) or via a custom qemu_config. This isn't too much of a concern given the nature of this script (and the qemu_config file is in python, you can do w/e you want already).
But it does have some other drawbacks.
One example of a kunit-specific pain point: If the relevant qemu binary is missing, we get output like this:
/bin/sh: line 1: qemu-system-aarch64: command not found
This in turn results in our KTAP parser complaining about missing/invalid KTAP, but we don't directly show the error! It's even more annoying to debug when you consider --raw_output only shows KUnit output by default, i.e. you need --raw_output=all to see it.
Whereas directly invoking the binary, Python will raise a FileNotFoundError for us, which is a noisier but more clear.
Making this change requires * splitting parameters like ['-m 256'] into ['-m', '256'] in kunit/qemu_configs/*.py * change [''] to [] in kunit/qemu_configs/*.py since otherwise QEMU fails w/ 'Device needs media, but drive is empty' * dropping explicit quoting of the kernel cmdline * using shlex.quote() when we print what command we're running so the user can copy-paste and run it
Signed-off-by: Daniel Latypov dlatypov@google.com --- tools/testing/kunit/kunit_kernel.py | 18 ++++++++++-------- tools/testing/kunit/qemu_configs/alpha.py | 2 +- tools/testing/kunit/qemu_configs/arm.py | 2 +- tools/testing/kunit/qemu_configs/arm64.py | 2 +- tools/testing/kunit/qemu_configs/i386.py | 2 +- tools/testing/kunit/qemu_configs/powerpc.py | 2 +- tools/testing/kunit/qemu_configs/riscv.py | 6 +++--- tools/testing/kunit/qemu_configs/s390.py | 4 ++-- tools/testing/kunit/qemu_configs/sparc.py | 2 +- tools/testing/kunit/qemu_configs/x86_64.py | 2 +- 10 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 483f78e15ce9..1b9c4922a675 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -11,6 +11,7 @@ import importlib.util import logging import subprocess import os +import shlex import shutil import signal import threading @@ -118,16 +119,17 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): '-nodefaults', '-m', '1024', '-kernel', kernel_path, - '-append', ''' + ' '.join(params + [self._kernel_command_line]) + ''', + '-append', ' '.join(params + [self._kernel_command_line]), '-no-reboot', '-nographic', - '-serial stdio'] + self._extra_qemu_params - print('Running tests with:\n$', ' '.join(qemu_command)) - return subprocess.Popen(' '.join(qemu_command), - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, shell=True, errors='backslashreplace') + '-serial', 'stdio'] + self._extra_qemu_params + # Note: shlex.join() does what we want, but requires python 3.8+. + print('Running tests with:\n$', ' '.join(shlex.quote(arg) for arg in qemu_command)) + return subprocess.Popen(qemu_command, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, errors='backslashreplace')
class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): """An abstraction over command line operations performed on a source tree.""" diff --git a/tools/testing/kunit/qemu_configs/alpha.py b/tools/testing/kunit/qemu_configs/alpha.py index 5d0c0cff03bd..3ac846e03a6b 100644 --- a/tools/testing/kunit/qemu_configs/alpha.py +++ b/tools/testing/kunit/qemu_configs/alpha.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='alpha', kernel_path='arch/alpha/boot/vmlinux', kernel_command_line='console=ttyS0', - extra_qemu_params=['']) + extra_qemu_params=[]) diff --git a/tools/testing/kunit/qemu_configs/arm.py b/tools/testing/kunit/qemu_configs/arm.py index b9c2a35e0296..db2160200566 100644 --- a/tools/testing/kunit/qemu_configs/arm.py +++ b/tools/testing/kunit/qemu_configs/arm.py @@ -10,4 +10,4 @@ CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''', qemu_arch='arm', kernel_path='arch/arm/boot/zImage', kernel_command_line='console=ttyAMA0', - extra_qemu_params=['-machine virt']) + extra_qemu_params=['-machine', 'virt']) diff --git a/tools/testing/kunit/qemu_configs/arm64.py b/tools/testing/kunit/qemu_configs/arm64.py index 517c04459f47..67d04064f785 100644 --- a/tools/testing/kunit/qemu_configs/arm64.py +++ b/tools/testing/kunit/qemu_configs/arm64.py @@ -9,4 +9,4 @@ CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''', qemu_arch='aarch64', kernel_path='arch/arm64/boot/Image.gz', kernel_command_line='console=ttyAMA0', - extra_qemu_params=['-machine virt', '-cpu cortex-a57']) + extra_qemu_params=['-machine', 'virt', '-cpu', 'cortex-a57']) diff --git a/tools/testing/kunit/qemu_configs/i386.py b/tools/testing/kunit/qemu_configs/i386.py index aed3ffd3937d..52b80be40e4b 100644 --- a/tools/testing/kunit/qemu_configs/i386.py +++ b/tools/testing/kunit/qemu_configs/i386.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='x86_64', kernel_path='arch/x86/boot/bzImage', kernel_command_line='console=ttyS0', - extra_qemu_params=['']) + extra_qemu_params=[]) diff --git a/tools/testing/kunit/qemu_configs/powerpc.py b/tools/testing/kunit/qemu_configs/powerpc.py index 35e9de24f0db..6c901149726b 100644 --- a/tools/testing/kunit/qemu_configs/powerpc.py +++ b/tools/testing/kunit/qemu_configs/powerpc.py @@ -9,4 +9,4 @@ CONFIG_HVC_CONSOLE=y''', qemu_arch='ppc64', kernel_path='vmlinux', kernel_command_line='console=ttyS0', - extra_qemu_params=['-M pseries', '-cpu power8']) + extra_qemu_params=['-M', ' pseries', '-cpu', 'power8']) diff --git a/tools/testing/kunit/qemu_configs/riscv.py b/tools/testing/kunit/qemu_configs/riscv.py index 9e528087cd7c..b882fde39435 100644 --- a/tools/testing/kunit/qemu_configs/riscv.py +++ b/tools/testing/kunit/qemu_configs/riscv.py @@ -26,6 +26,6 @@ CONFIG_SERIAL_EARLYCON_RISCV_SBI=y''', kernel_path='arch/riscv/boot/Image', kernel_command_line='console=ttyS0', extra_qemu_params=[ - '-machine virt', - '-cpu rv64', - '-bios opensbi-riscv64-generic-fw_dynamic.bin']) + '-machine', 'virt', + '-cpu', 'rv64', + '-bios', 'opensbi-riscv64-generic-fw_dynamic.bin']) diff --git a/tools/testing/kunit/qemu_configs/s390.py b/tools/testing/kunit/qemu_configs/s390.py index e310bd521113..98fa4fb60c0a 100644 --- a/tools/testing/kunit/qemu_configs/s390.py +++ b/tools/testing/kunit/qemu_configs/s390.py @@ -10,5 +10,5 @@ CONFIG_MODULES=y''', kernel_path='arch/s390/boot/bzImage', kernel_command_line='console=ttyS0', extra_qemu_params=[ - '-machine s390-ccw-virtio', - '-cpu qemu',]) + '-machine', 's390-ccw-virtio', + '-cpu', 'qemu',]) diff --git a/tools/testing/kunit/qemu_configs/sparc.py b/tools/testing/kunit/qemu_configs/sparc.py index 27f474e7ad6e..e975c4331a7c 100644 --- a/tools/testing/kunit/qemu_configs/sparc.py +++ b/tools/testing/kunit/qemu_configs/sparc.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='sparc', kernel_path='arch/sparc/boot/zImage', kernel_command_line='console=ttyS0 mem=256M', - extra_qemu_params=['-m 256']) + extra_qemu_params=['-m', '256']) diff --git a/tools/testing/kunit/qemu_configs/x86_64.py b/tools/testing/kunit/qemu_configs/x86_64.py index 77ab1aeee8a3..dc7949076863 100644 --- a/tools/testing/kunit/qemu_configs/x86_64.py +++ b/tools/testing/kunit/qemu_configs/x86_64.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='x86_64', kernel_path='arch/x86/boot/bzImage', kernel_command_line='console=ttyS0', - extra_qemu_params=['']) + extra_qemu_params=[])
base-commit: 59729170afcd4900e08997a482467ffda8d88c7f
On Wed, Apr 20, 2022 at 4:30 PM Daniel Latypov dlatypov@google.com wrote:
Note: this potentially breaks custom qemu_configs if people are using them! But the fix for them is simple, don't specify multiple arguments in one string and don't add on a redundant ''.
It feels a bit iffy to be using a shell in the first place.
There's the usual shenanigans where people could pass in arbitrary shell commands via --kernel_arg (since we're just adding '' around the kernel_cmdline) or via a custom qemu_config. This isn't too much of a concern given the nature of this script (and the qemu_config file is in python, you can do w/e you want already).
But it does have some other drawbacks.
One example of a kunit-specific pain point: If the relevant qemu binary is missing, we get output like this:
/bin/sh: line 1: qemu-system-aarch64: command not found
This in turn results in our KTAP parser complaining about missing/invalid KTAP, but we don't directly show the error! It's even more annoying to debug when you consider --raw_output only shows KUnit output by default, i.e. you need --raw_output=all to see it.
Whereas directly invoking the binary, Python will raise a FileNotFoundError for us, which is a noisier but more clear.
Making this change requires
- splitting parameters like ['-m 256'] into ['-m', '256'] in kunit/qemu_configs/*.py
- change [''] to [] in kunit/qemu_configs/*.py since otherwise QEMU fails w/ 'Device needs media, but drive is empty'
- dropping explicit quoting of the kernel cmdline
- using shlex.quote() when we print what command we're running so the user can copy-paste and run it
Signed-off-by: Daniel Latypov dlatypov@google.com
One issue I noticed below, otherwise:
Reviewed-by: Brendan Higgins brendanhiggins@google.com
tools/testing/kunit/kunit_kernel.py | 18 ++++++++++-------- tools/testing/kunit/qemu_configs/alpha.py | 2 +- tools/testing/kunit/qemu_configs/arm.py | 2 +- tools/testing/kunit/qemu_configs/arm64.py | 2 +- tools/testing/kunit/qemu_configs/i386.py | 2 +- tools/testing/kunit/qemu_configs/powerpc.py | 2 +- tools/testing/kunit/qemu_configs/riscv.py | 6 +++--- tools/testing/kunit/qemu_configs/s390.py | 4 ++-- tools/testing/kunit/qemu_configs/sparc.py | 2 +- tools/testing/kunit/qemu_configs/x86_64.py | 2 +- 10 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 483f78e15ce9..1b9c4922a675 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -11,6 +11,7 @@ import importlib.util import logging import subprocess import os +import shlex import shutil import signal import threading @@ -118,16 +119,17 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): '-nodefaults', '-m', '1024', '-kernel', kernel_path,
'-append', '\'' + ' '.join(params + [self._kernel_command_line]) + '\'',
'-append', ' '.join(params + [self._kernel_command_line]), '-no-reboot', '-nographic',
'-serial stdio'] + self._extra_qemu_paramsprint('Running tests with:\n$', ' '.join(qemu_command))return subprocess.Popen(' '.join(qemu_command),stdin=subprocess.PIPE,stdout=subprocess.PIPE,stderr=subprocess.STDOUT,text=True, shell=True, errors='backslashreplace')
'-serial', 'stdio'] + self._extra_qemu_params# Note: shlex.join() does what we want, but requires python 3.8+.print('Running tests with:\n$', ' '.join(shlex.quote(arg) for arg in qemu_command))return subprocess.Popen(qemu_command,stdin=subprocess.PIPE,stdout=subprocess.PIPE,stderr=subprocess.STDOUT,text=True, errors='backslashreplace')class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): """An abstraction over command line operations performed on a source tree.""" diff --git a/tools/testing/kunit/qemu_configs/alpha.py b/tools/testing/kunit/qemu_configs/alpha.py index 5d0c0cff03bd..3ac846e03a6b 100644 --- a/tools/testing/kunit/qemu_configs/alpha.py +++ b/tools/testing/kunit/qemu_configs/alpha.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='alpha', kernel_path='arch/alpha/boot/vmlinux', kernel_command_line='console=ttyS0',
extra_qemu_params=[''])
extra_qemu_params=[])diff --git a/tools/testing/kunit/qemu_configs/arm.py b/tools/testing/kunit/qemu_configs/arm.py index b9c2a35e0296..db2160200566 100644 --- a/tools/testing/kunit/qemu_configs/arm.py +++ b/tools/testing/kunit/qemu_configs/arm.py @@ -10,4 +10,4 @@ CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''', qemu_arch='arm', kernel_path='arch/arm/boot/zImage', kernel_command_line='console=ttyAMA0',
extra_qemu_params=['-machine virt'])
extra_qemu_params=['-machine', 'virt'])diff --git a/tools/testing/kunit/qemu_configs/arm64.py b/tools/testing/kunit/qemu_configs/arm64.py index 517c04459f47..67d04064f785 100644 --- a/tools/testing/kunit/qemu_configs/arm64.py +++ b/tools/testing/kunit/qemu_configs/arm64.py @@ -9,4 +9,4 @@ CONFIG_SERIAL_AMBA_PL011_CONSOLE=y''', qemu_arch='aarch64', kernel_path='arch/arm64/boot/Image.gz', kernel_command_line='console=ttyAMA0',
extra_qemu_params=['-machine virt', '-cpu cortex-a57'])
extra_qemu_params=['-machine', 'virt', '-cpu', 'cortex-a57'])diff --git a/tools/testing/kunit/qemu_configs/i386.py b/tools/testing/kunit/qemu_configs/i386.py index aed3ffd3937d..52b80be40e4b 100644 --- a/tools/testing/kunit/qemu_configs/i386.py +++ b/tools/testing/kunit/qemu_configs/i386.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='x86_64', kernel_path='arch/x86/boot/bzImage', kernel_command_line='console=ttyS0',
extra_qemu_params=[''])
extra_qemu_params=[])diff --git a/tools/testing/kunit/qemu_configs/powerpc.py b/tools/testing/kunit/qemu_configs/powerpc.py index 35e9de24f0db..6c901149726b 100644 --- a/tools/testing/kunit/qemu_configs/powerpc.py +++ b/tools/testing/kunit/qemu_configs/powerpc.py @@ -9,4 +9,4 @@ CONFIG_HVC_CONSOLE=y''', qemu_arch='ppc64', kernel_path='vmlinux', kernel_command_line='console=ttyS0',
extra_qemu_params=['-M pseries', '-cpu power8'])
extra_qemu_params=['-M', ' pseries', '-cpu', 'power8'])
This should be '-M', 'pseries', not '-M', ' pseries', (you left an extra space in.
Note this actually breaks QEMU on my machine.
diff --git a/tools/testing/kunit/qemu_configs/riscv.py b/tools/testing/kunit/qemu_configs/riscv.py index 9e528087cd7c..b882fde39435 100644 --- a/tools/testing/kunit/qemu_configs/riscv.py +++ b/tools/testing/kunit/qemu_configs/riscv.py @@ -26,6 +26,6 @@ CONFIG_SERIAL_EARLYCON_RISCV_SBI=y''', kernel_path='arch/riscv/boot/Image', kernel_command_line='console=ttyS0', extra_qemu_params=[
'-machine virt','-cpu rv64','-bios opensbi-riscv64-generic-fw_dynamic.bin'])
'-machine', 'virt','-cpu', 'rv64','-bios', 'opensbi-riscv64-generic-fw_dynamic.bin'])diff --git a/tools/testing/kunit/qemu_configs/s390.py b/tools/testing/kunit/qemu_configs/s390.py index e310bd521113..98fa4fb60c0a 100644 --- a/tools/testing/kunit/qemu_configs/s390.py +++ b/tools/testing/kunit/qemu_configs/s390.py @@ -10,5 +10,5 @@ CONFIG_MODULES=y''', kernel_path='arch/s390/boot/bzImage', kernel_command_line='console=ttyS0', extra_qemu_params=[
'-machine s390-ccw-virtio','-cpu qemu',])
'-machine', 's390-ccw-virtio','-cpu', 'qemu',])diff --git a/tools/testing/kunit/qemu_configs/sparc.py b/tools/testing/kunit/qemu_configs/sparc.py index 27f474e7ad6e..e975c4331a7c 100644 --- a/tools/testing/kunit/qemu_configs/sparc.py +++ b/tools/testing/kunit/qemu_configs/sparc.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='sparc', kernel_path='arch/sparc/boot/zImage', kernel_command_line='console=ttyS0 mem=256M',
extra_qemu_params=['-m 256'])
extra_qemu_params=['-m', '256'])diff --git a/tools/testing/kunit/qemu_configs/x86_64.py b/tools/testing/kunit/qemu_configs/x86_64.py index 77ab1aeee8a3..dc7949076863 100644 --- a/tools/testing/kunit/qemu_configs/x86_64.py +++ b/tools/testing/kunit/qemu_configs/x86_64.py @@ -7,4 +7,4 @@ CONFIG_SERIAL_8250_CONSOLE=y''', qemu_arch='x86_64', kernel_path='arch/x86/boot/bzImage', kernel_command_line='console=ttyS0',
extra_qemu_params=[''])
extra_qemu_params=[])base-commit: 59729170afcd4900e08997a482467ffda8d88c7f
2.36.0.rc0.470.gd361397f0d-goog
On Wed, May 11, 2022 at 4:22 PM Brendan Higgins brendanhiggins@google.com wrote:
extra_qemu_params=[])diff --git a/tools/testing/kunit/qemu_configs/powerpc.py b/tools/testing/kunit/qemu_configs/powerpc.py index 35e9de24f0db..6c901149726b 100644 --- a/tools/testing/kunit/qemu_configs/powerpc.py +++ b/tools/testing/kunit/qemu_configs/powerpc.py @@ -9,4 +9,4 @@ CONFIG_HVC_CONSOLE=y''', qemu_arch='ppc64', kernel_path='vmlinux', kernel_command_line='console=ttyS0',
extra_qemu_params=['-M pseries', '-cpu power8'])
extra_qemu_params=['-M', ' pseries', '-cpu', 'power8'])This should be '-M', 'pseries', not '-M', ' pseries', (you left an extra space in.
Note this actually breaks QEMU on my machine.
Thanks for catching this!
Ugh, I must have sent out a stale version of the .patch file. My tree has the fix since I noticed it right as I was preparing to send this out.
I have this locally: - extra_qemu_params=['-M pseries', '-cpu power8']) + extra_qemu_params=['-M', 'pseries', '-cpu', 'power8'])
Sent out as v2, https://lore.kernel.org/linux-kselftest/20220512142555.3554396-1-dlatypov@go...
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov