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

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

[Nicholas Piggin]

I foolishly promised Andrew I would look into shellcheck, so here
it is.

https://gitlab.com/npiggin/kvm-unit-tests/-/tree/powerpc?ref_type=heads

This is on top of the "v8 migration, powerpc improvements" series. For
now the patches are a bit raw but it does get down to zero[*] shellcheck
warnings while still passing gitlab CI.

[*] Modulo the relatively few cases where they're disabled or
suppressed.

I'd like comments about what should be enabled and disabled? There are
quite a lot of options. Lots of changes don't fix real bugs AFAIKS, so
there's some taste involved.

Could possibly be a couple of bugs, including in s390x specific. Any
review of those to confirm or deny bug is appreciated. I haven't tried
to create reproducers for them.

I added a quick comment on each one whether it looks like a bug or
harmless but I'm not a bash guru so could easily be wrong. I would
possibly pull any real bug fixes to the front of the series and describe
them as proper fix patches, and leave the other style / non-bugfixes in
the brief format.  shellcheck has a very good wiki explaining each issue
so there is not much point in rehashing that in the changelog.

One big thing kept disabled for now is the double-quoting to prevent
globbing and splitting warning that is disabled. That touches a lot of
code and we're very inconsistent about quoting variables today, but it's
not completely trivial because there are quite a lot of places that does
rely on splitting for invoking commands with arguments. That would need
some rework to avoid sprinkling a lot of warning suppressions around.
Possibly consistently using arrays for argument lists would be the best
solution?

Thanks,
Nick

Nicholas Piggin (17):
  Add initial shellcheck checking
  shellcheck: Fix SC2223
  shellcheck: Fix SC2295
  shellcheck: Fix SC2094
  shellcheck: Fix SC2006
  shellcheck: Fix SC2155
  shellcheck: Fix SC2235
  shellcheck: Fix SC2119, SC2120
  shellcheck: Fix SC2143
  shellcheck: Fix SC2013
  shellcheck: Fix SC2145
  shellcheck: Fix SC2124
  shellcheck: Fix SC2294
  shellcheck: Fix SC2178
  shellcheck: Fix SC2048
  shellcheck: Fix SC2153
  shellcheck: Suppress various messages

 .shellcheckrc           | 32 +++++++++++++++++++++++++
 Makefile                |  4 ++++
 README.md               |  2 ++
 arm/efi/run             |  4 ++--
 riscv/efi/run           |  4 ++--
 run_tests.sh            | 11 +++++----
 s390x/run               |  8 +++----
 scripts/arch-run.bash   | 52 ++++++++++++++++++++++++++++-------------
 scripts/common.bash     |  5 +++-
 scripts/mkstandalone.sh |  4 +++-
 scripts/runtime.bash    | 14 +++++++----
 scripts/s390x/func.bash |  2 +-
 12 files changed, 106 insertions(+), 36 deletions(-)
 create mode 100644 .shellcheckrc

-- 
2.43.0

[kvm-unit-tests RFC PATCH 01/17] 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>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 .shellcheckrc       | 32 ++++++++++++++++++++++++++++++++
 Makefile            |  4 ++++
 README.md           |  2 ++
 scripts/common.bash |  5 ++++-
 4 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 .shellcheckrc

diff --git a/.shellcheckrc b/.shellcheckrc
new file mode 100644
index 000000000..2a9a57c42
--- /dev/null
+++ b/.shellcheckrc
@@ -0,0 +1,32 @@
+# 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 SC2034 - config.mak contains a lot of these unused variable errors.
+# Maybe we could have a script extract the ones used by shell script and put
+# them in a generated file, to re-enable the warning.
+#
+# In config.mak line 1:
+# SRCDIR=/home/npiggin/src/kvm-unit-tests
+# ^----^ SC2034 (warning): SRCDIR appears unused. Verify use (or export if used externally).
+disable=SC2034
+
+# 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
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..77718675e 100644
--- a/README.md
+++ b/README.md
@@ -193,3 +193,5 @@ 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.
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

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

[Nicholas Piggin]

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

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

[kvm-unit-tests RFC PATCH 03/17] 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.

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

[kvm-unit-tests RFC PATCH 04/17] 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?

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

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

[Nicholas Piggin]

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

No bug identified.

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

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

[Nicholas Piggin]

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

No bug identified.

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

[kvm-unit-tests RFC PATCH 07/17] shellcheck: Fix SC2235

[Nicholas Piggin]

  SC2235 (style): Use { ..; } instead of (..) to avoid subshell
  overhead.

No bug identified. Overhead is pretty irrelevant.

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 ae4b06679..d1edd1d69 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -580,15 +580,15 @@ kvm_available ()
 		return 1
 
 	[ "$HOST" = "$ARCH_NAME" ] ||
-		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
-		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
+		{ [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ; } ||
+		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
 }
 
 hvf_available ()
 {
 	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
 	[ "$HOST" = "$ARCH_NAME" ] ||
-		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
+		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
 }
 
 set_qemu_accelerator ()
-- 
2.43.0

[kvm-unit-tests RFC PATCH 08/17] shellcheck: Fix SC2119, SC2120

[Nicholas Piggin]

  SC2119 (info): Use is_pv "$@" if function's $1 should mean script's
  $1.

  SC2120 (warning): is_pv references arguments, but none are ever
  passed.

Could be a bug?

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 s390x/run | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/s390x/run b/s390x/run
index e58fa4af9..34552c274 100755
--- a/s390x/run
+++ b/s390x/run
@@ -21,12 +21,12 @@ is_pv() {
 	return 1
 }
 
-if is_pv && [ "$ACCEL" = "tcg" ]; then
+if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then
 	echo "Protected Virtualization isn't supported under TCG"
 	exit 2
 fi
 
-if is_pv && [ "$MIGRATION" = "yes" ]; then
+if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then
 	echo "Migration isn't supported under Protected Virtualization"
 	exit 2
 fi
@@ -34,12 +34,12 @@ fi
 M='-machine s390-ccw-virtio'
 M+=",accel=$ACCEL$ACCEL_PROPS"
 
-if is_pv; then
+if is_pv "$@"; then
 	M+=",confidential-guest-support=pv0"
 fi
 
 command="$qemu -nodefaults -nographic $M"
-if is_pv; then
+if is_pv "$@"; then
 	command+=" -object s390-pv-guest,id=pv0"
 fi
 command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
-- 
2.43.0

[kvm-unit-tests RFC PATCH 09/17] shellcheck: Fix SC2143

[Nicholas Piggin]

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

Not a bug.

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 d1edd1d69..9dc34a54a 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

[kvm-unit-tests RFC PATCH 10/17] 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 9dc34a54a..e5750cb98 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

[kvm-unit-tests RFC PATCH 11/17] shellcheck: Fix SC2145

[Nicholas Piggin]

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

Could be a bug?

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

[kvm-unit-tests RFC PATCH 12/17] shellcheck: Fix SC2124

[Nicholas Piggin]

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

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 e5750cb98..89a496014 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

[kvm-unit-tests RFC PATCH 13/17] 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.

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 89a496014..ed440b4aa 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

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

[Nicholas Piggin]

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

Not sure if there's a real bug.

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

[kvm-unit-tests RFC PATCH 15/17] shellcheck: Fix SC2048

[Nicholas Piggin]

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

Not sure if there's a real bug.

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

[kvm-unit-tests RFC PATCH 16/17] shellcheck: Fix SC2153

[Nicholas Piggin]

  SC2153 (info): Possible misspelling: ACCEL may not be assigned. Did
  you mean accel?

Looks like a bug?

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

diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
index 6c75e89ae..fa47d0191 100644
--- a/scripts/s390x/func.bash
+++ b/scripts/s390x/func.bash
@@ -21,7 +21,7 @@ function arch_cmd_s390x()
 	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 
 	# run PV test case
-	if [ "$ACCEL" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
+	if [ "$accel" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
 		return
 	fi
 	kernel=${kernel%.elf}.pv.bin
-- 
2.43.0

[kvm-unit-tests RFC PATCH 17/17] 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>
---
 run_tests.sh            | 3 +++
 scripts/arch-run.bash   | 9 +++++++++
 scripts/mkstandalone.sh | 2 ++
 scripts/runtime.bash    | 2 ++
 4 files changed, 16 insertions(+)

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 ed440b4aa..fe8785cfd 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,8 @@ run_migration ()
 		-mon chardev=mon,mode=control \
 		< ${src_infifo} > ${src_outfifo} &
 	live_pid=$!
+	# SC complains about useless cat but I prefer it over redirect here.
+	# 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 +231,8 @@ do_migration ()
 		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
 		< ${dst_infifo} > ${dst_outfifo} &
 	incoming_pid=$!
+	# SC complains about useless cat but I prefer it over redirect here.
+	# shellcheck disable=SC2002
 	cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
 
 	# The test must prompt the user to migrate, so wait for the
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..c87613b96 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 allowed in the check parameter.
+        # shellcheck disable=SC2206
         check=($check)
         for check_param in "${check[@]}"; do
             path=${check_param%%=*}
-- 
2.43.0

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

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:32PM +1000, Nicholas Piggin wrote:
> I foolishly promised Andrew I would look into shellcheck, so here
> it is.

Thanks! I hope you only felt foolish since it was recently April
Fool's day, though.

> 
> https://gitlab.com/npiggin/kvm-unit-tests/-/tree/powerpc?ref_type=heads
> 
> This is on top of the "v8 migration, powerpc improvements" series. For
> now the patches are a bit raw but it does get down to zero[*] shellcheck
> warnings while still passing gitlab CI.
> 
> [*] Modulo the relatively few cases where they're disabled or
> suppressed.
> 
> I'd like comments about what should be enabled and disabled? There are
> quite a lot of options. Lots of changes don't fix real bugs AFAIKS, so
> there's some taste involved.

Yes, Bash is like that. We should probably eventually have a Bash style
guide as well as shellcheck and then tune shellcheck to the guide as
best we can.

> 
> Could possibly be a couple of bugs, including in s390x specific. Any
> review of those to confirm or deny bug is appreciated. I haven't tried
> to create reproducers for them.
> 
> I added a quick comment on each one whether it looks like a bug or
> harmless but I'm not a bash guru so could easily be wrong. I would
> possibly pull any real bug fixes to the front of the series and describe
> them as proper fix patches, and leave the other style / non-bugfixes in
> the brief format.  shellcheck has a very good wiki explaining each issue
> so there is not much point in rehashing that in the changelog.
> 
> One big thing kept disabled for now is the double-quoting to prevent
> globbing and splitting warning that is disabled. That touches a lot of
> code and we're very inconsistent about quoting variables today, but it's
> not completely trivial because there are quite a lot of places that does
> rely on splitting for invoking commands with arguments. That would need
> some rework to avoid sprinkling a lot of warning suppressions around.
> Possibly consistently using arrays for argument lists would be the best
> solution?

Yes, switching to arrays and using double-quoting would be good, but we
can leave it for follow-on work after a first round of shellcheck
integration.

Thanks,
drew

> 
> Thanks,
> Nick
> 
> Nicholas Piggin (17):
>   Add initial shellcheck checking
>   shellcheck: Fix SC2223
>   shellcheck: Fix SC2295
>   shellcheck: Fix SC2094
>   shellcheck: Fix SC2006
>   shellcheck: Fix SC2155
>   shellcheck: Fix SC2235
>   shellcheck: Fix SC2119, SC2120
>   shellcheck: Fix SC2143
>   shellcheck: Fix SC2013
>   shellcheck: Fix SC2145
>   shellcheck: Fix SC2124
>   shellcheck: Fix SC2294
>   shellcheck: Fix SC2178
>   shellcheck: Fix SC2048
>   shellcheck: Fix SC2153
>   shellcheck: Suppress various messages
> 
>  .shellcheckrc           | 32 +++++++++++++++++++++++++
>  Makefile                |  4 ++++
>  README.md               |  2 ++
>  arm/efi/run             |  4 ++--
>  riscv/efi/run           |  4 ++--
>  run_tests.sh            | 11 +++++----
>  s390x/run               |  8 +++----
>  scripts/arch-run.bash   | 52 ++++++++++++++++++++++++++++-------------
>  scripts/common.bash     |  5 +++-
>  scripts/mkstandalone.sh |  4 +++-
>  scripts/runtime.bash    | 14 +++++++----
>  scripts/s390x/func.bash |  2 +-
>  12 files changed, 106 insertions(+), 36 deletions(-)
>  create mode 100644 .shellcheckrc
> 
> -- 
> 2.43.0
> 
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:33PM +1000, Nicholas Piggin wrote:
> 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>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  .shellcheckrc       | 32 ++++++++++++++++++++++++++++++++
>  Makefile            |  4 ++++
>  README.md           |  2 ++
>  scripts/common.bash |  5 ++++-
>  4 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100644 .shellcheckrc
> 
> diff --git a/.shellcheckrc b/.shellcheckrc
> new file mode 100644
> index 000000000..2a9a57c42
> --- /dev/null
> +++ b/.shellcheckrc
> @@ -0,0 +1,32 @@
> +# 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

I vote keep disabled. The problem pointed out in the wiki can be handled
with ($a), similar to how one handles variables to C preprocessor macros.

> +
> +# Disable SC2034 - config.mak contains a lot of these unused variable errors.
> +# Maybe we could have a script extract the ones used by shell script and put
> +# them in a generated file, to re-enable the warning.
> +#
> +# In config.mak line 1:
> +# SRCDIR=/home/npiggin/src/kvm-unit-tests
> +# ^----^ SC2034 (warning): SRCDIR appears unused. Verify use (or export if used externally).
> +disable=SC2034

Maybe we should export everything in config.mak.

> +
> +# 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

Agreed. We can cross this bridge later.

> 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..77718675e 100644
> --- a/README.md
> +++ b/README.md
> @@ -193,3 +193,5 @@ 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
>

Other than the extension to the sentence in the README,

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

Thanks,
drew

Re: [kvm-unit-tests RFC PATCH 02/17] shellcheck: Fix SC2223

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:34PM +1000, Nicholas Piggin wrote:
>   SC2223 (info): This default assignment may cause DoS due to globbing.
>   Quote it.
> 
> 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

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

Re: [kvm-unit-tests RFC PATCH 03/17] shellcheck: Fix SC2295

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:35PM +1000, Nicholas Piggin wrote:
>   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.
> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 04/17] shellcheck: Fix SC2094

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:36PM +1000, Nicholas Piggin wrote:
>   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?
> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 05/17] shellcheck: Fix SC2006

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:37PM +1000, Nicholas Piggin wrote:
>   SC2006 (style): Use $(...) notation instead of legacy backticks `...`.
> 
> No bug identified.
> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 06/17] shellcheck: Fix SC2155

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:38PM +1000, Nicholas Piggin wrote:
>   SC2155 (warning): Declare and assign separately to avoid masking
>   return values.
> 
> No bug identified.
> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 07/17] shellcheck: Fix SC2235

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:39PM +1000, Nicholas Piggin wrote:
>   SC2235 (style): Use { ..; } instead of (..) to avoid subshell
>   overhead.
> 
> No bug identified. Overhead is pretty irrelevant.
> 
> 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 ae4b06679..d1edd1d69 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -580,15 +580,15 @@ kvm_available ()
>  		return 1
>  
>  	[ "$HOST" = "$ARCH_NAME" ] ||
> -		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
> -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> +		{ [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ; } ||
> +		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
>  }
>  
>  hvf_available ()
>  {
>  	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
>  	[ "$HOST" = "$ARCH_NAME" ] ||
> -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> +		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
>  }

This one is a bit ugly. Most developers are used to seeing expressions
like

  a || (b && c)

not 

 a || { b && c ; }

I'm inclined to add SC2235 to the ignore list...

Thanks,
drew

Re: [kvm-unit-tests RFC PATCH 08/17] shellcheck: Fix SC2119, SC2120

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:40PM +1000, Nicholas Piggin wrote:
>   SC2119 (info): Use is_pv "$@" if function's $1 should mean script's
>   $1.
> 
>   SC2120 (warning): is_pv references arguments, but none are ever
>   passed.
> 
> Could be a bug?

Looks like it to me.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  s390x/run | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/s390x/run b/s390x/run
> index e58fa4af9..34552c274 100755
> --- a/s390x/run
> +++ b/s390x/run
> @@ -21,12 +21,12 @@ is_pv() {
>  	return 1
>  }
>  
> -if is_pv && [ "$ACCEL" = "tcg" ]; then
> +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then
>  	echo "Protected Virtualization isn't supported under TCG"
>  	exit 2
>  fi
>  
> -if is_pv && [ "$MIGRATION" = "yes" ]; then
> +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then
>  	echo "Migration isn't supported under Protected Virtualization"
>  	exit 2
>  fi
> @@ -34,12 +34,12 @@ fi
>  M='-machine s390-ccw-virtio'
>  M+=",accel=$ACCEL$ACCEL_PROPS"
>  
> -if is_pv; then
> +if is_pv "$@"; then
>  	M+=",confidential-guest-support=pv0"
>  fi
>  
>  command="$qemu -nodefaults -nographic $M"
> -if is_pv; then
> +if is_pv "$@"; then
>  	command+=" -object s390-pv-guest,id=pv0"
>  fi
>  command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"
> -- 
> 2.43.0
>

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

Re: [kvm-unit-tests RFC PATCH 09/17] shellcheck: Fix SC2143

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:41PM +1000, Nicholas Piggin wrote:
>   SC2143 (style): Use ! grep -q instead of comparing output with
>   [ -z .. ].
> 
> Not a bug.
> 
> 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 d1edd1d69..9dc34a54a 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
>

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

Re: [kvm-unit-tests RFC PATCH 10/17] shellcheck: Fix SC2013

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:42PM +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 9dc34a54a..e5750cb98 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
>

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

Re: [kvm-unit-tests RFC PATCH 11/17] shellcheck: Fix SC2145

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:43PM +1000, Nicholas Piggin wrote:
>   SC2145 (error): Argument mixes string and array. Use * or separate
>   argument.
> 
> Could be a bug?

I don't think so, since the preceding string ends with a space and there
aren't any succeeding strings. Anyway, it's good to switch to *

> 
> 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
> 
>

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

Re: [kvm-unit-tests RFC PATCH 12/17] shellcheck: Fix SC2124

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:44PM +1000, Nicholas Piggin wrote:
>   SC2124 (warning): Assigning an array to a string! Assign as array, or
>   use * instead of @ to concatenate.
> 
> 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 e5750cb98..89a496014 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
>

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

Re: [kvm-unit-tests RFC PATCH 13/17] shellcheck: Fix SC2294

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:45PM +1000, Nicholas Piggin wrote:
>   SC2294 (warning): eval negates the benefit of arrays. Drop eval to
>   preserve whitespace/symbols (or eval as string).
> 
> No bug identified.
> 
> 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 89a496014..ed440b4aa 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
>

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

Re: [kvm-unit-tests RFC PATCH 14/17] shellcheck: Fix SC2178

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:46PM +1000, Nicholas Piggin wrote:
>   SC2178 (warning): Variable was used as an array but is now assigned a
>   string.
> 
> Not sure if there's a real bug.

Things seem to work, but the change looks better.

> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 15/17] shellcheck: Fix SC2048

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:47PM +1000, Nicholas Piggin wrote:
>   SC2048 (warning): Use "$@" (with quotes) to prevent whitespace
>   problems.
> 
> Not sure if there's a real bug.
> 
> 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
>

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

Re: [kvm-unit-tests RFC PATCH 16/17] shellcheck: Fix SC2153

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:48PM +1000, Nicholas Piggin wrote:
>   SC2153 (info): Possible misspelling: ACCEL may not be assigned. Did
>   you mean accel?
> 
> Looks like a bug?

Agreed.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  scripts/s390x/func.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/s390x/func.bash b/scripts/s390x/func.bash
> index 6c75e89ae..fa47d0191 100644
> --- a/scripts/s390x/func.bash
> +++ b/scripts/s390x/func.bash
> @@ -21,7 +21,7 @@ function arch_cmd_s390x()
>  	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
>  
>  	# run PV test case
> -	if [ "$ACCEL" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
> +	if [ "$accel" = 'tcg' ] || grep -q "migration" <<< "$groups"; then
>  		return
>  	fi
>  	kernel=${kernel%.elf}.pv.bin
> -- 
> 2.43.0
>

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

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

[Andrew Jones]

On Fri, Apr 05, 2024 at 07:00:49PM +1000, Nicholas Piggin wrote:
> Various info and warnings are suppressed here, where circumstances
> (commented) warrant.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  run_tests.sh            | 3 +++
>  scripts/arch-run.bash   | 9 +++++++++
>  scripts/mkstandalone.sh | 2 ++
>  scripts/runtime.bash    | 2 ++
>  4 files changed, 16 insertions(+)
> 
> 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 ed440b4aa..fe8785cfd 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,8 @@ run_migration ()
>  		-mon chardev=mon,mode=control \
>  		< ${src_infifo} > ${src_outfifo} &
>  	live_pid=$!
> +	# SC complains about useless cat but I prefer it over redirect here.

Let's spell out 'shellcheck' when referring to it rather than call it
'SC'. And instead of "but I prefer..." let's write

 # shellcheck complains about a useless cat, but using a redirect here is
 # harder to read

or something like that. Don't tell my cat-loving daughter that I just
wrote "a useless cat"!


> +	# 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 +231,8 @@ do_migration ()
>  		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
>  		< ${dst_infifo} > ${dst_outfifo} &
>  	incoming_pid=$!
> +	# SC complains about useless cat but I prefer it over redirect here.

Same comment as above.

> +	# shellcheck disable=SC2002
>  	cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
>  
>  	# The test must prompt the user to migrate, so wait for the
> 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..c87613b96 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 allowed in the check parameter.
> +        # shellcheck disable=SC2206
>          check=($check)

Hmm, I'm not sure about this one. $check is an arbitrary path, which means
it can have spaces, then =, and then an arbitrary value, which means it can
contain spaces. If there are multiple check path=value pairs then
separation by space is a bad idea, and any deliminator really is. It seems
like each pair should be quoted, i.e.

 check = "path1=value1" "path2=value2"

and then that should be managed here.

>          for check_param in "${check[@]}"; do
>              path=${check_param%%=*}
> -- 
> 2.43.0
>

Thanks,
drew

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

[Nicholas Piggin]

On Sat Apr 6, 2024 at 12:55 AM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:49PM +1000, Nicholas Piggin wrote:
> > Various info and warnings are suppressed here, where circumstances
> > (commented) warrant.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  run_tests.sh            | 3 +++
> >  scripts/arch-run.bash   | 9 +++++++++
> >  scripts/mkstandalone.sh | 2 ++
> >  scripts/runtime.bash    | 2 ++
> >  4 files changed, 16 insertions(+)
> > 
> > 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 ed440b4aa..fe8785cfd 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,8 @@ run_migration ()
> >  		-mon chardev=mon,mode=control \
> >  		< ${src_infifo} > ${src_outfifo} &
> >  	live_pid=$!
> > +	# SC complains about useless cat but I prefer it over redirect here.
>
> Let's spell out 'shellcheck' when referring to it rather than call it
> 'SC'. And instead of "but I prefer..." let's write
>
>  # shellcheck complains about a useless cat, but using a redirect here is
>  # harder to read

Sounds good, will do.

>
> or something like that. Don't tell my cat-loving daughter that I just
> wrote "a useless cat"!

:D

>
>
> > +	# 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 +231,8 @@ do_migration ()
> >  		-mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
> >  		< ${dst_infifo} > ${dst_outfifo} &
> >  	incoming_pid=$!
> > +	# SC complains about useless cat but I prefer it over redirect here.
>
> Same comment as above.
>
> > +	# shellcheck disable=SC2002
> >  	cat ${dst_outfifo} | tee ${dst_out} | filter_quiet_msgs &
> >  
> >  	# The test must prompt the user to migrate, so wait for the
> > 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..c87613b96 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 allowed in the check parameter.
> > +        # shellcheck disable=SC2206
> >          check=($check)
>
> Hmm, I'm not sure about this one. $check is an arbitrary path, which means
> it can have spaces, then =, and then an arbitrary value, which means it can
> contain spaces. If there are multiple check path=value pairs then
> separation by space is a bad idea, and any deliminator really is. It seems
> like each pair should be quoted, i.e.
>
>  check = "path1=value1" "path2=value2"
>
> and then that should be managed here.

Yeah I did think of that, valid paths could also have = and ", and even
with double quotes it seems to be tricky to handle spaces.

Maybe I'll just add to the unittest.cfg docs to stick with alphanumeric
paths, and we can improve it if someone complains?

Thanks,
Nick

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

[Nicholas Piggin]

On Fri Apr 5, 2024 at 11:59 PM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:32PM +1000, Nicholas Piggin wrote:
> > I foolishly promised Andrew I would look into shellcheck, so here
> > it is.
>
> Thanks! I hope you only felt foolish since it was recently April
> Fool's day, though.

Hah, no it was fine, it was a good idea and I've been mucking with
a lot of the bash so, no worries.

>
> > 
> > https://gitlab.com/npiggin/kvm-unit-tests/-/tree/powerpc?ref_type=heads
> > 
> > This is on top of the "v8 migration, powerpc improvements" series. For
> > now the patches are a bit raw but it does get down to zero[*] shellcheck
> > warnings while still passing gitlab CI.
> > 
> > [*] Modulo the relatively few cases where they're disabled or
> > suppressed.
> > 
> > I'd like comments about what should be enabled and disabled? There are
> > quite a lot of options. Lots of changes don't fix real bugs AFAIKS, so
> > there's some taste involved.
>
> Yes, Bash is like that. We should probably eventually have a Bash style
> guide as well as shellcheck and then tune shellcheck to the guide as
> best we can.

+1

>
> > 
> > Could possibly be a couple of bugs, including in s390x specific. Any
> > review of those to confirm or deny bug is appreciated. I haven't tried
> > to create reproducers for them.
> > 
> > I added a quick comment on each one whether it looks like a bug or
> > harmless but I'm not a bash guru so could easily be wrong. I would
> > possibly pull any real bug fixes to the front of the series and describe
> > them as proper fix patches, and leave the other style / non-bugfixes in
> > the brief format.  shellcheck has a very good wiki explaining each issue
> > so there is not much point in rehashing that in the changelog.
> > 
> > One big thing kept disabled for now is the double-quoting to prevent
> > globbing and splitting warning that is disabled. That touches a lot of
> > code and we're very inconsistent about quoting variables today, but it's
> > not completely trivial because there are quite a lot of places that does
> > rely on splitting for invoking commands with arguments. That would need
> > some rework to avoid sprinkling a lot of warning suppressions around.
> > Possibly consistently using arrays for argument lists would be the best
> > solution?
>
> Yes, switching to arrays and using double-quoting would be good, but we
> can leave it for follow-on work after a first round of shellcheck
> integration.

Okay good, thanks for all the review on it.

Thanks,
Nick

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

[Nicholas Piggin]

On Sat Apr 6, 2024 at 12:12 AM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:33PM +1000, Nicholas Piggin wrote:
> > 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>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  .shellcheckrc       | 32 ++++++++++++++++++++++++++++++++
> >  Makefile            |  4 ++++
> >  README.md           |  2 ++
> >  scripts/common.bash |  5 ++++-
> >  4 files changed, 42 insertions(+), 1 deletion(-)
> >  create mode 100644 .shellcheckrc
> > 
> > diff --git a/.shellcheckrc b/.shellcheckrc
> > new file mode 100644
> > index 000000000..2a9a57c42
> > --- /dev/null
> > +++ b/.shellcheckrc
> > @@ -0,0 +1,32 @@
> > +# 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
>
> I vote keep disabled. The problem pointed out in the wiki can be handled
> with ($a), similar to how one handles variables to C preprocessor macros.
>
> > +
> > +# Disable SC2034 - config.mak contains a lot of these unused variable errors.
> > +# Maybe we could have a script extract the ones used by shell script and put
> > +# them in a generated file, to re-enable the warning.
> > +#
> > +# In config.mak line 1:
> > +# SRCDIR=/home/npiggin/src/kvm-unit-tests
> > +# ^----^ SC2034 (warning): SRCDIR appears unused. Verify use (or export if used externally).
> > +disable=SC2034
>
> Maybe we should export everything in config.mak.

It would be nice to enable the warning.... Oh, actually it looks like
we can disable a warning for an entire file by adding the disable
directive at the top. I didn't notice that before, I'll try that first.

>
> > +
> > +# 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
>
> Agreed. We can cross this bridge later.
>
> > 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..77718675e 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -193,3 +193,5 @@ 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.

Yeah good point.

Thanks,
Nick

>
>
> > 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
> >
>
> Other than the extension to the sentence in the README,
>
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
>
> Thanks,
> drew

Re: [kvm-unit-tests RFC PATCH 07/17] shellcheck: Fix SC2235

[Nicholas Piggin]

On Sat Apr 6, 2024 at 12:24 AM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:39PM +1000, Nicholas Piggin wrote:
> >   SC2235 (style): Use { ..; } instead of (..) to avoid subshell
> >   overhead.
> > 
> > No bug identified. Overhead is pretty irrelevant.
> > 
> > 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 ae4b06679..d1edd1d69 100644
> > --- a/scripts/arch-run.bash
> > +++ b/scripts/arch-run.bash
> > @@ -580,15 +580,15 @@ kvm_available ()
> >  		return 1
> >  
> >  	[ "$HOST" = "$ARCH_NAME" ] ||
> > -		( [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ) ||
> > -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> > +		{ [ "$HOST" = aarch64 ] && [ "$ARCH" = arm ] ; } ||
> > +		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
> >  }
> >  
> >  hvf_available ()
> >  {
> >  	[ "$(sysctl -n kern.hv_support 2>/dev/null)" = "1" ] || return 1
> >  	[ "$HOST" = "$ARCH_NAME" ] ||
> > -		( [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] )
> > +		{ [ "$HOST" = x86_64 ] && [ "$ARCH" = i386 ] ; }
> >  }
>
> This one is a bit ugly. Most developers are used to seeing expressions
> like
>
>   a || (b && c)
>
> not 
>
>  a || { b && c ; }
>
> I'm inclined to add SC2235 to the ignore list...

I'm sure it's a good warning for bash scripts that do a lot of
computation and have these in hot loops. But I _think_ none of
our scripts are like that so I would be okay with disabling
the warning. I'll do that unless someone disagrees.

Thanks,
Nick

Re: [kvm-unit-tests RFC PATCH 11/17] shellcheck: Fix SC2145

[Nicholas Piggin]

On Sat Apr 6, 2024 at 12:35 AM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:43PM +1000, Nicholas Piggin wrote:
> >   SC2145 (error): Argument mixes string and array. Use * or separate
> >   argument.
> > 
> > Could be a bug?
>
> I don't think so, since the preceding string ends with a space and there
> aren't any succeeding strings. Anyway, it's good to switch to *

Okay I think you're right.

Thanks,
Nick

>
> > 
> > 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
> > 
> >
>
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

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

[Andrew Jones]

On Sat, Apr 06, 2024 at 04:31:17PM +1000, Nicholas Piggin wrote:
...
> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > index 3b76aec9e..c87613b96 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 allowed in the check parameter.
> > > +        # shellcheck disable=SC2206
> > >          check=($check)
> >
> > Hmm, I'm not sure about this one. $check is an arbitrary path, which means
> > it can have spaces, then =, and then an arbitrary value, which means it can
> > contain spaces. If there are multiple check path=value pairs then
> > separation by space is a bad idea, and any deliminator really is. It seems
> > like each pair should be quoted, i.e.
> >
> >  check = "path1=value1" "path2=value2"
> >
> > and then that should be managed here.
> 
> Yeah I did think of that, valid paths could also have = and ", and even
> with double quotes it seems to be tricky to handle spaces.
> 
> Maybe I'll just add to the unittest.cfg docs to stick with alphanumeric
> paths, and we can improve it if someone complains?

Works for me.

Thanks,
drew

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

[Nicholas Piggin]

On Sat Apr 6, 2024 at 12:55 AM AEST, Andrew Jones wrote:
> On Fri, Apr 05, 2024 at 07:00:49PM +1000, Nicholas Piggin wrote:
> > Various info and warnings are suppressed here, where circumstances
> > (commented) warrant.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  run_tests.sh            | 3 +++
> >  scripts/arch-run.bash   | 9 +++++++++
> >  scripts/mkstandalone.sh | 2 ++
> >  scripts/runtime.bash    | 2 ++
> >  4 files changed, 16 insertions(+)
> > 
> > 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 ed440b4aa..fe8785cfd 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,8 @@ run_migration ()
> >  		-mon chardev=mon,mode=control \
> >  		< ${src_infifo} > ${src_outfifo} &
> >  	live_pid=$!
> > +	# SC complains about useless cat but I prefer it over redirect here.
>
> Let's spell out 'shellcheck' when referring to it rather than call it
> 'SC'. And instead of "but I prefer..." let's write

Okay I got reminded why I did this, because starting # shellcheck blah
makes shellcheck try to parse it and fails. We could use # * shellcheck
for comments?

Thanks,
Nick