[RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[Nicholas Piggin]

Tree here

https://gitlab.com/npiggin/kvm-unit-tests/-/tree/shellcheck

Again on top of the "v8 migration, powerpc improvements" series. I
don't plan to rebase the other way around since it's a lot of work.
So this is still in RFC until the other big series gets merged.

Thanks to Andrew for a lot of review. A submitted the likely s390x
bugs separately ahead of this series, and also disabled one of the
tests and dropped its fix patch as-per review comments. Hence 3 fewer
patches. Other than that, since last post:

* Tidied commit messages and added some of Andrew's comments.
* Removed the "SC2034 unused variable" blanket disable, and just
  suppressed the config.mak and a couple of other warnings.
* Blanket disabled "SC2235 Use { ..; } instead of (..)" and dropped
  the fix for it.
* Change warning suppression comments as per Andrew's review, also
  mention in the new unittests doc about the "check =" option not
  allowing whitespace etc in the name since we don't cope with that.

Thanks,
Nick

Nicholas Piggin (14):
  Add initial shellcheck checking
  shellcheck: Fix SC2223
  shellcheck: Fix SC2295
  shellcheck: Fix SC2094
  shellcheck: Fix SC2006
  shellcheck: Fix SC2155
  shellcheck: Fix SC2143
  shellcheck: Fix SC2013
  shellcheck: Fix SC2145
  shellcheck: Fix SC2124
  shellcheck: Fix SC2294
  shellcheck: Fix SC2178
  shellcheck: Fix SC2048
  shellcheck: Suppress various messages

 .shellcheckrc           | 30 ++++++++++++++++++++++++
 Makefile                |  4 ++++
 README.md               |  3 +++
 arm/efi/run             |  4 ++--
 configure               |  2 ++
 riscv/efi/run           |  4 ++--
 run_tests.sh            | 11 +++++----
 scripts/arch-run.bash   | 52 ++++++++++++++++++++++++++++++-----------
 scripts/common.bash     |  5 +++-
 scripts/mkstandalone.sh |  4 +++-
 scripts/runtime.bash    | 14 +++++++----
 11 files changed, 105 insertions(+), 28 deletions(-)
 create mode 100644 .shellcheckrc

-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 01/14] Add initial shellcheck checking

[Nicholas Piggin]

This adds a basic shellcheck sytle file, some directives to help
find scripts, and a make shellcheck target.

When changes settle down this could be made part of the standard
build / CI flow.

Suggested-by: Andrew Jones <andrew.jones@linux.dev>
Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 .shellcheckrc       | 30 ++++++++++++++++++++++++++++++
 Makefile            |  4 ++++
 README.md           |  3 +++
 scripts/common.bash |  5 ++++-
 4 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 .shellcheckrc

diff --git a/.shellcheckrc b/.shellcheckrc
new file mode 100644
index 000000000..491af18bd
--- /dev/null
+++ b/.shellcheckrc
@@ -0,0 +1,30 @@
+# shellcheck configuration file
+external-sources=true
+
+# Optional extras --  https://www.shellcheck.net/wiki/Optional
+# Possibilities, e.g., -
+# quote‐safe‐variables
+# require-double-brackets
+# require-variable-braces
+# add-default-case
+
+# Disable SC2004 style? I.e.,
+# In run_tests.sh line 67:
+#            if (( $unittest_run_queues <= 0 )); then
+#                  ^------------------^ SC2004 (style): $/${} is unnecessary on arithmetic variables.
+disable=SC2004
+
+# Disable SC2086 for now, double quote to prevent globbing and word
+# splitting. There are lots of places that use it for word splitting
+# (e.g., invoking commands with arguments) that break. Should have a
+# more consistent approach for this (perhaps use arrays for such cases)
+# but for now disable.
+# SC2086 (info): Double quote to prevent globbing and word splitting.
+disable=SC2086
+
+# Disable SC2235.  Most developers are used to seeing expressions
+# like a || (b && c), not a || { b && c ; }. The subshell overhead in
+# kvm-unit-tests is negligible as it's not shell-heavy in the first
+# place (time is dominated by qemu startup/shutdown and test execution)
+# SC2235 (style): Use { ..; } instead of (..) to avoid subshell overhead.
+disable=SC2235
diff --git a/Makefile b/Makefile
index 4e0f54543..4863cfdc6 100644
--- a/Makefile
+++ b/Makefile
@@ -141,6 +141,10 @@ cscope:
 		-name '*.[chsS]' -exec realpath --relative-base=$(CURDIR) {} \; | sort -u > ./cscope.files
 	cscope -bk
 
+.PHONY: shellcheck
+shellcheck:
+	shellcheck -a run_tests.sh */run */efi/run scripts/mkstandalone.sh
+
 .PHONY: tags
 tags:
 	ctags -R
diff --git a/README.md b/README.md
index 6e82dc225..03ff5994e 100644
--- a/README.md
+++ b/README.md
@@ -193,3 +193,6 @@ with `git config diff.orderFile scripts/git.difforder` enables it.
 
 We strive to follow the Linux kernels coding style so it's recommended
 to run the kernel's ./scripts/checkpatch.pl on new patches.
+
+Also run make shellcheck before submitting a patch which touches bash
+scripts.
diff --git a/scripts/common.bash b/scripts/common.bash
index ee1dd8659..3aa557c8c 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -82,8 +82,11 @@ function arch_cmd()
 }
 
 # The current file has to be the only file sourcing the arch helper
-# file
+# file. Shellcheck can't follow this so help it out. There doesn't appear to be a
+# way to specify multiple alternatives, so we will have to rethink this if things
+# get more complicated.
 ARCH_FUNC=scripts/${ARCH}/func.bash
 if [ -f "${ARCH_FUNC}" ]; then
+# shellcheck source=scripts/s390x/func.bash
 	source "${ARCH_FUNC}"
 fi
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 02/14] shellcheck: Fix SC2223

[Nicholas Piggin]

  SC2223 (info): This default assignment may cause DoS due to globbing.
  Quote it.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 run_tests.sh         | 4 ++--
 scripts/runtime.bash | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index bb3024ff9..9067e529e 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -158,8 +158,8 @@ function run_task()
 	fi
 }
 
-: ${unittest_log_dir:=logs}
-: ${unittest_run_queues:=1}
+: "${unittest_log_dir:=logs}"
+: "${unittest_run_queues:=1}"
 config=$TEST_DIR/unittests.cfg
 
 print_testname()
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index e4ad1962f..2d7ff5baa 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -1,6 +1,6 @@
 : "${RUNTIME_arch_run?}"
-: ${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}
-: ${TIMEOUT:=90s}
+: "${MAX_SMP:=$(getconf _NPROCESSORS_ONLN)}"
+: "${TIMEOUT:=90s}"
 
 PASS() { echo -ne "\e[32mPASS\e[0m"; }
 SKIP() { echo -ne "\e[33mSKIP\e[0m"; }
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 03/14] shellcheck: Fix SC2295

[Nicholas Piggin]

  SC2295 (info): Expansions inside ${..} need to be quoted separately,
  otherwise they match as patterns.

Doesn't appear to be a bug since the match string does not include
patterns.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 run_tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run_tests.sh b/run_tests.sh
index 9067e529e..116188e92 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -99,7 +99,7 @@ else
         local testname="$1"
         CR=$'\r'
         while read -r line; do
-            line="${line%$CR}"
+            line="${line%"$CR"}"
             case "${line:0:4}" in
                 PASS)
                     echo "ok TEST_NUMBER - ${testname}: ${line#??????}" >&3
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 04/14] shellcheck: Fix SC2094

[Nicholas Piggin]

  SC2094 (info): Make sure not to read and write the same file in the same
  pipeline.

This is not as clearly bad as overwriting an input file with >, but
could appended characters possibly be read in from the input
redirection?

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 1901a929f..472c31b08 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -492,6 +492,8 @@ env_file ()
 
 env_errata ()
 {
+	local new_env
+
 	if [ "$ACCEL" = "tcg" ]; then
 		export "ERRATA_FORCE=y"
 	elif [ "$ERRATATXT" ] && [ ! -f "$ERRATATXT" ]; then
@@ -500,7 +502,8 @@ env_errata ()
 	elif [ "$ERRATATXT" ]; then
 		env_generate_errata
 	fi
-	sort <(env | grep '^ERRATA_') <(grep '^ERRATA_' $KVM_UNIT_TESTS_ENV) | uniq -u >>$KVM_UNIT_TESTS_ENV
+	new_env=$(sort <(env | grep '^ERRATA_') <(grep '^ERRATA_' $KVM_UNIT_TESTS_ENV) | uniq -u)
+	echo "$new_env" >>$KVM_UNIT_TESTS_ENV
 }
 
 env_generate_errata ()
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 05/14] shellcheck: Fix SC2006

[Nicholas Piggin]

  SC2006 (style): Use $(...) notation instead of legacy backticks `...`.

No bug identified.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 6 +++---
 scripts/runtime.bash  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 472c31b08..f9d1fade9 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -271,16 +271,16 @@ do_migration ()
 	qmp ${src_qmp} '"migrate", "arguments": { "uri": "unix:'${dst_incoming}'" }' > ${src_qmpout}
 
 	# Wait for the migration to complete
-	migstatus=`qmp ${src_qmp} '"query-migrate"' | grep return`
+	migstatus=$(qmp ${src_qmp} '"query-migrate"' | grep return)
 	while ! grep -q '"completed"' <<<"$migstatus" ; do
 		sleep 0.1
-		if ! migstatus=`qmp ${src_qmp} '"query-migrate"'`; then
+		if ! migstatus=$(qmp ${src_qmp} '"query-migrate"'); then
 			echo "ERROR: Querying migration state failed." >&2
 			echo > ${dst_infifo}
 			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
 			return 2
 		fi
-		migstatus=`grep return <<<"$migstatus"`
+		migstatus=$(grep return <<<"$migstatus")
 		if grep -q '"failed"' <<<"$migstatus"; then
 			echo "ERROR: Migration failed." >&2
 			echo > ${dst_infifo}
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 2d7ff5baa..f79c4e281 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -61,9 +61,9 @@ function print_result()
     local reason="$4"
 
     if [ -z "$reason" ]; then
-        echo "`$status` $testname $summary"
+        echo "$($status) $testname $summary"
     else
-        echo "`$status` $testname ($reason)"
+        echo "$($status) $testname ($reason)"
     fi
 }
 
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 06/14] shellcheck: Fix SC2155

[Nicholas Piggin]

  SC2155 (warning): Declare and assign separately to avoid masking
  return values.

No bug identified.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 10 +++++++---
 scripts/runtime.bash  |  4 +++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index f9d1fade9..ae4b06679 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -411,7 +411,8 @@ initrd_cleanup ()
 {
 	rm -f $KVM_UNIT_TESTS_ENV
 	if [ "$KVM_UNIT_TESTS_ENV_OLD" ]; then
-		export KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD"
+		export KVM_UNIT_TESTS_ENV
+		KVM_UNIT_TESTS_ENV="$KVM_UNIT_TESTS_ENV_OLD"
 	else
 		unset KVM_UNIT_TESTS_ENV
 	fi
@@ -423,7 +424,8 @@ initrd_create ()
 	if [ "$ENVIRON_DEFAULT" = "yes" ]; then
 		trap_exit_push 'initrd_cleanup'
 		[ -f "$KVM_UNIT_TESTS_ENV" ] && export KVM_UNIT_TESTS_ENV_OLD="$KVM_UNIT_TESTS_ENV"
-		export KVM_UNIT_TESTS_ENV=$(mktemp)
+		export KVM_UNIT_TESTS_ENV
+		KVM_UNIT_TESTS_ENV=$(mktemp)
 		env_params
 		env_file
 		env_errata || return $?
@@ -566,7 +568,9 @@ env_generate_errata ()
 
 trap_exit_push ()
 {
-	local old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
+	local old_exit
+
+	old_exit=$(trap -p EXIT | sed "s/^[^']*'//;s/'[^']*$//")
 	trap -- "$1; $old_exit" EXIT
 }
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index f79c4e281..3b76aec9e 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -15,7 +15,9 @@ extract_summary()
 # We assume that QEMU is going to work if it tried to load the kernel
 premature_failure()
 {
-    local log="$(eval "$(get_cmdline _NO_FILE_4Uhere_)" 2>&1)"
+    local log
+
+    log="$(eval "$(get_cmdline _NO_FILE_4Uhere_)" 2>&1)"
 
     echo "$log" | grep "_NO_FILE_4Uhere_" |
         grep -q -e "[Cc]ould not \(load\|open\) kernel" \
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 07/14] shellcheck: Fix SC2143

[Nicholas Piggin]

  SC2143 (style): Use ! grep -q instead of comparing output with
  [ -z .. ].

Not a bug.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index ae4b06679..cd75405c8 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -61,7 +61,11 @@ run_qemu ()
 		# Even when ret==1 (unittest success) if we also got stderr
 		# logs, then we assume a QEMU failure. Otherwise we translate
 		# status of 1 to 0 (SUCCESS)
-		if [ -z "$(echo "$errors" | grep -vi warning)" ]; then
+	        if [ "$errors" ]; then
+			if ! grep -qvi warning <<<"$errors" ; then
+				ret=0
+			fi
+		else
 			ret=0
 		fi
 	fi
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 08/14] shellcheck: Fix SC2013

[Nicholas Piggin]

  SC2013 (info): To read lines rather than words, pipe/redirect to a
  'while read' loop.

Not a bug.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index cd75405c8..45ec8f57d 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -487,7 +487,7 @@ env_file ()
 
 	[ ! -f "$KVM_UNIT_TESTS_ENV_OLD" ] && return
 
-	for line in $(grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD"); do
+	grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD" | while IFS= read -r line ; do
 		var=${line%%=*}
 		if ! grep -q "^$var=" $KVM_UNIT_TESTS_ENV; then
 			eval export "$line"
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 09/14] shellcheck: Fix SC2145

[Nicholas Piggin]

  SC2145 (error): Argument mixes string and array. Use * or separate
  argument.

Shouldn't be a bug, since the preceding string ends with a space and
there aren't any succeeding strings.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arm/efi/run             | 2 +-
 riscv/efi/run           | 2 +-
 scripts/mkstandalone.sh | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arm/efi/run b/arm/efi/run
index f07a6e55c..cf6d34b0b 100755
--- a/arm/efi/run
+++ b/arm/efi/run
@@ -87,7 +87,7 @@ uefi_shell_run()
 if [ "$EFI_DIRECT" = "y" ]; then
 	$TEST_DIR/run \
 		$KERNEL_NAME \
-		-append "$(basename $KERNEL_NAME) ${cmd_args[@]}" \
+		-append "$(basename $KERNEL_NAME) ${cmd_args[*]}" \
 		-bios "$EFI_UEFI" \
 		"${qemu_args[@]}"
 else
diff --git a/riscv/efi/run b/riscv/efi/run
index 982b8b9c4..cce068694 100755
--- a/riscv/efi/run
+++ b/riscv/efi/run
@@ -97,7 +97,7 @@ if [ "$EFI_DIRECT" = "y" ]; then
 	fi
 	$TEST_DIR/run \
 		$KERNEL_NAME \
-		-append "$(basename $KERNEL_NAME) ${cmd_args[@]}" \
+		-append "$(basename $KERNEL_NAME) ${cmd_args[*]}" \
 		-machine pflash0=pflash0 \
 		-blockdev node-name=pflash0,driver=file,read-only=on,filename="$EFI_UEFI" \
 		"${qemu_args[@]}"
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 86c7e5498..756647f29 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -76,7 +76,7 @@ generate_test ()
 
 	cat scripts/runtime.bash
 
-	echo "run ${args[@]}"
+	echo "run ${args[*]}"
 }
 
 function mkstandalone()
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 10/14] shellcheck: Fix SC2124

[Nicholas Piggin]

  SC2124 (warning): Assigning an array to a string! Assign as array, or
  use * instead of @ to concatenate.

Shouldn't be a bug since bash concatenates with space and eval is
used on the result.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 45ec8f57d..95b6fa64d 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -150,7 +150,7 @@ run_migration ()
 		return 77
 	fi
 
-	migcmdline=$@
+	migcmdline=("$@")
 
 	trap 'trap - TERM ; kill 0 ; exit 2' INT TERM
 	trap 'rm -f ${src_out} ${dst_out} ${src_outfifo} ${dst_outfifo} ${dst_incoming} ${src_qmp} ${dst_qmp} ${src_infifo} ${dst_infifo}' RETURN EXIT
@@ -179,7 +179,7 @@ run_migration ()
 	exec {src_infifo_fd}<>${src_infifo}
 	exec {dst_infifo_fd}<>${dst_infifo}
 
-	eval "$migcmdline" \
+	eval "${migcmdline[@]}" \
 		-chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
 		-mon chardev=mon,mode=control \
 		< ${src_infifo} > ${src_outfifo} &
@@ -219,7 +219,7 @@ run_migration ()
 
 do_migration ()
 {
-	eval "$migcmdline" \
+	eval "${migcmdline[@]}" \
 		-chardev socket,id=mon,path=${dst_qmp},server=on,wait=off \
 		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
 		< ${dst_infifo} > ${dst_outfifo} &
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 11/14] shellcheck: Fix SC2294

[Nicholas Piggin]

  SC2294 (warning): eval negates the benefit of arrays. Drop eval to
  preserve whitespace/symbols (or eval as string).

No bug identified.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 95b6fa64d..98d29b671 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -179,7 +179,7 @@ run_migration ()
 	exec {src_infifo_fd}<>${src_infifo}
 	exec {dst_infifo_fd}<>${dst_infifo}
 
-	eval "${migcmdline[@]}" \
+	"${migcmdline[@]}" \
 		-chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
 		-mon chardev=mon,mode=control \
 		< ${src_infifo} > ${src_outfifo} &
@@ -219,7 +219,7 @@ run_migration ()
 
 do_migration ()
 {
-	eval "${migcmdline[@]}" \
+	"${migcmdline[@]}" \
 		-chardev socket,id=mon,path=${dst_qmp},server=on,wait=off \
 		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
 		< ${dst_infifo} > ${dst_outfifo} &
@@ -357,7 +357,7 @@ run_panic ()
 	qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX)
 
 	# start VM stopped so we don't miss any events
-	eval "$@" -chardev socket,id=mon,path=${qmp},server=on,wait=off \
+	"$@" -chardev socket,id=mon,path=${qmp},server=on,wait=off \
 		-mon chardev=mon,mode=control -S &
 
 	panic_event_count=$(qmp_events ${qmp} | jq -c 'select(.event == "GUEST_PANICKED")' | wc -l)
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 12/14] shellcheck: Fix SC2178

[Nicholas Piggin]

  SC2178 (warning): Variable was used as an array but is now assigned a
  string.

No bug identified.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arm/efi/run   | 2 +-
 riscv/efi/run | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arm/efi/run b/arm/efi/run
index cf6d34b0b..8f41fc02d 100755
--- a/arm/efi/run
+++ b/arm/efi/run
@@ -44,7 +44,7 @@ qemu_args=()
 cmd_args=()
 while (( "$#" )); do
 	if [ "$1" = "-append" ]; then
-		cmd_args=$2
+		cmd_args=("$2")
 		shift 2
 	else
 		qemu_args+=("$1")
diff --git a/riscv/efi/run b/riscv/efi/run
index cce068694..5a72683a6 100755
--- a/riscv/efi/run
+++ b/riscv/efi/run
@@ -47,7 +47,7 @@ qemu_args=()
 cmd_args=()
 while (( "$#" )); do
 	if [ "$1" = "-append" ]; then
-		cmd_args=$2
+		cmd_args=("$2")
 		shift 2
 	else
 		qemu_args+=("$1")
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 13/14] shellcheck: Fix SC2048

[Nicholas Piggin]

  SC2048 (warning): Use "$@" (with quotes) to prevent whitespace
  problems.

No bug identified.

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 run_tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run_tests.sh b/run_tests.sh
index 116188e92..938bb8edf 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -44,7 +44,7 @@ fi
 
 only_tests=""
 list_tests=""
-args=$(getopt -u -o ag:htj:vl -l all,group:,help,tap13,parallel:,verbose,list,probe-maxsmp -- $*)
+args=$(getopt -u -o ag:htj:vl -l all,group:,help,tap13,parallel:,verbose,list,probe-maxsmp -- "$@")
 [ $? -ne 0 ] && exit 2;
 set -- $args;
 while [ $# -gt 0 ]; do
-- 
2.43.0

[RFC kvm-unit-tests PATCH v2 14/14] shellcheck: Suppress various messages

[Nicholas Piggin]

Various info and warnings are suppressed here, where circumstances
(commented) warrant.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 configure               |  2 ++
 run_tests.sh            |  3 +++
 scripts/arch-run.bash   | 15 +++++++++++++++
 scripts/mkstandalone.sh |  2 ++
 scripts/runtime.bash    |  2 ++
 5 files changed, 24 insertions(+)

diff --git a/configure b/configure
index 8508396af..6ebac7e0a 100755
--- a/configure
+++ b/configure
@@ -437,6 +437,8 @@ ln -sf "$asm" lib/asm
 
 # create the config
 cat <<EOF > config.mak
+# Shellcheck does not see these are used
+# shellcheck disable=SC2034
 SRCDIR=$srcdir
 PREFIX=$prefix
 HOST=$host
diff --git a/run_tests.sh b/run_tests.sh
index 938bb8edf..152323ffc 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -45,6 +45,9 @@ fi
 only_tests=""
 list_tests=""
 args=$(getopt -u -o ag:htj:vl -l all,group:,help,tap13,parallel:,verbose,list,probe-maxsmp -- "$@")
+# Shellcheck likes to test commands directly rather than with $? but sometimes they
+# are too long to put in the same test.
+# shellcheck disable=SC2181
 [ $? -ne 0 ] && exit 2;
 set -- $args;
 while [ $# -gt 0 ]; do
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 98d29b671..7e5b2bdf1 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -44,6 +44,8 @@ run_qemu ()
 	if [ "$errors" ]; then
 		sig=$(grep 'terminating on signal' <<<"$errors")
 		if [ "$sig" ]; then
+			# This is too complex for ${var/search/replace}
+			# shellcheck disable=SC2001
 			sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"$sig")
 		fi
 	fi
@@ -174,9 +176,12 @@ run_migration ()
 
 	# Holding both ends of the input fifo open prevents opens from
 	# blocking and readers getting EOF when a writer closes it.
+	# These fds appear to be unused to shellcheck so quieten the warning.
 	mkfifo ${src_infifo}
 	mkfifo ${dst_infifo}
+	# shellcheck disable=SC2034
 	exec {src_infifo_fd}<>${src_infifo}
+	# shellcheck disable=SC2034
 	exec {dst_infifo_fd}<>${dst_infifo}
 
 	"${migcmdline[@]}" \
@@ -184,6 +189,9 @@ run_migration ()
 		-mon chardev=mon,mode=control \
 		< ${src_infifo} > ${src_outfifo} &
 	live_pid=$!
+	# Shellcheck complains about useless cat but it is clearer than a
+	# redirect in this case.
+	# shellcheck disable=SC2002
 	cat ${src_outfifo} | tee ${src_out} | filter_quiet_msgs &
 
 	# Start the first destination QEMU machine in advance of the test
@@ -224,6 +232,9 @@ do_migration ()
 		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
 		< ${dst_infifo} > ${dst_outfifo} &
 	incoming_pid=$!
+	# Shellcheck complains about useless cat but it is clearer than a
+	# redirect in this case.
+	# shellcheck disable=SC2002
 	cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
 
 	# The test must prompt the user to migrate, so wait for the
@@ -467,6 +478,8 @@ env_params ()
 			[ -n "$ACCEL" ] && QEMU_ACCEL=$ACCEL
 		fi
 		QEMU_VERSION_STRING="$($qemu -h | head -1)"
+		# SC does not seee QEMU_MAJOR|MINOR|MICRO are used
+		# shellcheck disable=SC2034
 		IFS='[ .]' read -r _ _ _ QEMU_MAJOR QEMU_MINOR QEMU_MICRO rest <<<"$QEMU_VERSION_STRING"
 	fi
 	env_add_params QEMU_ACCEL QEMU_VERSION_STRING QEMU_MAJOR QEMU_MINOR QEMU_MICRO
@@ -597,6 +610,8 @@ hvf_available ()
 
 set_qemu_accelerator ()
 {
+	# Shellcheck does not seee ACCEL_PROPS is used
+	# shellcheck disable=SC2034
 	ACCEL_PROPS=${ACCEL#"${ACCEL%%,*}"}
 	ACCEL=${ACCEL%%,*}
 
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 756647f29..2318a85f0 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -65,6 +65,8 @@ generate_test ()
 	fi
 
 	temp_file bin "$kernel"
+	# Don't want to expand $bin but print it as-is.
+	# shellcheck disable=SC2016
 	args[3]='$bin'
 
 	(echo "#!/usr/bin/env bash"
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 3b76aec9e..6e712214d 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -137,6 +137,8 @@ function run()
     # the check line can contain multiple files to check separated by a space
     # but each check parameter needs to be of the form <path>=<value>
     if [ "$check" ]; then
+        # There is no globbing or whitespace allowed in check parameters.
+        # shellcheck disable=SC2206
         check=($check)
         for check_param in "${check[@]}"; do
             path=${check_param%%=*}
-- 
2.43.0

Re: [RFC kvm-unit-tests PATCH v2 08/14] shellcheck: Fix SC2013

[Andrew Jones]

On Sat, Apr 06, 2024 at 10:38:17PM +1000, Nicholas Piggin wrote:
>   SC2013 (info): To read lines rather than words, pipe/redirect to a
>   'while read' loop.
> 
> Not a bug.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  scripts/arch-run.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index cd75405c8..45ec8f57d 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -487,7 +487,7 @@ env_file ()
>  
>  	[ ! -f "$KVM_UNIT_TESTS_ENV_OLD" ] && return
>  
> -	for line in $(grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD"); do
> +	grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD" | while IFS= read -r line ; do
>  		var=${line%%=*}
>  		if ! grep -q "^$var=" $KVM_UNIT_TESTS_ENV; then
>  			eval export "$line"
> -- 
> 2.43.0
>

I already gave an r-b on this one. Here it is again,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Re: [RFC kvm-unit-tests PATCH v2 14/14] shellcheck: Suppress various messages

[Andrew Jones]

On Sat, Apr 06, 2024 at 10:38:23PM +1000, Nicholas Piggin wrote:
> Various info and warnings are suppressed here, where circumstances
> (commented) warrant.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  configure               |  2 ++
>  run_tests.sh            |  3 +++
>  scripts/arch-run.bash   | 15 +++++++++++++++
>  scripts/mkstandalone.sh |  2 ++
>  scripts/runtime.bash    |  2 ++
>  5 files changed, 24 insertions(+)
> 
> diff --git a/configure b/configure
> index 8508396af..6ebac7e0a 100755
> --- a/configure
> +++ b/configure
> @@ -437,6 +437,8 @@ ln -sf "$asm" lib/asm
>  
>  # create the config
>  cat <<EOF > config.mak
> +# Shellcheck does not see these are used
> +# shellcheck disable=SC2034
>  SRCDIR=$srcdir
>  PREFIX=$prefix
>  HOST=$host
> diff --git a/run_tests.sh b/run_tests.sh
> index 938bb8edf..152323ffc 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -45,6 +45,9 @@ fi
>  only_tests=""
>  list_tests=""
>  args=$(getopt -u -o ag:htj:vl -l all,group:,help,tap13,parallel:,verbose,list,probe-maxsmp -- "$@")
> +# Shellcheck likes to test commands directly rather than with $? but sometimes they
> +# are too long to put in the same test.
> +# shellcheck disable=SC2181
>  [ $? -ne 0 ] && exit 2;
>  set -- $args;
>  while [ $# -gt 0 ]; do
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 98d29b671..7e5b2bdf1 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -44,6 +44,8 @@ run_qemu ()
>  	if [ "$errors" ]; then
>  		sig=$(grep 'terminating on signal' <<<"$errors")
>  		if [ "$sig" ]; then
> +			# This is too complex for ${var/search/replace}
> +			# shellcheck disable=SC2001
>  			sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"$sig")
>  		fi
>  	fi
> @@ -174,9 +176,12 @@ run_migration ()
>  
>  	# Holding both ends of the input fifo open prevents opens from
>  	# blocking and readers getting EOF when a writer closes it.
> +	# These fds appear to be unused to shellcheck so quieten the warning.
>  	mkfifo ${src_infifo}
>  	mkfifo ${dst_infifo}
> +	# shellcheck disable=SC2034
>  	exec {src_infifo_fd}<>${src_infifo}
> +	# shellcheck disable=SC2034
>  	exec {dst_infifo_fd}<>${dst_infifo}
>  
>  	"${migcmdline[@]}" \
> @@ -184,6 +189,9 @@ run_migration ()
>  		-mon chardev=mon,mode=control \
>  		< ${src_infifo} > ${src_outfifo} &
>  	live_pid=$!
> +	# Shellcheck complains about useless cat but it is clearer than a
> +	# redirect in this case.
> +	# shellcheck disable=SC2002
>  	cat ${src_outfifo} | tee ${src_out} | filter_quiet_msgs &
>  
>  	# Start the first destination QEMU machine in advance of the test
> @@ -224,6 +232,9 @@ do_migration ()
>  		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
>  		< ${dst_infifo} > ${dst_outfifo} &
>  	incoming_pid=$!
> +	# Shellcheck complains about useless cat but it is clearer than a
> +	# redirect in this case.
> +	# shellcheck disable=SC2002
>  	cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
>  
>  	# The test must prompt the user to migrate, so wait for the
> @@ -467,6 +478,8 @@ env_params ()
>  			[ -n "$ACCEL" ] && QEMU_ACCEL=$ACCEL
>  		fi
>  		QEMU_VERSION_STRING="$($qemu -h | head -1)"
> +		# SC does not seee QEMU_MAJOR|MINOR|MICRO are used

Shellcheck does not see

> +		# shellcheck disable=SC2034
>  		IFS='[ .]' read -r _ _ _ QEMU_MAJOR QEMU_MINOR QEMU_MICRO rest <<<"$QEMU_VERSION_STRING"
>  	fi
>  	env_add_params QEMU_ACCEL QEMU_VERSION_STRING QEMU_MAJOR QEMU_MINOR QEMU_MICRO
> @@ -597,6 +610,8 @@ hvf_available ()
>  
>  set_qemu_accelerator ()
>  {
> +	# Shellcheck does not seee ACCEL_PROPS is used

see

> +	# shellcheck disable=SC2034
>  	ACCEL_PROPS=${ACCEL#"${ACCEL%%,*}"}
>  	ACCEL=${ACCEL%%,*}
>  
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index 756647f29..2318a85f0 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -65,6 +65,8 @@ generate_test ()
>  	fi
>  
>  	temp_file bin "$kernel"
> +	# Don't want to expand $bin but print it as-is.
> +	# shellcheck disable=SC2016
>  	args[3]='$bin'
>  
>  	(echo "#!/usr/bin/env bash"
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 3b76aec9e..6e712214d 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -137,6 +137,8 @@ function run()
>      # the check line can contain multiple files to check separated by a space
>      # but each check parameter needs to be of the form <path>=<value>
>      if [ "$check" ]; then
> +        # There is no globbing or whitespace allowed in check parameters.
> +        # shellcheck disable=SC2206
>          check=($check)
>          for check_param in "${check[@]}"; do
>              path=${check_param%%=*}
> -- 
> 2.43.0
>

Other than comment fixes,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Re: [RFC kvm-unit-tests PATCH v2 08/14] shellcheck: Fix SC2013

[Nicholas Piggin]

On Mon Apr 8, 2024 at 5:34 PM AEST, Andrew Jones wrote:
> On Sat, Apr 06, 2024 at 10:38:17PM +1000, Nicholas Piggin wrote:
> >   SC2013 (info): To read lines rather than words, pipe/redirect to a
> >   'while read' loop.
> > 
> > Not a bug.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  scripts/arch-run.bash | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> > index cd75405c8..45ec8f57d 100644
> > --- a/scripts/arch-run.bash
> > +++ b/scripts/arch-run.bash
> > @@ -487,7 +487,7 @@ env_file ()
> >  
> >  	[ ! -f "$KVM_UNIT_TESTS_ENV_OLD" ] && return
> >  
> > -	for line in $(grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD"); do
> > +	grep -E '^[[:blank:]]*[[:alpha:]_][[:alnum:]_]*=' "$KVM_UNIT_TESTS_ENV_OLD" | while IFS= read -r line ; do
> >  		var=${line%%=*}
> >  		if ! grep -q "^$var=" $KVM_UNIT_TESTS_ENV; then
> >  			eval export "$line"
> > -- 
> > 2.43.0
> >
>
> I already gave an r-b on this one. Here it is again,
>
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Yeah I realised just after sending. Thank you.

Thanks,
Nick

Re: [RFC kvm-unit-tests PATCH v2 01/14] Add initial shellcheck checking

[Thomas Huth]

On 06/04/2024 14.38, Nicholas Piggin wrote:
> This adds a basic shellcheck sytle file, some directives to help

s/sytle/style/

> find scripts, and a make shellcheck target.
> 
> When changes settle down this could be made part of the standard
> build / CI flow.
> 
> Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
...
> diff --git a/README.md b/README.md
> index 6e82dc225..03ff5994e 100644
> --- a/README.md
> +++ b/README.md
> @@ -193,3 +193,6 @@ with `git config diff.orderFile scripts/git.difforder` enables it.
>   
>   We strive to follow the Linux kernels coding style so it's recommended
>   to run the kernel's ./scripts/checkpatch.pl on new patches.
> +
> +Also run make shellcheck before submitting a patch which touches bash

I'd maybe put "make shellcheck" in quotes to make the sentence more readable?

> +scripts.
> diff --git a/scripts/common.bash b/scripts/common.bash
> index ee1dd8659..3aa557c8c 100644
> --- a/scripts/common.bash
> +++ b/scripts/common.bash
> @@ -82,8 +82,11 @@ function arch_cmd()
>   }
>   
>   # The current file has to be the only file sourcing the arch helper
> -# file
> +# file. Shellcheck can't follow this so help it out. There doesn't appear to be a
> +# way to specify multiple alternatives, so we will have to rethink this if things
> +# get more complicated.
>   ARCH_FUNC=scripts/${ARCH}/func.bash
>   if [ -f "${ARCH_FUNC}" ]; then
> +# shellcheck source=scripts/s390x/func.bash
>   	source "${ARCH_FUNC}"
>   fi

  Thomas

Re: [RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[Thomas Huth]

On 06/04/2024 14.38, Nicholas Piggin wrote:
> Tree here
> 
> https://gitlab.com/npiggin/kvm-unit-tests/-/tree/shellcheck
> 
> Again on top of the "v8 migration, powerpc improvements" series. I
> don't plan to rebase the other way around since it's a lot of work.
> So this is still in RFC until the other big series gets merged.
> 
> Thanks to Andrew for a lot of review. A submitted the likely s390x
> bugs separately ahead of this series, and also disabled one of the
> tests and dropped its fix patch as-per review comments. Hence 3 fewer
> patches. Other than that, since last post:
> 
> * Tidied commit messages and added some of Andrew's comments.
> * Removed the "SC2034 unused variable" blanket disable, and just
>    suppressed the config.mak and a couple of other warnings.
> * Blanket disabled "SC2235 Use { ..; } instead of (..)" and dropped
>    the fix for it.
> * Change warning suppression comments as per Andrew's review, also
>    mention in the new unittests doc about the "check =" option not
>    allowing whitespace etc in the name since we don't cope with that.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (14):
>    Add initial shellcheck checking
>    shellcheck: Fix SC2223
>    shellcheck: Fix SC2295
>    shellcheck: Fix SC2094
>    shellcheck: Fix SC2006
>    shellcheck: Fix SC2155
>    shellcheck: Fix SC2143
>    shellcheck: Fix SC2013
>    shellcheck: Fix SC2145
>    shellcheck: Fix SC2124
>    shellcheck: Fix SC2294
>    shellcheck: Fix SC2178
>    shellcheck: Fix SC2048
>    shellcheck: Suppress various messages

I went ahead and pushed a bunch of your patches to the k-u-t master branch 
now. However, there were also some patches which did not apply cleanly to 
master anymore, so please rebase the remaining patches and then send them again.

  Thanks!
   Thomas

Re: [RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[Nicholas Piggin]

On Mon Apr 15, 2024 at 9:59 PM AEST, Thomas Huth wrote:
> On 06/04/2024 14.38, Nicholas Piggin wrote:
> > Tree here
> > 
> > https://gitlab.com/npiggin/kvm-unit-tests/-/tree/shellcheck
> > 
> > Again on top of the "v8 migration, powerpc improvements" series. I
> > don't plan to rebase the other way around since it's a lot of work.
> > So this is still in RFC until the other big series gets merged.
> > 
> > Thanks to Andrew for a lot of review. A submitted the likely s390x
> > bugs separately ahead of this series, and also disabled one of the
> > tests and dropped its fix patch as-per review comments. Hence 3 fewer
> > patches. Other than that, since last post:
> > 
> > * Tidied commit messages and added some of Andrew's comments.
> > * Removed the "SC2034 unused variable" blanket disable, and just
> >    suppressed the config.mak and a couple of other warnings.
> > * Blanket disabled "SC2235 Use { ..; } instead of (..)" and dropped
> >    the fix for it.
> > * Change warning suppression comments as per Andrew's review, also
> >    mention in the new unittests doc about the "check =" option not
> >    allowing whitespace etc in the name since we don't cope with that.
> > 
> > Thanks,
> > Nick
> > 
> > Nicholas Piggin (14):
> >    Add initial shellcheck checking
> >    shellcheck: Fix SC2223
> >    shellcheck: Fix SC2295
> >    shellcheck: Fix SC2094
> >    shellcheck: Fix SC2006
> >    shellcheck: Fix SC2155
> >    shellcheck: Fix SC2143
> >    shellcheck: Fix SC2013
> >    shellcheck: Fix SC2145
> >    shellcheck: Fix SC2124
> >    shellcheck: Fix SC2294
> >    shellcheck: Fix SC2178
> >    shellcheck: Fix SC2048
> >    shellcheck: Suppress various messages
>
> I went ahead and pushed a bunch of your patches to the k-u-t master branch 
> now. However, there were also some patches which did not apply cleanly to 
> master anymore, so please rebase the remaining patches and then send them again.

Hey Thomas,

Yeah the sc patches were based on top of the big series, so some
collisions expected. I'll look at rebasing.

Thanks,
Nick

Re: [RFC kvm-unit-tests PATCH v2 01/14] Add initial shellcheck checking

[Nicholas Piggin]

On Thu Apr 11, 2024 at 5:03 PM AEST, Thomas Huth wrote:
> On 06/04/2024 14.38, Nicholas Piggin wrote:
> > This adds a basic shellcheck sytle file, some directives to help
>
> s/sytle/style/
>
> > find scripts, and a make shellcheck target.
> > 
> > When changes settle down this could be made part of the standard
> > build / CI flow.
> > 
> > Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> > Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> ...
> > diff --git a/README.md b/README.md
> > index 6e82dc225..03ff5994e 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -193,3 +193,6 @@ with `git config diff.orderFile scripts/git.difforder` enables it.
> >   
> >   We strive to follow the Linux kernels coding style so it's recommended
> >   to run the kernel's ./scripts/checkpatch.pl on new patches.
> > +
> > +Also run make shellcheck before submitting a patch which touches bash
>
> I'd maybe put "make shellcheck" in quotes to make the sentence more readable?

Agreed.

Thanks,
Nick

Re: [RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[Thomas Huth]

On 16/04/2024 05.26, Nicholas Piggin wrote:
> On Mon Apr 15, 2024 at 9:59 PM AEST, Thomas Huth wrote:
>> On 06/04/2024 14.38, Nicholas Piggin wrote:
>>> Tree here
>>>
>>> https://gitlab.com/npiggin/kvm-unit-tests/-/tree/shellcheck
>>>
>>> Again on top of the "v8 migration, powerpc improvements" series. I
>>> don't plan to rebase the other way around since it's a lot of work.
>>> So this is still in RFC until the other big series gets merged.
>>>
>>> Thanks to Andrew for a lot of review. A submitted the likely s390x
>>> bugs separately ahead of this series, and also disabled one of the
>>> tests and dropped its fix patch as-per review comments. Hence 3 fewer
>>> patches. Other than that, since last post:
>>>
>>> * Tidied commit messages and added some of Andrew's comments.
>>> * Removed the "SC2034 unused variable" blanket disable, and just
>>>     suppressed the config.mak and a couple of other warnings.
>>> * Blanket disabled "SC2235 Use { ..; } instead of (..)" and dropped
>>>     the fix for it.
>>> * Change warning suppression comments as per Andrew's review, also
>>>     mention in the new unittests doc about the "check =" option not
>>>     allowing whitespace etc in the name since we don't cope with that.
>>>
>>> Thanks,
>>> Nick
>>>
>>> Nicholas Piggin (14):
>>>     Add initial shellcheck checking
>>>     shellcheck: Fix SC2223
>>>     shellcheck: Fix SC2295
>>>     shellcheck: Fix SC2094
>>>     shellcheck: Fix SC2006
>>>     shellcheck: Fix SC2155
>>>     shellcheck: Fix SC2143
>>>     shellcheck: Fix SC2013
>>>     shellcheck: Fix SC2145
>>>     shellcheck: Fix SC2124
>>>     shellcheck: Fix SC2294
>>>     shellcheck: Fix SC2178
>>>     shellcheck: Fix SC2048
>>>     shellcheck: Suppress various messages
>>
>> I went ahead and pushed a bunch of your patches to the k-u-t master branch
>> now. However, there were also some patches which did not apply cleanly to
>> master anymore, so please rebase the remaining patches and then send them again.
> 
> Hey Thomas,
> 
> Yeah the sc patches were based on top of the big series, so some
> collisions expected. I'll look at rebasing.

Ah, ok, we can also try to get in the big series first ... I just lack 
enough spare time for reviewing currently, so it might take a while :-/

  Thomas

Re: [RFC kvm-unit-tests PATCH v2 00/14] add shellcheck support

[Nicholas Piggin]

On Tue Apr 16, 2024 at 2:46 PM AEST, Thomas Huth wrote:
> On 16/04/2024 05.26, Nicholas Piggin wrote:
> > On Mon Apr 15, 2024 at 9:59 PM AEST, Thomas Huth wrote:
> >> On 06/04/2024 14.38, Nicholas Piggin wrote:
> >>> Tree here
> >>>
> >>> https://gitlab.com/npiggin/kvm-unit-tests/-/tree/shellcheck
> >>>
> >>> Again on top of the "v8 migration, powerpc improvements" series. I
> >>> don't plan to rebase the other way around since it's a lot of work.
> >>> So this is still in RFC until the other big series gets merged.
> >>>
> >>> Thanks to Andrew for a lot of review. A submitted the likely s390x
> >>> bugs separately ahead of this series, and also disabled one of the
> >>> tests and dropped its fix patch as-per review comments. Hence 3 fewer
> >>> patches. Other than that, since last post:
> >>>
> >>> * Tidied commit messages and added some of Andrew's comments.
> >>> * Removed the "SC2034 unused variable" blanket disable, and just
> >>>     suppressed the config.mak and a couple of other warnings.
> >>> * Blanket disabled "SC2235 Use { ..; } instead of (..)" and dropped
> >>>     the fix for it.
> >>> * Change warning suppression comments as per Andrew's review, also
> >>>     mention in the new unittests doc about the "check =" option not
> >>>     allowing whitespace etc in the name since we don't cope with that.
> >>>
> >>> Thanks,
> >>> Nick
> >>>
> >>> Nicholas Piggin (14):
> >>>     Add initial shellcheck checking
> >>>     shellcheck: Fix SC2223
> >>>     shellcheck: Fix SC2295
> >>>     shellcheck: Fix SC2094
> >>>     shellcheck: Fix SC2006
> >>>     shellcheck: Fix SC2155
> >>>     shellcheck: Fix SC2143
> >>>     shellcheck: Fix SC2013
> >>>     shellcheck: Fix SC2145
> >>>     shellcheck: Fix SC2124
> >>>     shellcheck: Fix SC2294
> >>>     shellcheck: Fix SC2178
> >>>     shellcheck: Fix SC2048
> >>>     shellcheck: Suppress various messages
> >>
> >> I went ahead and pushed a bunch of your patches to the k-u-t master branch
> >> now. However, there were also some patches which did not apply cleanly to
> >> master anymore, so please rebase the remaining patches and then send them again.
> > 
> > Hey Thomas,
> > 
> > Yeah the sc patches were based on top of the big series, so some
> > collisions expected. I'll look at rebasing.
>
> Ah, ok, we can also try to get in the big series first ... I just lack 

They should have come first, but I'd written the multi migration code
before Andrew suggested adding sc, and it looked like hard work to
rebase the other way. I'll try again.

> enough spare time for reviewing currently, so it might take a while :-/

Understandable. You've done heaps of reviewing already so I really
appreciate it.

Thanks,
Nick