diff mbox series

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

Message ID 20240405090052.375599-18-npiggin@gmail.com (mailing list archive)
State New, archived
Headers show
Series add shellcheck support | expand

Commit Message

Nicholas Piggin April 5, 2024, 9 a.m. UTC
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(+)

Comments

Andrew Jones April 5, 2024, 2:55 p.m. UTC | #1
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
Nicholas Piggin April 6, 2024, 6:31 a.m. UTC | #2
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
Andrew Jones April 6, 2024, 7:30 a.m. UTC | #3
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
Nicholas Piggin April 6, 2024, 10:56 a.m. UTC | #4
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
diff mbox series

Patch

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%%=*}