| Message ID | 20180418215445.98912-1-pshier@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@google.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 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
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@redhat.com> 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@google.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 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
On Fri, Apr 20, 2018 at 12:06:30AM +0000, Peter Shier wrote: > 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. > Yeah, that's fine. One other comment on your patch though is that you were only changing the 'test -v' in env_generate_errata(). There's another one in env_add_errata() that should also be changed. If you can do exhaustive testing or some analysis of the bash code to ensure it's 4.1 compatible, then we can now document that requirement in the README as well. I wouldn't want to document it until we're sure though. Thanks, drew
I have run the whole suite on 4.1 but not sure if everything in this script runs in that case. New patching coming right after this. Thx. On Fri, Apr 20, 2018 at 12:35 AM Andrew Jones <drjones@redhat.com> wrote: > On Fri, Apr 20, 2018 at 12:06:30AM +0000, Peter Shier wrote: > > 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. > > > Yeah, that's fine. One other comment on your patch though is that you > were only changing the 'test -v' in env_generate_errata(). There's > another one in env_add_errata() that should also be changed. > If you can do exhaustive testing or some analysis of the bash code to > ensure it's 4.1 compatible, then we can now document that requirement > in the README as well. I wouldn't want to document it until we're > sure though. > Thanks, > drew
On Fri, Apr 20, 2018 at 04:59:58PM +0000, Peter Shier wrote: > I have run the whole suite on 4.1 but not sure if everything in this script > runs in that case. New patching coming right after this. Thx. As long as you've also done a 'configure && make standalone' and run the generated tests with bash 4.1, then I think you've exercised the majority of the code. We'll still be missing code paths as some of them are dependent on test return status or whether or not a signal is received. Also powerpc uses a couple extra functions, namely run_qemu_status() and run_migration(). Oh wait[*], you should also run with 'run_tests.sh -j <some-number>' to test the parallel execution support, which I now see won't work with Bash older than 4.3, as it's using the '-n' option for 'wait'[**]. We can rework that or just special case it, i.e. do a Bash version check when '-j' is enabled and then say it's not supported when we detect old Bash. With that, I think we can have reasonable confidence that we're 4.1 compliant and document it. I'll try to find time to write test cases to ensure full coverage and run them with Bash 4.1 as well. Thanks, drew [*] Pun intended [**] Now you see the pun, right? I hope you're laughing. > On Fri, Apr 20, 2018 at 12:35 AM Andrew Jones <drjones@redhat.com> wrote: > > > On Fri, Apr 20, 2018 at 12:06:30AM +0000, Peter Shier wrote: > > > 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. > > > > > > Yeah, that's fine. One other comment on your patch though is that you > > were only changing the 'test -v' in env_generate_errata(). There's > > another one in env_add_errata() that should also be changed. > > > If you can do exhaustive testing or some analysis of the bash code to > > ensure it's 4.1 compatible, then we can now document that requirement > > in the README as well. I wouldn't want to document it until we're > > sure though. > > > Thanks, > > drew
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" 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 IFS=. read -r v p rest <<<"$minver" IFS=- read -r s x <<<"$rest"
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@google.com> --- scripts/arch-run.bash | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)