* [kvm-unit-tests PATCH v3 0/5] add shellcheck support
@ 2024-05-01 11:29 Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking Nicholas Piggin
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
@ 2024-05-01 11:29 ` Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 2/5] shellcheck: Fix SC2155 Nicholas Piggin
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH v3 2/5] shellcheck: Fix SC2155
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking Nicholas Piggin
@ 2024-05-01 11:29 ` Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 3/5] shellcheck: Fix SC2124 Nicholas Piggin
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH v3 3/5] shellcheck: Fix SC2124
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 2/5] shellcheck: Fix SC2155 Nicholas Piggin
@ 2024-05-01 11:29 ` Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 4/5] shellcheck: Fix SC2294 Nicholas Piggin
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH v3 4/5] shellcheck: Fix SC2294
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
` (2 preceding siblings ...)
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 3/5] shellcheck: Fix SC2124 Nicholas Piggin
@ 2024-05-01 11:29 ` Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 5/5] shellcheck: Suppress various messages Nicholas Piggin
` (2 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [kvm-unit-tests PATCH v3 5/5] shellcheck: Suppress various messages
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
` (3 preceding siblings ...)
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 4/5] shellcheck: Fix SC2294 Nicholas Piggin
@ 2024-05-01 11:29 ` Nicholas Piggin
2024-05-02 8:12 ` [kvm-unit-tests PATCH v3 0/5] add shellcheck support Andrew Jones
2024-05-02 8:23 ` Thomas Huth
6 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-01 11:29 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Thomas Huth, Alexandru Elisei,
Eric Auger, Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
` (4 preceding siblings ...)
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 5/5] shellcheck: Suppress various messages Nicholas Piggin
@ 2024-05-02 8:12 ` Andrew Jones
2024-05-02 8:23 ` Thomas Huth
6 siblings, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2024-05-02 8:12 UTC (permalink / raw
To: Nicholas Piggin
Cc: Paolo Bonzini, Thomas Huth, Alexandru Elisei, Eric Auger,
Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
` (5 preceding siblings ...)
2024-05-02 8:12 ` [kvm-unit-tests PATCH v3 0/5] add shellcheck support Andrew Jones
@ 2024-05-02 8:23 ` Thomas Huth
2024-05-02 8:56 ` Andrew Jones
6 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2024-05-02 8:23 UTC (permalink / raw
To: Nicholas Piggin, Andrew Jones
Cc: Paolo Bonzini, Alexandru Elisei, Eric Auger, Janosch Frank,
Claudio Imbrenda, Nico Böhr, David Hildenbrand,
Shaoqin Huang, Nikos Nikoleris, David Woodhouse, Ricardo Koller,
rminmin, Gavin Shan, Nina Schoetterl-Glausch, Sean Christopherson,
kvm, kvmarm, kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-02 8:23 ` Thomas Huth
@ 2024-05-02 8:56 ` Andrew Jones
2024-05-02 9:34 ` Thomas Huth
2024-05-03 5:11 ` Nicholas Piggin
0 siblings, 2 replies; 15+ messages in thread
From: Andrew Jones @ 2024-05-02 8:56 UTC (permalink / raw
To: Thomas Huth
Cc: Nicholas Piggin, Paolo Bonzini, Alexandru Elisei, Eric Auger,
Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-02 8:56 ` Andrew Jones
@ 2024-05-02 9:34 ` Thomas Huth
2024-05-02 9:48 ` Andrew Jones
2024-05-03 5:02 ` Nicholas Piggin
2024-05-03 5:11 ` Nicholas Piggin
1 sibling, 2 replies; 15+ messages in thread
From: Thomas Huth @ 2024-05-02 9:34 UTC (permalink / raw
To: Andrew Jones
Cc: Nicholas Piggin, Paolo Bonzini, Alexandru Elisei, Eric Auger,
Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-02 9:34 ` Thomas Huth
@ 2024-05-02 9:48 ` Andrew Jones
2024-05-03 5:02 ` Nicholas Piggin
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Jones @ 2024-05-02 9:48 UTC (permalink / raw
To: Thomas Huth
Cc: Nicholas Piggin, Paolo Bonzini, Alexandru Elisei, Eric Auger,
Janosch Frank, Claudio Imbrenda, Nico Böhr,
David Hildenbrand, Shaoqin Huang, Nikos Nikoleris,
David Woodhouse, Ricardo Koller, rminmin, Gavin Shan,
Nina Schoetterl-Glausch, Sean Christopherson, kvm, kvmarm,
kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-02 9:34 ` Thomas Huth
2024-05-02 9:48 ` Andrew Jones
@ 2024-05-03 5:02 ` Nicholas Piggin
2024-05-03 5:13 ` Thomas Huth
1 sibling, 1 reply; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-03 5:02 UTC (permalink / raw
To: Thomas Huth, Andrew Jones
Cc: Paolo Bonzini, Alexandru Elisei, Eric Auger, Janosch Frank,
Claudio Imbrenda, Nico Böhr, David Hildenbrand,
Shaoqin Huang, Nikos Nikoleris, David Woodhouse, Ricardo Koller,
rminmin, Gavin Shan, Nina Schoetterl-Glausch, Sean Christopherson,
kvm, kvmarm, kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-02 8:56 ` Andrew Jones
2024-05-02 9:34 ` Thomas Huth
@ 2024-05-03 5:11 ` Nicholas Piggin
1 sibling, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-03 5:11 UTC (permalink / raw
To: Andrew Jones, Thomas Huth
Cc: Paolo Bonzini, Alexandru Elisei, Eric Auger, Janosch Frank,
Claudio Imbrenda, Nico Böhr, David Hildenbrand,
Shaoqin Huang, Nikos Nikoleris, David Woodhouse, Ricardo Koller,
rminmin, Gavin Shan, Nina Schoetterl-Glausch, Sean Christopherson,
kvm, kvmarm, kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-03 5:02 ` Nicholas Piggin
@ 2024-05-03 5:13 ` Thomas Huth
2024-05-07 4:26 ` Nicholas Piggin
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2024-05-03 5:13 UTC (permalink / raw
To: Nicholas Piggin, Andrew Jones
Cc: Paolo Bonzini, Alexandru Elisei, Eric Auger, Janosch Frank,
Claudio Imbrenda, Nico Böhr, David Hildenbrand,
Shaoqin Huang, Nikos Nikoleris, David Woodhouse, Ricardo Koller,
rminmin, Gavin Shan, Nina Schoetterl-Glausch, Sean Christopherson,
kvm, kvmarm, kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kvm-unit-tests PATCH v3 0/5] add shellcheck support
2024-05-03 5:13 ` Thomas Huth
@ 2024-05-07 4:26 ` Nicholas Piggin
0 siblings, 0 replies; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-07 4:26 UTC (permalink / raw
To: Thomas Huth, Andrew Jones
Cc: Paolo Bonzini, Alexandru Elisei, Eric Auger, Janosch Frank,
Claudio Imbrenda, Nico Böhr, David Hildenbrand,
Shaoqin Huang, Nikos Nikoleris, David Woodhouse, Ricardo Koller,
rminmin, Gavin Shan, Nina Schoetterl-Glausch, Sean Christopherson,
kvm, kvmarm, kvm-riscv, linux-s390
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-05-07 4:26 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 11:29 [kvm-unit-tests PATCH v3 0/5] add shellcheck support Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 1/5] Add initial shellcheck checking Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 2/5] shellcheck: Fix SC2155 Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 3/5] shellcheck: Fix SC2124 Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 4/5] shellcheck: Fix SC2294 Nicholas Piggin
2024-05-01 11:29 ` [kvm-unit-tests PATCH v3 5/5] shellcheck: Suppress various messages Nicholas Piggin
2024-05-02 8:12 ` [kvm-unit-tests PATCH v3 0/5] add shellcheck support Andrew Jones
2024-05-02 8:23 ` Thomas Huth
2024-05-02 8:56 ` Andrew Jones
2024-05-02 9:34 ` Thomas Huth
2024-05-02 9:48 ` Andrew Jones
2024-05-03 5:02 ` Nicholas Piggin
2024-05-03 5:13 ` Thomas Huth
2024-05-07 4:26 ` Nicholas Piggin
2024-05-03 5:11 ` Nicholas Piggin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.