On Wed, Apr 18, 2018 at 02:54:45PM -0700, Peter Shier wrote:
> KVM unit tests scripts/arch-run.bash uses test -v to check whether a variable
> was set. The -v switch was introduced in Bash 4.2. This patch uses the older
> test -z to test for an empty string.
>
> test -v was used on a variable that is set on the prior line so it would never
> be empty or considered unset. This patch moves the test earlier to check whether
> there is a SHA in the errata file.
>
> This patch also adds double quotes around source strings for read commands. On
> older Bash versions, without the quotes the read will parse the string into its
> components but then place them all into a single variable separated by spaces
> rather than into separate variables as the script intends.
>
> Tested: Ran with an injected empty string in errata.txt to verify with
> git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git at
> 073ea627a4268333e0e2245382ecf5fabc28f594.
>
> Signed-off-by: Peter Shier <pshier@xxxxxxxxxx>
> ---
> 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 e13af8e8064a..f0a9b1d7c53c 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -234,8 +234,8 @@ env_generate_errata ()
> local kernel_version kernel_patchlevel kernel_sublevel kernel_extraversion
> local line commit minver errata rest v p s x have
>
> - IFS=. read -r kernel_version kernel_patchlevel rest <<<$kernel_version_string
> - IFS=- read -r kernel_sublevel kernel_extraversion <<<$rest
> + IFS=. read -r kernel_version kernel_patchlevel rest <<<"$kernel_version_string"
> + IFS=- read -r kernel_sublevel kernel_extraversion <<<"$rest"
I agree with this change. Indeed ShellCheck complains about these and many
other cases where we've (I've) been too lax with the double quotes. We
should probably do another round of ShellCheck cleanups, but with less
ignoring of the warnings.
> kernel_sublevel=${kernel_sublevel%%[!0-9]*}
> kernel_extraversion=${kernel_extraversion%%[!0-9]*}
>
> @@ -249,8 +249,8 @@ env_generate_errata ()
> commit=${line%:*}
> minver=${line#*:}
>
> + test -z "$commit" && continue
> errata="ERRATA_$commit"
> - test -v $errata && continue
These two tests are quite different. 'test -z "$var"' returns true
when the variable 'var' evaluates to an empty string. 'test -v "$var"'
evaluates true when the string 'var' evaluates to is a shell variable
itself and is set.
The 'test -z "$commit"' check is probably a good idea, but if that
happens we shouldn't continue, we should abort and complain about
the errata file.
>
> IFS=. read -r v p rest <<<"$minver"
> IFS=- read -r s x <<<"$rest"
> --
> 2.17.0.484.g0c8726318c-goog
>
We've definitely got enough bash code now that we should take our
bash programming more seriously by doing ShellCheck cleanups and by
documenting which version and later of Bash we require. In the subject
you wrote Bash 4.1 and older, but I guess you meant 'and later'. I'm
fine with reworking code to support Bash 4.1 and later if there's
a good reason for that. Otherwise, if we're sure that 4.2 works,
then I'd say we should just document that fact and from now on
avoid using features introduced after 4.2.
Thanks,
drew
