On 03/05/2014 04:46 PM, Pádraig Brady wrote: > Note the ':>' is problematic though as the : always returns true. > Well it does for solaris, freebsd, dash, bash-4.3 at least. > In ksh and bash<=4.2 it does return an error as you assumed. > The ':>' construct is only needed for csh anyway I think > which we don't support. > > I notice a few more cases: git grep ": *>.*" > which I'll fix in a follow up patch with maybe a syntax check.
Attached. thanks, Pádraig.
>From 056d6de78fcf11a0f404f25faf8cecc9f9ee9c1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Wed, 5 Mar 2014 18:41:16 +0000 Subject: [PATCH] tests: avoid the :> construct which can hide errors On most shells `:>file || framework_failure_` will not evaluate the framework_failure_ even if there was an error writing the file. shells which do evaluate the failure are ksh 93u+ and bash 4.2, while shells wich don't include bash 4.3, solaris, freebsd, dash. Furthermore this construct is problematic on Solaris 10 sh, which will try to optimize away a `:' command in a loop after the first iteration, even if it is redirected. * tests/cp/link-deref.sh: Remove the leading colon on redirections. * tests/cp/reflink-perm.sh: Likewise. * tests/id/zero.sh: Likewise. * tests/install/install-C.sh: Likewise. * tests/misc/env.sh: Likewise. * tests/misc/md5sum-bsd.sh: Likewise. * tests/misc/runcon-no-reorder.sh: Likewise. * tests/mv/partition-perm.sh: Likewise. * tests/rm/r-root.sh: Likewise. * tests/split/l-chunk.sh: Likewise. * tests/split/line-bytes.sh: Likewise. * tests/tail-2/inotify-rotate.sh: Likewise. * tests/tail-2/retry.sh: Likewise. * tests/tail-2/symlink.sh: Likewise. * tests/tail-2/wait.sh: Likewise. * tests/touch/read-only.sh: Likewise. + cfg.mk (sc_prohibit_colon_redirection): A new syntax check to avoid further instances of this creeping in. --- cfg.mk | 7 +++++++ tests/cp/link-deref.sh | 2 +- tests/cp/reflink-perm.sh | 4 ++-- tests/id/zero.sh | 2 +- tests/install/install-C.sh | 2 +- tests/misc/env.sh | 2 +- tests/misc/md5sum-bsd.sh | 2 +- tests/misc/runcon-no-reorder.sh | 2 +- tests/mv/partition-perm.sh | 6 +++--- tests/rm/r-root.sh | 6 +++--- tests/split/l-chunk.sh | 8 ++++---- tests/split/line-bytes.sh | 4 ++-- tests/tail-2/inotify-rotate.sh | 2 +- tests/tail-2/retry.sh | 2 +- tests/tail-2/symlink.sh | 4 ++-- tests/tail-2/wait.sh | 2 +- tests/touch/read-only.sh | 2 +- 17 files changed, 33 insertions(+), 26 deletions(-) diff --git a/cfg.mk b/cfg.mk index 7f74afa..cc6f4b8 100644 --- a/cfg.mk +++ b/cfg.mk @@ -123,6 +123,13 @@ sc_tests_executable: | sed -e "s/^/$(ME): Please make test executable: /" | grep . \ && exit 1; : +# Avoid :>file which doesn't propagate errors +sc_prohibit_colon_redirection: + @cd $(srcdir)/tests && GIT_PAGER= git grep -n ': *>.*||' \ + && { echo '$(ME): '"The leading colon in :> will hide errors" 1>&2; \ + exit 1; } \ + || : + # Create a list of regular expressions matching the names # of files included from system.h. Exclude a couple. .re-list: diff --git a/tests/cp/link-deref.sh b/tests/cp/link-deref.sh index 73c86a8..1b66658 100755 --- a/tests/cp/link-deref.sh +++ b/tests/cp/link-deref.sh @@ -31,7 +31,7 @@ if grep '^#define HAVE_LINKAT 1' "$CONFIG_HEADER" > /dev/null \ fi mkdir dir || framework_failure_ -: > file || framework_failure_ +> file || framework_failure_ ln -s dir dirlink || framework_failure_ ln -s file filelink || framework_failure_ ln -s nowhere danglink || framework_failure_ diff --git a/tests/cp/reflink-perm.sh b/tests/cp/reflink-perm.sh index 0650065..52f83fc 100755 --- a/tests/cp/reflink-perm.sh +++ b/tests/cp/reflink-perm.sh @@ -20,8 +20,8 @@ print_ver_ cp -: > time_check -: > file +> time_check +> file ts='2009-08-28 19:00' touch -d "$ts" file || framework_failure_ test time_check -nt file || skip_ "The system clock is wrong" diff --git a/tests/id/zero.sh b/tests/id/zero.sh index 939b162..c826ff4 100755 --- a/tests/id/zero.sh +++ b/tests/id/zero.sh @@ -42,7 +42,7 @@ printf '%s\n' $users '' >> users || framework_failure_ # Exercise "id -z" with various options. printf '\n' > exp || framework_failure_ -:> out || framework_failure_ +> out || framework_failure_ while read u ; do for o in g gr G Gr u ur ; do diff --git a/tests/install/install-C.sh b/tests/install/install-C.sh index 6ce4977..9ee1685 100755 --- a/tests/install/install-C.sh +++ b/tests/install/install-C.sh @@ -54,7 +54,7 @@ echo test > a || framework_failure_ echo "'a' -> 'b'" > out_installed_first || framework_failure_ echo "removed 'b' 'a' -> 'b'" > out_installed_second || framework_failure_ -: > out_empty || framework_failure_ +> out_empty || framework_failure_ # destination file does not exist ginstall -Cv -m$mode1 a b > out || fail=1 diff --git a/tests/misc/env.sh b/tests/misc/env.sh index 0987f7f..877a5e3 100755 --- a/tests/misc/env.sh +++ b/tests/misc/env.sh @@ -62,7 +62,7 @@ fi ENV_TEST1=a export ENV_TEST1 -: >out || framework_failure_ +>out || framework_failure_ env ENV_TEST2= > all || fail=1 grep '^ENV_TEST' all | LC_ALL=C sort >> out || framework_failure_ env -u ENV_TEST1 ENV_TEST3=c > all || fail=1 diff --git a/tests/misc/md5sum-bsd.sh b/tests/misc/md5sum-bsd.sh index 77f0a59..f73e062 100755 --- a/tests/misc/md5sum-bsd.sh +++ b/tests/misc/md5sum-bsd.sh @@ -70,7 +70,7 @@ nl=' tab=' ' rm check.md5 for i in 'a\b' 'a\' "a${nl}b" "a${tab}b"; do - :> "$i" + > "$i" md5sum --tag "$i" >> check.md5 done md5sum --strict -c check.md5 || fail=1 diff --git a/tests/misc/runcon-no-reorder.sh b/tests/misc/runcon-no-reorder.sh index 7338ad5..ca8ad5b 100755 --- a/tests/misc/runcon-no-reorder.sh +++ b/tests/misc/runcon-no-reorder.sh @@ -27,7 +27,7 @@ echo "$diag" > exp || framework_failure_ # On such a system it fails with the above diagnostic, which is fine. # Before the no-reorder change, it would have failed with a diagnostic # about -j being an invalid option. -runcon $(id -Z) true -j 2> out && : > exp +runcon $(id -Z) true -j 2> out && > exp # When run on a system with no /selinux/context (i.e., in a chroot), # it chcon fails with this: "runcon: invalid context: \ diff --git a/tests/mv/partition-perm.sh b/tests/mv/partition-perm.sh index 2a25e52..713988f 100755 --- a/tests/mv/partition-perm.sh +++ b/tests/mv/partition-perm.sh @@ -21,11 +21,11 @@ print_ver_ mv cleanup_() { rm -rf "$other_partition_tmpdir"; } . "$abs_srcdir/tests/other-fs-tmpdir" -: > file -chmod a=rwx file +> file || framework_failure_ +chmod a=rwx file || framework_failure_ umask 077 -mv file "$other_partition_tmpdir" +mv file "$other_partition_tmpdir" || framework_failure_ test -f file && fail=1 test -f "$other_partition_tmpdir/file" || fail=1 diff --git a/tests/rm/r-root.sh b/tests/rm/r-root.sh index 04a88eb..e17b85b 100755 --- a/tests/rm/r-root.sh +++ b/tests/rm/r-root.sh @@ -104,7 +104,7 @@ test -d dir && framework_failure_ # rm(1) must succeed as before, but this time both the evidence file "x" # and the test file / directory must still exist afterward. mkdir dir || framework_failure_ -: > file || framework_failure_ +> file || framework_failure_ skip= for file in dir file ; do @@ -168,8 +168,8 @@ done # Exercise "rm -r file1 / file2". # Expect a non-Zero exit status representing failure to remove "/", # yet 'file1' and 'file2' should be removed. -: > file1 || framework_failure_ -: > file2 || framework_failure_ +> file1 || framework_failure_ +> file2 || framework_failure_ # Now that we know that 'rm' won't call the unlinkat() system function for "/", # we could probably execute it without the LD_PRELOAD'ed safety net. diff --git a/tests/split/l-chunk.sh b/tests/split/l-chunk.sh index 066fb01..792c556 100755 --- a/tests/split/l-chunk.sh +++ b/tests/split/l-chunk.sh @@ -71,7 +71,7 @@ DEBUGGING= test "$DEBUGGING" && test "$VERBOSE" && set +x for ELIDE_EMPTY in '' '-e'; do for IO_BLKSIZE in 1 2 5 10 80 100; do - : > out + > out test "$DEBUGGING" && printf "\n---io-blk-size=$IO_BLKSIZE $ELIDE_EMPTY\n" for N in 6 8 12 15 22; do rm -f x* @@ -119,15 +119,15 @@ test "$DEBUGGING" && test "$VERBOSE" && set -x # Check extraction of particular chunks -: > out +> out printf '1\n12345\n' > exp split -n l/13/15 in > out compare exp out || fail=1 -: > out +> out printf '' > exp split -n l/14/15 in > out compare exp out || fail=1 -: > out +> out printf '1\n12345\n1\n' > exp split -n l/15/15 in > out compare exp out || fail=1 diff --git a/tests/split/line-bytes.sh b/tests/split/line-bytes.sh index 9dc06a6..5f6f505 100755 --- a/tests/split/line-bytes.sh +++ b/tests/split/line-bytes.sh @@ -63,8 +63,8 @@ cat <<\EOF > no_eol_splits_exp EOF for b in $(seq 10); do - : > splits - : > no_eol_splits + > splits + > no_eol_splits for s in $(seq 11); do rm x?? split ---io=$b -C$s in || fail=1 diff --git a/tests/tail-2/inotify-rotate.sh b/tests/tail-2/inotify-rotate.sh index a95f4ec..1c942cc 100755 --- a/tests/tail-2/inotify-rotate.sh +++ b/tests/tail-2/inotify-rotate.sh @@ -47,7 +47,7 @@ for i in $(seq 50); do # Normally less than a second is required here, but with heavy load # and a lot of disk activity, even 20 seconds is insufficient, which # leads to this timeout killing tail before the "ok" is written below. - :>k && :>x || framework_failure_ failed to initialize files + >k && >x || framework_failure_ failed to initialize files timeout 40 tail -F k > out 2>&1 & pid=$! sleep .1 diff --git a/tests/tail-2/retry.sh b/tests/tail-2/retry.sh index 5b74b74..dbe66a4 100755 --- a/tests/tail-2/retry.sh +++ b/tests/tail-2/retry.sh @@ -45,7 +45,7 @@ grep -F 'tail: warning: --retry ignored' out || fail=1 # === Test: # Ensure that "tail --retry --follow=name" waits for the file to appear. # Clear 'out' so that we can check its contents without races -:>out || framework_failure_ +>out || framework_failure_ timeout 10 tail -s.1 --follow=name --retry missing >out 2>&1 & pid=$! retry_delay_ wait4lines_ .1 6 1 || fail=1 # Wait for "cannot open" error. echo "X" > missing || fail=1 diff --git a/tests/tail-2/symlink.sh b/tests/tail-2/symlink.sh index 362cb63..b21f9e1 100755 --- a/tests/tail-2/symlink.sh +++ b/tests/tail-2/symlink.sh @@ -32,7 +32,7 @@ wait4lines_ () # Ensure changing targets of cli specified symlinks are handled. # Prior to v8.22, inotify would fail to recognize changes in the targets. # Clear 'out' so that we can check its contents without races. -:>out || framework_failure_ +>out || framework_failure_ ln -nsf target symlink || framework_failure_ timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$! retry_delay_ wait4lines_ .1 6 1 || fail=1 # Wait for "cannot open..." @@ -50,7 +50,7 @@ rm -f target out || framework_failure_ # Ensure we correctly handle the source symlink itself changing. # I.E. that we don't operate solely on the targets. # Clear 'out' so that we can check its contents without races. -:>out || framework_failure_ +>out || framework_failure_ echo "X1" > target1 || framework_failure_ ln -nsf target1 symlink || framework_failure_ timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$! diff --git a/tests/tail-2/wait.sh b/tests/tail-2/wait.sh index 42a93bf..13898ac 100755 --- a/tests/tail-2/wait.sh +++ b/tests/tail-2/wait.sh @@ -52,7 +52,7 @@ for inotify in ---disable-inotify ''; do grep -Ev 'inotify (resources exhausted|cannot be used)' tail.err > x mv x tail.err test -s tail.err && fail=1 - :>tail.err + >tail.err tail_F() { diff --git a/tests/touch/read-only.sh b/tests/touch/read-only.sh index 95cd4f7..d239738 100755 --- a/tests/touch/read-only.sh +++ b/tests/touch/read-only.sh @@ -20,7 +20,7 @@ print_ver_ touch skip_if_root_ -: > read-only || framework_failure_ +> read-only || framework_failure_ chmod 444 read-only || framework_failure_ -- 1.7.7.6
