From 664844373c2dc33ff96af0368468cf9c9d7a65ee Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:24:50 -0700 Subject: [PATCH 01/10] Silence flake8 E251 warnings Due to what appears to be a yapf bug. boot-qemu.py:60:14: E251 unexpected spaces around keyword / parameter equals boot-qemu.py:68:14: E251 unexpected spaces around keyword / parameter equals boot-qemu.py:74:14: E251 unexpected spaces around keyword / parameter equals boot-qemu.py:81:14: E251 unexpected spaces around keyword / parameter equals boot-uml.py:25:14: E251 unexpected spaces around keyword / parameter equals boot-uml.py:33:14: E251 unexpected spaces around keyword / parameter equals Link: https://github.com/google/yapf/issues/393 Signed-off-by: Nathan Chancellor --- boot-qemu.py | 8 ++++---- boot-uml.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 43ef6e5..ee4a870 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -54,7 +54,7 @@ def parse_arguments(): "--interactive", "--shell", action="store_true", - help= + help= # noqa: E251 "Instead of immediately shutting down the machine upon successful boot, pass 'rdinit=/bin/sh' on the kernel command line to allow interacting with the machine via a shell." ) parser.add_argument( @@ -62,20 +62,20 @@ def parse_arguments(): "--kernel-location", required=True, type=str, - help= + help= # noqa: E251 "Path to kernel image or kernel build folder to search for image in. Can be an absolute or relative path." ) parser.add_argument( "--no-kvm", action="store_true", - help= + help= # noqa: E251 "Do not use KVM for acceleration even when supported (only recommended for debugging)." ) parser.add_argument( "-s", "--smp", type=int, - help= + help= # noqa: E251 "Number of processors for virtual machine. By default, only machines spawned with KVM will use multiple vCPUS." ) parser.add_argument( diff --git a/boot-uml.py b/boot-uml.py index 0169644..b624fba 100755 --- a/boot-uml.py +++ b/boot-uml.py @@ -22,7 +22,7 @@ def parse_arguments(): "-i", "--interactive", action="store_true", - help= + help= # noqa: E251 "Instead of immediately shutting down upon successful boot, pass 'init=/bin/sh' to the UML executable to allow interacting with UML via a shell." ) parser.add_argument( @@ -30,7 +30,7 @@ def parse_arguments(): "--kernel-location", required=True, type=str, - help= + help= # noqa: E251 "Path to UML executable ('linux') or kernel build folder to search for executable in. Can be an absolute or relative path." ) From 24133b31e142f29aa6f344217aef198764b1f67c Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:27:16 -0700 Subject: [PATCH 02/10] Use sys.exit() over exit() As recommended by pylint: boot-qemu.py:698:12: R1722: Consider using sys.exit() (consider-using-sys-exit) utils.py:29:4: R1722: Consider using sys.exit() (consider-using-sys-exit) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 3 ++- utils.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index ee4a870..39d9835 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -7,6 +7,7 @@ import re import shutil import subprocess +import sys import utils @@ -695,7 +696,7 @@ def launch_qemu(cfg): utils.red("ERROR: QEMU timed out!") else: utils.red("ERROR: QEMU did not exit cleanly!") - exit(ex.returncode) + sys.exit(ex.returncode) if __name__ == '__main__': diff --git a/utils.py b/utils.py index af2da94..c1be6dc 100755 --- a/utils.py +++ b/utils.py @@ -2,6 +2,7 @@ from pathlib import Path import shutil +import sys def check_cmd(cmd): @@ -26,7 +27,7 @@ def die(string): automatically. """ red(f"ERROR: {string}") - exit(1) + sys.exit(1) def get_full_kernel_path(kernel_location, image, arch=None): From a7bd42c3ce332571bde844644bb7a364bd64af3e Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:31:57 -0700 Subject: [PATCH 03/10] Use 'in' for certain comparisons Cleans up the following pylint warnings: utils.py:55:11: R1714: Consider merging these comparisons with 'in' by using 'image in ('vmlinux', 'linux')'. Use a set instead if elements are hashable. (consider-using-in) boot-qemu.py:124:19: R1714: Consider merging these comparisons with 'in' by using 'guest_arch in ('arm', 'arm32_v7')'. Use a set instead if elements are hashable. (consider-using-in) boot-qemu.py:404:9: R1714: Consider merging these comparisons with 'in' by using 'arch in ('arm', 'arm32_v7')'. Use a set instead if elements are hashable. (consider-using-in) boot-qemu.py:414:9: R1714: Consider merging these comparisons with 'in' by using 'arch in ('arm64', 'arm64be')'. Use a set instead if elements are hashable. (consider-using-in) boot-qemu.py:456:9: R1714: Consider merging these comparisons with 'in' by using 'arch in ('mips', 'mipsel')'. Use a set instead if elements are hashable. (consider-using-in) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 8 ++++---- utils.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 39d9835..84e1851 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -121,7 +121,7 @@ def can_use_kvm(can_test_for_kvm, guest_arch): if "arm64" in guest_arch: return True # 32-bit EL1 is not always supported, test for it first - if guest_arch == "arm" or guest_arch == "arm32_v7": + if guest_arch in ("arm", "arm32_v7"): check_32_bit_el1_exec = base_folder.joinpath( "utils", "aarch64_32_bit_el1_supported") check_32_bit_el1 = subprocess.run([check_32_bit_el1_exec]) @@ -401,7 +401,7 @@ def get_qemu_args(cfg): qemu = "qemu-system-arm" qemu_args += ["-machine", "romulus-bmc"] - elif arch == "arm" or arch == "arm32_v7": + elif arch in ("arm", "arm32_v7"): append += " console=ttyAMA0 earlycon" kernel_arch = "arm" qemu_args += ["-machine", "virt"] @@ -411,7 +411,7 @@ def get_qemu_args(cfg): else: qemu = "qemu-system-arm" - elif arch == "arm64" or arch == "arm64be": + elif arch in ("arm64", "arm64be"): append += " console=ttyAMA0 earlycon" kernel_arch = "arm64" kernel_image = "Image.gz" @@ -453,7 +453,7 @@ def get_qemu_args(cfg): qemu_args += ["-cpu", "m68040"] qemu_args += ["-M", "q800"] - elif arch == "mips" or arch == "mipsel": + elif arch in ("mips", "mipsel"): kernel_arch = "mips" kernel_image = "vmlinux" qemu = f"qemu-system-{arch}" diff --git a/utils.py b/utils.py index c1be6dc..b6d378b 100755 --- a/utils.py +++ b/utils.py @@ -52,7 +52,7 @@ def get_full_kernel_path(kernel_location, image, arch=None): else: # If the image is an uncompressed vmlinux or a UML image, it is in the # root of the build folder - if image == "vmlinux" or image == "linux": + if image in ("vmlinux", "linux"): kernel = kernel_location.joinpath(image) # Otherwise, it is in the architecture's boot directory else: From 8ebd7cb01f3f3d3f968fa8540e70b76f04e9dfe0 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:34:20 -0700 Subject: [PATCH 04/10] Use read_text() from pathlib in can_use_kvm() Helps compact code and eliminate the following pylint warnings: boot-qemu.py:132:21: W1514: Using open without explicitly specifying an encoding (unspecified-encoding) boot-qemu.py:132:46: C0103: Variable name "f" doesn't conform to snake_case naming style (invalid-name) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 84e1851..38e7570 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -129,8 +129,7 @@ def can_use_kvm(can_test_for_kvm, guest_arch): if host_arch == "x86_64" and "x86" in guest_arch: # Check /proc/cpuinfo for whether or not the machine supports hardware virtualization - with open("/proc/cpuinfo") as f: - cpuinfo = f.read() + cpuinfo = Path("/proc/cpuinfo").read_text(encoding='utf-8') # SVM is AMD, VMX is Intel return cpuinfo.count("svm") > 0 or cpuinfo.count("vmx") > 0 From 05b4f3b213c0d084cd2c05847ecba78e078de0e1 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:37:37 -0700 Subject: [PATCH 05/10] Update 'with open()' in get_smp_value() Elminate the following pylint warnings: boot-qemu.py:190:13: W1514: Using open without explicitly specifying an encoding (unspecified-encoding) boot-qemu.py:190:34: C0103: Variable name "f" doesn't conform to snake_case naming style (invalid-name) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 38e7570..be8f8e6 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -186,8 +186,8 @@ def get_smp_value(args): # CONFIG_NR_CPUS then get the actual value if possible. config_nr_cpus = 8 if config_file: - with open(config_file) as f: - for line in f: + with open(config_file, encoding='utf-8') as file: + for line in file: if "CONFIG_NR_CPUS=" in line: config_nr_cpus = int(line.split("=", 1)[1]) break From 8b5b27469ab0db53fef4a56292bdc532e10da0d3 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:45:07 -0700 Subject: [PATCH 06/10] Use Popen() in context manager in launch_qemu() As suggested by pylint: boot-qemu.py:666:27: R1732: Consider using 'with' for resource-allocating operations (consider-using-with) This allows us to drop the explicit call to wait(), as the exit path for Popen() as a context manager automatically calls wait(). Signed-off-by: Nathan Chancellor --- boot-qemu.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index be8f8e6..5a53868 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -663,18 +663,16 @@ def launch_qemu(cfg): utils.die("Port 1234 is already in use, is QEMU running?") utils.green("Starting QEMU with GDB connection on port 1234...") - qemu_process = subprocess.Popen(qemu_cmd + ["-s", "-S"]) - - utils.green("Starting GDB...") - utils.check_cmd(gdb_bin) - gdb_cmd = [gdb_bin] - gdb_cmd += [kernel_location.joinpath("vmlinux")] - gdb_cmd += ["-ex", "target remote :1234"] - subprocess.run(gdb_cmd) - - utils.red("Killing QEMU...") - qemu_process.kill() - qemu_process.wait() + with subprocess.Popen(qemu_cmd + ["-s", "-S"]) as qemu_process: + utils.green("Starting GDB...") + utils.check_cmd(gdb_bin) + gdb_cmd = [gdb_bin] + gdb_cmd += [kernel_location.joinpath("vmlinux")] + gdb_cmd += ["-ex", "target remote :1234"] + subprocess.run(gdb_cmd) + + utils.red("Killing QEMU...") + qemu_process.kill() answer = input("Re-run QEMU + gdb? [y/n] ") if answer.lower() == "n": From 58116c5ee5f3638ceb17ff50b657b0daaa7688f3 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 12:51:30 -0700 Subject: [PATCH 07/10] Add explicit check parameter to subprocess.run() calls An explicit check parameter is recommended to make it obvious what the error handling is. Cleans up the following pylint warnings: boot-qemu.py:127:39: W1510: Using subprocess.run without explicitly set `check` is not recommended. (subprocess-run-check) boot-qemu.py:659:19: W1510: Using subprocess.run without explicitly set `check` is not recommended. (subprocess-run-check) boot-qemu.py:672:16: W1510: Using subprocess.run without explicitly set `check` is not recommended. (subprocess-run-check) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 5a53868..8026d24 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -122,10 +122,13 @@ def can_use_kvm(can_test_for_kvm, guest_arch): return True # 32-bit EL1 is not always supported, test for it first if guest_arch in ("arm", "arm32_v7"): - check_32_bit_el1_exec = base_folder.joinpath( + check_32_bit_el1 = base_folder.joinpath( "utils", "aarch64_32_bit_el1_supported") - check_32_bit_el1 = subprocess.run([check_32_bit_el1_exec]) - return check_32_bit_el1.returncode == 0 + try: + subprocess.run([check_32_bit_el1], check=True) + except subprocess.CalledSubprocessError: + return False + return True if host_arch == "x86_64" and "x86" in guest_arch: # Check /proc/cpuinfo for whether or not the machine supports hardware virtualization @@ -658,7 +661,8 @@ def launch_qemu(cfg): utils.check_cmd("lsof") lsof = subprocess.run(["lsof", "-i:1234"], stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL) + stderr=subprocess.DEVNULL, + check=False) if lsof.returncode == 0: utils.die("Port 1234 is already in use, is QEMU running?") @@ -669,7 +673,7 @@ def launch_qemu(cfg): gdb_cmd = [gdb_bin] gdb_cmd += [kernel_location.joinpath("vmlinux")] gdb_cmd += ["-ex", "target remote :1234"] - subprocess.run(gdb_cmd) + subprocess.run(gdb_cmd, check=False) utils.red("Killing QEMU...") qemu_process.kill() From 6249ca49161d17cf5fbdd2bd4a3ab753691ccda9 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 13:00:54 -0700 Subject: [PATCH 08/10] Avoid redefining outer names As recommended by pylint: boot-uml.py:47:4: W0621: Redefining name 'rootfs' from outer scope (line 80) (redefined-outer-name) boot-uml.py:60:15: W0621: Redefining name 'kernel' from outer scope (line 79) (redefined-outer-name) boot-uml.py:60:23: W0621: Redefining name 'rootfs' from outer scope (line 80) (redefined-outer-name) boot-qemu.py:141:18: W0621: Redefining name 'args' from outer scope (line 703) (redefined-outer-name) boot-qemu.py:202:14: W0621: Redefining name 'args' from outer scope (line 703) (redefined-outer-name) boot-qemu.py:322:26: W0621: Redefining name 'cfg' from outer scope (line 706) (redefined-outer-name) boot-qemu.py:356:18: W0621: Redefining name 'cfg' from outer scope (line 706) (redefined-outer-name) boot-qemu.py:632:16: W0621: Redefining name 'cfg' from outer scope (line 706) (redefined-outer-name) As it turns out, this caught a bug in can_use_kvm(), where args was being used without being passed into the function. Signed-off-by: Nathan Chancellor --- boot-qemu.py | 9 ++++----- boot-uml.py | 9 ++++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/boot-qemu.py b/boot-qemu.py index 8026d24..957c508 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -113,7 +113,6 @@ def can_use_kvm(can_test_for_kvm, guest_arch): if can_test_for_kvm: # /dev/kvm must exist to use KVM with QEMU if Path("/dev/kvm").exists(): - guest_arch = args.architecture host_arch = platform.machine() if host_arch == "aarch64": @@ -701,10 +700,10 @@ def launch_qemu(cfg): if __name__ == '__main__': - args = parse_arguments() + arguments = parse_arguments() # Build configuration from arguments and QEMU flags - cfg = setup_cfg(args) - cfg = get_qemu_args(cfg) + config = setup_cfg(arguments) + config = get_qemu_args(config) - launch_qemu(cfg) + launch_qemu(config) diff --git a/boot-uml.py b/boot-uml.py index b624fba..08113b4 100755 --- a/boot-uml.py +++ b/boot-uml.py @@ -57,17 +57,17 @@ def decomp_rootfs(): return rootfs -def run_kernel(kernel, rootfs, interactive): +def run_kernel(kernel_image, rootfs, interactive): """ Run UML command with path to rootfs and additional arguments based on user input. Parameters: - * kernel (Path): kernel Path object containing full path to kernel. + * kernel_image (Path): kernel Path object containing full path to kernel. * rootfs (Path): rootfs Path object containing full path to rootfs. * interactive (bool): Whether or not to run UML interactively. """ - uml_cmd = [kernel, f"ubd0={rootfs}"] + uml_cmd = [kernel_image, f"ubd0={rootfs}"] if interactive: uml_cmd += ["init=/bin/sh"] print(f"$ {' '.join([str(element) for element in uml_cmd])}") @@ -77,6 +77,5 @@ def run_kernel(kernel, rootfs, interactive): if __name__ == '__main__': args = parse_arguments() kernel = utils.get_full_kernel_path(args.kernel_location, "linux") - rootfs = decomp_rootfs() - run_kernel(kernel, rootfs, args.interactive) + run_kernel(kernel, decomp_rootfs(), args.interactive) From 098b8c19fd77eba099101381111393ae3d29b5e9 Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Sat, 3 Dec 2022 19:53:22 -0700 Subject: [PATCH 09/10] Silence module name warnings These are scripts, not modules. boot-uml.py:1:0: C0103: Module name "boot-uml" doesn't conform to snake_case naming style (invalid-name) boot-qemu.py:1:0: C0103: Module name "boot-qemu" doesn't conform to snake_case naming style (invalid-name) Signed-off-by: Nathan Chancellor --- boot-qemu.py | 1 + boot-uml.py | 1 + 2 files changed, 2 insertions(+) diff --git a/boot-qemu.py b/boot-qemu.py index 957c508..7c99bbb 100755 --- a/boot-qemu.py +++ b/boot-qemu.py @@ -1,4 +1,5 @@ #!/usr/bin/env python3 +# pylint: disable=invalid-name import argparse import os diff --git a/boot-uml.py b/boot-uml.py index 08113b4..ff2860a 100755 --- a/boot-uml.py +++ b/boot-uml.py @@ -1,4 +1,5 @@ #!/usr/bin/env python3 +# pylint: disable=invalid-name import argparse from pathlib import Path From 3da9dd2b9413b729a2c7232835a38cddc827287f Mon Sep 17 00:00:00 2001 From: Nathan Chancellor Date: Wed, 7 Dec 2022 12:50:05 -0700 Subject: [PATCH 10/10] github workflows: Switch to new Python linting workflow Signed-off-by: Nathan Chancellor --- .github/workflows/main.yml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f179c51..3dc9e77 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,10 +1,16 @@ -# Run shellcheck and shfmt on all shell files and yapf on all Python files in this repository +# Run shellcheck and shfmt on all shell files and several Python linters on all Python files in this repository name: Lint checks on: [push, pull_request] jobs: + python: + strategy: + fail-fast: false + matrix: + version: ['3.11', '3.10', '3.9', '3.8'] + uses: ClangBuiltLinux/actions-workflows/.github/workflows/python_lint.yml@main + with: + python_version: ${{ matrix.version }} shellcheck: uses: ClangBuiltLinux/actions-workflows/.github/workflows/shellcheck.yml@main shfmt: uses: ClangBuiltLinux/actions-workflows/.github/workflows/shfmt.yml@main - yapf: - uses: ClangBuiltLinux/actions-workflows/.github/workflows/yapf.yml@main