[kvm-unit-tests PATCH v3 0/5] add shellcheck support

[kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Nicholas Piggin]

This is based on upstream directly now, not ahead of the powerpc
series.

Since v2:
- Rebased to upstream with some patches merged.
- Just a few comment typos and small issues (e.g., quoting
  `make shellcheck` in docs) that people picked up from the
  last round.

`make shellcheck` passes with no warnings for me after this series.
(You should be able to put patch 1 at the end without conflicts if
you prefer to only introduce the shellcheck Makefile target when the
tree is clean.)

Thanks,
Nick

Nicholas Piggin (5):
  Add initial shellcheck checking
  shellcheck: Fix SC2155
  shellcheck: Fix SC2124
  shellcheck: Fix SC2294
  shellcheck: Suppress various messages

 .shellcheckrc           | 30 ++++++++++++++++++++++++++++++
 Makefile                |  4 ++++
 README.md               |  3 +++
 configure               |  2 ++
 run_tests.sh            |  3 +++
 scripts/arch-run.bash   | 33 ++++++++++++++++++++++++++-------
 scripts/common.bash     |  5 ++++-
 scripts/mkstandalone.sh |  2 ++
 scripts/runtime.bash    |  6 +++++-
 9 files changed, 79 insertions(+), 9 deletions(-)
 create mode 100644 .shellcheckrc

-- 
2.43.0

[kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking

[Nicholas Piggin]

This adds a basic shellcheck style 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 4f35fffc6..6240d8dfa 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..2d6f7db56 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 b9413d683..5e9ad53e2 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -78,8 +78,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 PATCH v3 2/5] 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 2ac7b0b84..45ec8f57d 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -415,7 +415,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
@@ -427,7 +428,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 $?
@@ -570,7 +572,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 e7af9bda9..597c90991 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 "could not \(load\|open\) kernel" -e "error loading" -e "failed to load" &&
-- 
2.43.0

[kvm-unit-tests PATCH v3 3/5] 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

[kvm-unit-tests PATCH v3 4/5] 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

[kvm-unit-tests PATCH v3 5/5] shellcheck: Suppress various messages

[Nicholas Piggin]

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

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
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 49f047cb2..e13a346d3 100755
--- a/configure
+++ b/configure
@@ -422,6 +422,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..8643bab3b 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)"
+		# Shellcheck does not see 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 see 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 597c90991..fb7c83a25 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -127,6 +127,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: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Andrew Jones]

On Wed, May 01, 2024 at 09:29:29PM GMT, Nicholas Piggin wrote:
> This is based on upstream directly now, not ahead of the powerpc
> series.
> 
> Since v2:
> - Rebased to upstream with some patches merged.
> - Just a few comment typos and small issues (e.g., quoting
>   `make shellcheck` in docs) that people picked up from the
>   last round.
> 
> `make shellcheck` passes with no warnings for me after this series.
> (You should be able to put patch 1 at the end without conflicts if
> you prefer to only introduce the shellcheck Makefile target when the
> tree is clean.)
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (5):
>   Add initial shellcheck checking
>   shellcheck: Fix SC2155
>   shellcheck: Fix SC2124
>   shellcheck: Fix SC2294
>   shellcheck: Suppress various messages
> 
>  .shellcheckrc           | 30 ++++++++++++++++++++++++++++++
>  Makefile                |  4 ++++
>  README.md               |  3 +++
>  configure               |  2 ++
>  run_tests.sh            |  3 +++
>  scripts/arch-run.bash   | 33 ++++++++++++++++++++++++++-------
>  scripts/common.bash     |  5 ++++-
>  scripts/mkstandalone.sh |  2 ++
>  scripts/runtime.bash    |  6 +++++-
>  9 files changed, 79 insertions(+), 9 deletions(-)
>  create mode 100644 .shellcheckrc
> 
> -- 
> 2.43.0
>

Merged.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Thomas Huth]

On 01/05/2024 13.29, Nicholas Piggin wrote:
> This is based on upstream directly now, not ahead of the powerpc
> series.

Thanks! ... maybe you could also rebase the powerpc series on this now? (I 
haven't forgotten about it, just did not find enough spare time for more 
reviewing yet)

> Since v2:
> - Rebased to upstream with some patches merged.
> - Just a few comment typos and small issues (e.g., quoting
>    `make shellcheck` in docs) that people picked up from the
>    last round.

When I now run "make shellcheck", I'm still getting an error:

In config.mak line 16:
AR=ar
^-- SC2209 (warning): Use var=$(command) to assign output (or quote to 
assign string).

Not sure why it's complaining about "ar" but not about the other lines in there?

Also, it only seems to work for in-tree builds. If I run it from an 
out-of-tree build directory, I get:

*/efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory)

  Thomas

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Andrew Jones]

On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
> On 01/05/2024 13.29, Nicholas Piggin wrote:
> > This is based on upstream directly now, not ahead of the powerpc
> > series.
> 
> Thanks! ... maybe you could also rebase the powerpc series on this now? (I
> haven't forgotten about it, just did not find enough spare time for more
> reviewing yet)
> 
> > Since v2:
> > - Rebased to upstream with some patches merged.
> > - Just a few comment typos and small issues (e.g., quoting
> >    `make shellcheck` in docs) that people picked up from the
> >    last round.
> 
> When I now run "make shellcheck", I'm still getting an error:
> 
> In config.mak line 16:
> AR=ar
> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
> assign string).

I didn't see this one when testing. I have shellcheck version 0.9.0.

> 
> Not sure why it's complaining about "ar" but not about the other lines in there?
> 
> Also, it only seems to work for in-tree builds. If I run it from an
> out-of-tree build directory, I get:
> 
> */efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory)

I'm glad you checked this. I wish I had before merging :-/

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Thomas Huth]

On 02/05/2024 10.56, Andrew Jones wrote:
> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
>> On 01/05/2024 13.29, Nicholas Piggin wrote:
>>> This is based on upstream directly now, not ahead of the powerpc
>>> series.
>>
>> Thanks! ... maybe you could also rebase the powerpc series on this now? (I
>> haven't forgotten about it, just did not find enough spare time for more
>> reviewing yet)
>>
>>> Since v2:
>>> - Rebased to upstream with some patches merged.
>>> - Just a few comment typos and small issues (e.g., quoting
>>>     `make shellcheck` in docs) that people picked up from the
>>>     last round.
>>
>> When I now run "make shellcheck", I'm still getting an error:
>>
>> In config.mak line 16:
>> AR=ar
>> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
>> assign string).
> 
> I didn't see this one when testing. I have shellcheck version 0.9.0.

I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config?

Anyway, I'm in favor of turning this warning of in the config file, it does 
not seem to be really helpful in my eyes. What do you think?

  Thomas

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Andrew Jones]

On Thu, May 02, 2024 at 11:34:24AM GMT, Thomas Huth wrote:
> On 02/05/2024 10.56, Andrew Jones wrote:
> > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
> > > On 01/05/2024 13.29, Nicholas Piggin wrote:
> > > > This is based on upstream directly now, not ahead of the powerpc
> > > > series.
> > > 
> > > Thanks! ... maybe you could also rebase the powerpc series on this now? (I
> > > haven't forgotten about it, just did not find enough spare time for more
> > > reviewing yet)
> > > 
> > > > Since v2:
> > > > - Rebased to upstream with some patches merged.
> > > > - Just a few comment typos and small issues (e.g., quoting
> > > >     `make shellcheck` in docs) that people picked up from the
> > > >     last round.
> > > 
> > > When I now run "make shellcheck", I'm still getting an error:
> > > 
> > > In config.mak line 16:
> > > AR=ar
> > > ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
> > > assign string).
> > 
> > I didn't see this one when testing. I have shellcheck version 0.9.0.
> 
> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config?

Yeah, I tested with AArch64, which sets AR to aarch64-linux-gnu-ar. I just
tried x86 and see the warning.

> 
> Anyway, I'm in favor of turning this warning of in the config file, it does
> not seem to be really helpful in my eyes. What do you think?

I agree. The 2209 description says this warning will only appear for
commands that are in a hard coded list, so it's an odd check anyway.

Thanks,
drew

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Nicholas Piggin]

On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote:
> On 02/05/2024 10.56, Andrew Jones wrote:
> > On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
> >> On 01/05/2024 13.29, Nicholas Piggin wrote:
> >>> This is based on upstream directly now, not ahead of the powerpc
> >>> series.
> >>
> >> Thanks! ... maybe you could also rebase the powerpc series on this now? (I
> >> haven't forgotten about it, just did not find enough spare time for more
> >> reviewing yet)
> >>
> >>> Since v2:
> >>> - Rebased to upstream with some patches merged.
> >>> - Just a few comment typos and small issues (e.g., quoting
> >>>     `make shellcheck` in docs) that people picked up from the
> >>>     last round.
> >>
> >> When I now run "make shellcheck", I'm still getting an error:
> >>
> >> In config.mak line 16:
> >> AR=ar
> >> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
> >> assign string).
> > 
> > I didn't see this one when testing. I have shellcheck version 0.9.0.
>
> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config?

I have 0.10.0 from Debian with no changes to config defaults and no
warning.

> Anyway, I'm in favor of turning this warning of in the config file, it does 
> not seem to be really helpful in my eyes. What do you think?

Maybe it would be useful. I don't mind quoting strings usually, although
for this kind of pattern it's a bit pointless and config.mak is also
Makefile so that has its own issues. Maybe just disable it for this
file?

Thanks,
Nick

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Thomas Huth]

On 03/05/2024 07.02, Nicholas Piggin wrote:
> On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote:
>> On 02/05/2024 10.56, Andrew Jones wrote:
>>> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
>>>> On 01/05/2024 13.29, Nicholas Piggin wrote:
>>>>> This is based on upstream directly now, not ahead of the powerpc
>>>>> series.
>>>>
>>>> Thanks! ... maybe you could also rebase the powerpc series on this now? (I
>>>> haven't forgotten about it, just did not find enough spare time for more
>>>> reviewing yet)
>>>>
>>>>> Since v2:
>>>>> - Rebased to upstream with some patches merged.
>>>>> - Just a few comment typos and small issues (e.g., quoting
>>>>>      `make shellcheck` in docs) that people picked up from the
>>>>>      last round.
>>>>
>>>> When I now run "make shellcheck", I'm still getting an error:
>>>>
>>>> In config.mak line 16:
>>>> AR=ar
>>>> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
>>>> assign string).
>>>
>>> I didn't see this one when testing. I have shellcheck version 0.9.0.
>>
>> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config?
> 
> I have 0.10.0 from Debian with no changes to config defaults and no
> warning.

If I understood it correctly, it warns for AR=ar but it does not warn for 
AR=powerpc-linux-gnu-ar ... could you try the first term, too, if you 
haven't done so yet?

>> Anyway, I'm in favor of turning this warning of in the config file, it does
>> not seem to be really helpful in my eyes. What do you think?
> 
> Maybe it would be useful. I don't mind quoting strings usually, although
> for this kind of pattern it's a bit pointless and config.mak is also
> Makefile so that has its own issues. Maybe just disable it for this
> file?

Yes, either for this file only, or globally ... I don't mind. Could you send 
a patch, please?

  Thanks,
   Thomas

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Nicholas Piggin]

On Fri May 3, 2024 at 3:13 PM AEST, Thomas Huth wrote:
> On 03/05/2024 07.02, Nicholas Piggin wrote:
> > On Thu May 2, 2024 at 7:34 PM AEST, Thomas Huth wrote:
> >> On 02/05/2024 10.56, Andrew Jones wrote:
> >>> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
> >>>> On 01/05/2024 13.29, Nicholas Piggin wrote:
> >>>>> This is based on upstream directly now, not ahead of the powerpc
> >>>>> series.
> >>>>
> >>>> Thanks! ... maybe you could also rebase the powerpc series on this now? (I
> >>>> haven't forgotten about it, just did not find enough spare time for more
> >>>> reviewing yet)
> >>>>
> >>>>> Since v2:
> >>>>> - Rebased to upstream with some patches merged.
> >>>>> - Just a few comment typos and small issues (e.g., quoting
> >>>>>      `make shellcheck` in docs) that people picked up from the
> >>>>>      last round.
> >>>>
> >>>> When I now run "make shellcheck", I'm still getting an error:
> >>>>
> >>>> In config.mak line 16:
> >>>> AR=ar
> >>>> ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
> >>>> assign string).
> >>>
> >>> I didn't see this one when testing. I have shellcheck version 0.9.0.
> >>
> >> I'm also using 0.9.0 (from Fedora). Maybe we've got a different default config?
> > 
> > I have 0.10.0 from Debian with no changes to config defaults and no
> > warning.
>
> If I understood it correctly, it warns for AR=ar but it does not warn for 
> AR=powerpc-linux-gnu-ar ... could you try the first term, too, if you 
> haven't done so yet?

Ah you're right, that does warn here too.

Thanks,
Nick

Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support

[Nicholas Piggin]

On Thu May 2, 2024 at 6:56 PM AEST, Andrew Jones wrote:
> On Thu, May 02, 2024 at 10:23:22AM GMT, Thomas Huth wrote:
> > On 01/05/2024 13.29, Nicholas Piggin wrote:
> > > This is based on upstream directly now, not ahead of the powerpc
> > > series.
> > 
> > Thanks! ... maybe you could also rebase the powerpc series on this now? (I
> > haven't forgotten about it, just did not find enough spare time for more
> > reviewing yet)
> > 
> > > Since v2:
> > > - Rebased to upstream with some patches merged.
> > > - Just a few comment typos and small issues (e.g., quoting
> > >    `make shellcheck` in docs) that people picked up from the
> > >    last round.
> > 
> > When I now run "make shellcheck", I'm still getting an error:
> > 
> > In config.mak line 16:
> > AR=ar
> > ^-- SC2209 (warning): Use var=$(command) to assign output (or quote to
> > assign string).
>
> I didn't see this one when testing. I have shellcheck version 0.9.0.
>
> > 
> > Not sure why it's complaining about "ar" but not about the other lines in there?
> > 
> > Also, it only seems to work for in-tree builds. If I run it from an
> > out-of-tree build directory, I get:
> > 
> > */efi/run: */efi/run: openBinaryFile: does not exist (No such file or directory)
>
> I'm glad you checked this. I wish I had before merging :-/

I'll send a patch. Possibly some other targets (check-kerneldoc, ctags?)
may not work out of tree either. I'll fix this one up first though.

Thanks,
Nick