* [PATCH] kunit: tool: stop using a shell to run kernel under QEMU
@ 2022-04-20 20:30 Daniel Latypov
2022-05-11 23:22 ` Brendan Higgins
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Latypov @ 2022-04-20 20:30 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
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
--
2.36.0.rc0.470.gd361397f0d-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: stop using a shell to run kernel under QEMU
2022-04-20 20:30 [PATCH] kunit: tool: stop using a shell to run kernel under QEMU Daniel Latypov
@ 2022-05-11 23:22 ` Brendan Higgins
2022-05-12 14:28 ` Daniel Latypov
0 siblings, 1 reply; 3+ messages in thread
From: Brendan Higgins @ 2022-05-11 23:22 UTC (permalink / raw)
To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan
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_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'])
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: stop using a shell to run kernel under QEMU
2022-05-11 23:22 ` Brendan Higgins
@ 2022-05-12 14:28 ` Daniel Latypov
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Latypov @ 2022-05-12 14:28 UTC (permalink / raw)
To: Brendan Higgins; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan
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@google.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-12 14:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 20:30 [PATCH] kunit: tool: stop using a shell to run kernel under QEMU Daniel Latypov
2022-05-11 23:22 ` Brendan Higgins
2022-05-12 14:28 ` Daniel Latypov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.