Thanks very much for the quick review. I indeed had a bug there losing the
check for unset ERRATA_$commit.
We do require Bash 4.1 and later compatibility. I would like to change the
test -v lines to use indirection:
[[ -n "${!errata}" ]] && continue
I tested this on 4.1 and on 4.4. It treats unset and "" as the same which
should be sufficient here AFAICT (while maintaining compatibility).
I have a new patch ready to go. Please let me know if this works for you.
Thanks,
Peter
On Thu, Apr 19, 2018 at 12:58 AM Andrew Jones <drjones@xxxxxxxxxx> wrote:
> 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
