| Message ID | 1486376719-6668-1-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > Convert all scripts to use /bin/sh shebang and fix all shellcheck > reported problems. Pro-tip, this is the place reserved in commit messages for describing *why* you think the change is needed or for the better. ;) BR, Jani. > > v2: Include tests/Makefile.sources (Petri) > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Petri Latvala <petri.latvala@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Petri Latvala <petri.latvala@intel.com> > --- > tests/Makefile.sources | 1 - > tests/check_drm_clients | 15 +++++++++---- > tests/ddx_intel_after_fbdev | 42 ++++++++++++++++++------------------- > tests/debugfs_emon_crash | 16 +++++++++----- > tests/debugfs_wedged | 18 ++++++++++------ > tests/drm_getopt.sh | 20 ++++++++++-------- > tests/drm_lib.sh | 51 +++++++++++++++++++++++---------------------- > tests/drv_debugfs_reader | 17 ++++++++++----- > tests/igt_command_line.sh | 26 +++++++++++------------ > tests/sysfs_l3_parity | 30 ++++++++++++++++---------- > tests/test_rte_check | 6 ------ > tests/tools_test | 17 +++++++++------ > 12 files changed, 147 insertions(+), 112 deletions(-) > delete mode 100755 tests/test_rte_check > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 6e07d93..dcc80c5 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -242,7 +242,6 @@ TESTS_scripts = \ > debugfs_emon_crash \ > drv_debugfs_reader \ > sysfs_l3_parity \ > - test_rte_check \ > tools_test \ > $(NULL) > > diff --git a/tests/check_drm_clients b/tests/check_drm_clients > index 2a891b8..25c03b1 100755 > --- a/tests/check_drm_clients > +++ b/tests/check_drm_clients > @@ -1,6 +1,13 @@ > -#!/bin/bash > +#!/bin/sh > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > + > +exit "$IGT_EXIT_SUCCESS" > > -exit $IGT_EXIT_SUCCESS > diff --git a/tests/ddx_intel_after_fbdev b/tests/ddx_intel_after_fbdev > index f068209..2ca5ce0 100755 > --- a/tests/ddx_intel_after_fbdev > +++ b/tests/ddx_intel_after_fbdev > @@ -1,4 +1,4 @@ > -#!/bin/bash > +#!/bin/sh > # > # Testcase: Load Intel DDX after fbdev was loaded > # > @@ -9,7 +9,7 @@ whoami | grep -q root || { > } > > # no other X session should be running > -find /tmp/ -name .X*lock 2>/dev/null | grep -q X && { > +find /tmp/ -name ".X*lock" 2>/dev/null | grep -q X && { > echo "ERROR: X session already running" > exit 1 > } > @@ -19,14 +19,14 @@ TMPDIR=$(mktemp -d /tmp/igt.XXXX) || { > exit 1 > } > > -cat > $TMPDIR/xorg.conf.fbdev << EOF > +cat > "$TMPDIR/xorg.conf.fbdev" << EOF > Section "Device" > Driver "fbdev" > Identifier "Device[fbdev]" > EndSection > EOF > > -cat > $TMPDIR/xorg.conf.intel << EOF > +cat > "$TMPDIR/xorg.conf.intel" << EOF > Section "Device" > Driver "intel" > Identifier "Device[intel]" > @@ -34,40 +34,40 @@ EndSection > EOF > > # log before fbdev > -dmesg -c > $TMPDIR/dmesg.1.before.fbdev > -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.1.before.fbdev > +dmesg -c > "$TMPDIR/dmesg.1.before.fbdev" > +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.1.before.fbdev" > > # run fbdev > -xinit -- /usr/bin/X -config $TMPDIR/xorg.conf.fbdev & > +xinit -- /usr/bin/X -config "$TMPDIR/xorg.conf.fbdev" & > sleep 5 > -if [ -f `which intel_reg` ]; then > -`which intel_reg` dump > $TMPDIR/intel_reg_dump.1.fbdev > +if [ -f "$(which intel_reg)" ]; then > + intel_reg dump > "$TMPDIR/intel_reg_dump.1.fbdev" > fi > killall X > > # log after fbdev & before intel > -dmesg -c > $TMPDIR/dmesg.2.after.fbdev.before.intel > -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.2.after.fbdev.before.intel > +dmesg -c > "$TMPDIR/dmesg.2.after.fbdev.before.intel" > +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.2.after.fbdev.before.intel" > > sleep 5 > > # run intel > -xinit -- /usr/bin/X -config $TMPDIR/xorg.conf.intel & > -sleep 5 > -if [ -f `which intel_reg` ]; then > -`which intel_reg` dump > $TMPDIR/intel_reg_dump.2.intel > +xinit -- /usr/bin/X -config "$TMPDIR/xorg.conf.intel" & > +sleep 5 > +if [ -f "$(which intel_reg)" ]; then > + intel_reg dump > "$TMPDIR/intel_reg_dump.2.intel" > fi > killall X > > # log after intel > -dmesg -c > $TMPDIR/dmesg.3.after.intel > -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.3.after.intel > +dmesg -c > "$TMPDIR/dmesg.3.after.intel" > +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.3.after.intel" > > -cp $0 $TMPDIR/ > +cp "$0" "$TMPDIR/" > > -tar czf $TMPDIR.tar.gz $TMPDIR/* > -if [ -f $TMPDIR.tar.gz ]; then > - echo $TMPDIR.tar.gz contains this script, all configs and logs generated on this tests > +tar czf "$TMPDIR.tar.gz" "$TMPDIR/" > +if [ -f "$TMPDIR.tar.gz" ]; then > + echo "'$TMPDIR.tar.gz' contains this script, all configs and logs generated on this tests" > fi > > exit 0 > diff --git a/tests/debugfs_emon_crash b/tests/debugfs_emon_crash > index 1dbfcb2..11b1c44 100755 > --- a/tests/debugfs_emon_crash > +++ b/tests/debugfs_emon_crash > @@ -1,16 +1,22 @@ > -#!/bin/bash > +#!/bin/sh > # > # This check if we can crash the kernel with segmentation-fault > # by reading /sys/kernel/debug/dri/0/i915_emon_status too quickly > # > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > > for z in $(seq 1 1000); do > - cat $i915_dfs_path/i915_emon_status > /dev/null 2&>1 > + cat "$i915_dfs_path/i915_emon_status" > /dev/null 2>&1 > done > > # If we got here, we haven't crashed > +exit "$IGT_EXIT_SUCCESS" > > -exit $IGT_EXIT_SUCCESS > diff --git a/tests/debugfs_wedged b/tests/debugfs_wedged > index f15ac46..26b40de 100755 > --- a/tests/debugfs_wedged > +++ b/tests/debugfs_wedged > @@ -1,10 +1,16 @@ > -#!/bin/bash > +#!/bin/sh > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > > # Testcase: wedge the hw to check the error_state reading > -# > +# > # Unfortunately wedged is permanent, so this test is not run by default > -echo 1 > ${i915_dfs_path}/i915_wedged > -cat $i915_dfs_path/i915_error_state > /dev/null 2>&1 > +echo 1 > "${i915_dfs_path}/i915_wedged" > +cat "$i915_dfs_path/i915_error_state" > /dev/null 2>&1 > diff --git a/tests/drm_getopt.sh b/tests/drm_getopt.sh > index a94a759..10f367c 100755 > --- a/tests/drm_getopt.sh > +++ b/tests/drm_getopt.sh > @@ -1,3 +1,4 @@ > +#!/bin/sh > # This is required for check/distcheck target as it has to --list-subtests > # for each test. Source it if you add a new test in form of a (shell) script. > > @@ -9,30 +10,31 @@ IGT_EXIT_SUCCESS=0 > IGT_EXIT_INVALID=79 > IGT_EXIT_FAILURE=99 > > -# hacked-up long option parsing > -for arg in $@ ; do > - case $arg in > +PROG=$(basename "$THIS_SCRIPT") > + > +for arg do > + case "$arg" in > --list-subtests) > - exit $IGT_EXIT_INVALID > + exit "$IGT_EXIT_INVALID" > ;; > --run-subtest) > - exit $IGT_EXIT_INVALID > + exit "$IGT_EXIT_INVALID" > ;; > --debug) > IGT_LOG_LEVEL=debug > ;; > --help-description) > - echo $IGT_TEST_DESCRIPTION > - exit $IGT_EXIT_SUCCESS > + echo "$IGT_TEST_DESCRIPTION" > + exit "$IGT_EXIT_SUCCESS" > ;; > --help) > - echo "Usage: `basename $0` [OPTIONS]" > + echo "Usage: $PROG [OPTIONS]" > echo " --list-subtests" > echo " --run-subtest <pattern>" > echo " --debug" > echo " --help-description" > echo " --help" > - exit $IGT_EXIT_SUCCESS > + exit "$IGT_EXIT_SUCCESS" > ;; > esac > done > diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh > index af104ad..2aa073d 100755 > --- a/tests/drm_lib.sh > +++ b/tests/drm_lib.sh > @@ -1,23 +1,22 @@ > -#!/bin/bash > +#!/bin/sh > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_getopt.sh > +. "$SOURCE_DIR/drm_getopt.sh" > > -NAME=$(basename "$0") > +NAME=$(basename "$THIS_SCRIPT") > > dynamic_debug= > > hda_dynamic_debug_enable() { > if [ -e "$dynamic_debug" ]; then > - echo -n "module snd_hda_intel +pf" > $dynamic_debug > - echo -n "module snd_hda_core +pf" > $dynamic_debug > + echo "module snd_hda_intel +pf" > "$dynamic_debug" > + echo "module snd_hda_core +pf" > "$dynamic_debug" > fi > } > > hda_dynamic_debug_disable() { > if [ -e "$dynamic_debug" ]; then > - echo -n "module snd_hda_core =_" > $dynamic_debug > - echo -n "module snd_hda_intel =_" > $dynamic_debug > + echo "module snd_hda_core =_" > "$dynamic_debug" > + echo "module snd_hda_intel =_" > "$dynamic_debug" > fi > } > > @@ -31,7 +30,9 @@ KERN_INFO="<6>" > KERN_DEBUG="<7>" > > kmsg() { > - echo "$@" > /dev/kmsg > + if [ -w "/dev/kmsg" ]; then > + echo "$@" > /dev/kmsg > + fi > } > > finish() { > @@ -46,16 +47,16 @@ kmsg "${KERN_INFO}[IGT] $NAME: executing" > > skip() { > echo "$@" > - exit $IGT_EXIT_SKIP > + exit "$IGT_EXIT_SKIP" > } > > die() { > echo "$@" > - exit $IGT_EXIT_FAILURE > + exit "$IGT_EXIT_FAILURE" > } > > do_or_die() { > - $@ > /dev/null 2>&1 || (echo "FAIL: $@ ($?)" && exit $IGT_EXIT_FAILURE) > + "$@" > /dev/null 2>&1 || ( echo "FAIL: $* ($?)" && exit "$IGT_EXIT_FAILURE" ) > } > > if [ -d /sys/kernel/debug ]; then > @@ -66,47 +67,47 @@ else > skip "debugfs not found" > fi > > -dynamic_debug=$debugfs_path/dynamic_debug/control > +dynamic_debug="$debugfs_path/dynamic_debug/control" > if [ ! -e "$dynamic_debug" ]; then > echo "WARNING: dynamic debug control not available" > fi > > -if [ ! -d $debugfs_path/dri ]; then > +if [ ! -d "$debugfs_path/dri" ]; then > skip "dri debugfs not found" > fi > > -i915_dfs_path=x > -for minor in `seq 0 16`; do > - if [ -f $debugfs_path/dri/$minor/i915_error_state ] ; then > - i915_dfs_path=$debugfs_path/dri/$minor > +i915_dfs_path= > +for minor in $(seq 0 16); do > + if [ -f "$debugfs_path/dri/$minor/i915_error_state" ] ; then > + i915_dfs_path="$debugfs_path/dri/$minor" > break > fi > done > > -if [ $i915_dfs_path = "x" ] ; then > - skip " i915 debugfs path not found." > +if [ -z "$i915_dfs_path" ] ; then > + skip "i915 debugfs path not found." > fi > > # read everything we can > -if [ `cat $i915_dfs_path/clients | wc -l` -gt "2" ] ; then > +if [ "$(wc -l "$i915_dfs_path/clients")" -gt 2 ] ; then > [ -n "$DRM_LIB_ALLOW_NO_MASTER" ] || \ > die "ERROR: other drm clients running" > fi > > -whoami | grep -q root || ( echo ERROR: not running as root; exit $IGT_EXIT_FAILURE ) > +whoami | grep -q root || ( echo "ERROR: not running as root"; exit "$IGT_EXIT_FAILURE" ) > > i915_sfs_path= > if [ -d /sys/class/drm ] ; then > sysfs_path=/sys/class/drm > - if [ -f $sysfs_path/card$minor/error ] ; then > + if [ -f "$sysfs_path/card$minor/error" ] ; then > i915_sfs_path="$sysfs_path/card$minor" > fi > fi > # sysfs may not exist as the 'error' is a new interface in 3.11 > > -function drmtest_skip_on_simulation() > +drmtest_skip_on_simulation() > { > - [ -n "$INTEL_SIMULATION" ] && exit $IGT_EXIT_SKIP > + [ -n "$INTEL_SIMULATION" ] && exit "$IGT_EXIT_SKIP" > } > > drmtest_skip_on_simulation > diff --git a/tests/drv_debugfs_reader b/tests/drv_debugfs_reader > index 6ea4e64..58d02de 100755 > --- a/tests/drv_debugfs_reader > +++ b/tests/drv_debugfs_reader > @@ -1,9 +1,16 @@ > -#!/bin/bash > +#!/bin/sh > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > > # read everything we can > -cat $i915_dfs_path/* > /dev/null 2>&1 > +cat "$i915_dfs_path"/* > /dev/null 2>&1 > + > +exit "$IGT_EXIT_SUCCESS" > > -exit $IGT_EXIT_SUCCESS > diff --git a/tests/igt_command_line.sh b/tests/igt_command_line.sh > index a20e44c..bab3a77 100755 > --- a/tests/igt_command_line.sh > +++ b/tests/igt_command_line.sh > @@ -26,11 +26,11 @@ > # > > if [ -z "$top_builddir" ]; then > - top_builddir="$(dirname $0)" > + top_builddir=$(dirname "$0") > fi > > # allow to run this script from top directory > -TESTLIST=`cat $top_builddir/test-list.txt` > +TESTLIST=$(cat "$top_builddir/test-list.txt") > if [ $? -ne 0 ]; then > # distcheck requires this hack > TESTLIST=$(cat test-list.txt) > @@ -51,7 +51,7 @@ for test in $TESTLIST; do > fi > > # top_builddir is empty for distcheck > - test=$top_builddir/$test > + test="$top_builddir/$test" > > # distcheck requires this hack > if [ ! -x "$test" ]; then > @@ -62,29 +62,29 @@ for test in $TESTLIST; do > > # check invalid option handling > echo " Checking invalid option handling..." > - ./$test --invalid-option 2> /dev/null && fail $test > + "./$test" --invalid-option 2> /dev/null && fail "$test" > > # check valid options succeed > echo " Checking valid option handling..." > - ./$test --help > /dev/null || fail $test > + "./$test" --help > /dev/null || fail "$test" > > # check --list-subtests works correctly > echo " Checking subtest enumeration..." > - LIST=`./$test --list-subtests` > + LIST=$("./$test" --list-subtests) > RET=$? > - if [ $RET -ne 0 -a $RET -ne 79 ]; then > - fail $test > + if [ "$RET" -ne 0 -a $RET -ne 79 ]; then > + fail "$test" > fi > > - if [ $RET -eq 79 -a -n "$LIST" ]; then > - fail $test > + if [ "$RET" -eq 79 -a -n "$LIST" ]; then > + fail "$test" > fi > > - if [ $RET -eq 0 -a -z "$LIST" ]; then > - fail $test > + if [ "$RET" -eq 0 -a -z "$LIST" ]; then > + fail "$test" > fi > > # check invalid subtest handling > echo " Checking invalid subtest handling..." > - ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail $test > + "./$test" --run-subtest invalid-subtest > /dev/null 2>&1 && fail "$test" > done > diff --git a/tests/sysfs_l3_parity b/tests/sysfs_l3_parity > index d5f3284..37a03f1 100755 > --- a/tests/sysfs_l3_parity > +++ b/tests/sysfs_l3_parity > @@ -1,22 +1,30 @@ > -#!/bin/bash > +#!/bin/sh > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > > -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e || exit $? > +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -e || exit $? > > #Check that we can remap a row > -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -d > -disabled=`$SOURCE_DIR/../tools/intel_l3_parity -l | grep -c 'Row 0, Bank 0, Subbank 0 is disabled'` > -if [ "$disabled" != "1" ] ; then > +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -d > +disabled=$("$SOURCE_DIR/../tools/intel_l3_parity" -l | grep -c 'Row 0, Bank 0, Subbank 0 is disabled') > +if [ "$disabled" != 1 ] ; then > echo "Fail" > - exit $IGT_EXIT_FAILURE > + exit "$IGT_EXIT_FAILURE" > fi > > -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e > +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -e > + > > #Check that we can clear remaps > -if [ `$SOURCE_DIR/../tools/intel_l3_parity -l | wc -l` != 1 ] ; then > +can_clear=$("$SOURCE_DIR/../tools/intel_l3_parity" -l | wc -l) > +if [ "$can_clear" != 1 ] ; then > echo "Fail 2" > - exit $IGT_EXIT_FAILURE > + exit "$IGT_EXIT_FAILURE" > fi > diff --git a/tests/test_rte_check b/tests/test_rte_check > deleted file mode 100755 > index 2a891b8..0000000 > --- a/tests/test_rte_check > +++ /dev/null > @@ -1,6 +0,0 @@ > -#!/bin/bash > - > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > - > -exit $IGT_EXIT_SUCCESS > diff --git a/tests/tools_test b/tests/tools_test > index a27fb87..dd10531 100755 > --- a/tests/tools_test > +++ b/tests/tools_test > @@ -1,16 +1,21 @@ > -#!/bin/bash > +#!/bin/sh > # Test some of the most critical tools we have accidentally broken before. > # TODO: Possibly make tests parse output > > -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" > -. $SOURCE_DIR/drm_lib.sh > +if [ -n "$BASH_SOURCE" ]; then > + THIS_SCRIPT=$BASH_SOURCE > +else > + THIS_SCRIPT=$0 > +fi > + > +SOURCE_DIR=$(dirname "$THIS_SCRIPT") > +. "$SOURCE_DIR/drm_lib.sh" > > # ARB_MODE has existed for many gens > -PATH=$SOURCE_DIR/../tools:$PATH > +PATH="$SOURCE_DIR/../tools:$PATH" > do_or_die "intel_reg read 0x4030" > do_or_die "intel_reg dump" > > # TODO: Add more tests > > -exit $IGT_EXIT_SUCCESS > - > +exit "$IGT_EXIT_SUCCESS"
On Mon, Feb 06, 2017 at 03:25:27PM +0200, Jani Nikula wrote: > On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > Convert all scripts to use /bin/sh shebang and fix all shellcheck > > reported problems. > > Pro-tip, this is the place reserved in commit messages for describing > *why* you think the change is needed or for the better. ;) And this reply here seems to be the place where I'm asking why we don't switch to C if we go through all this effort. I don't really see what sh over bash buys us (and you can pretty much expect me to re-add bashism the next time around I touch these ...). -Daniel
On ma, 2017-02-06 at 17:40 +0100, Daniel Vetter wrote: > On Mon, Feb 06, 2017 at 03:25:27PM +0200, Jani Nikula wrote: > > > > On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > > > > > Convert all scripts to use /bin/sh shebang and fix all shellcheck > > > reported problems. > > > > Pro-tip, this is the place reserved in commit messages for describing > > *why* you think the change is needed or for the better. ;) > > And this reply here seems to be the place where I'm asking why we don't > switch to C if we go through all this effort. I don't really see what sh > over bash buys us (and you can pretty much expect me to re-add bashism the > next time around I touch these ...). Oh, totally forgot due to writing the huge RFC e-mail about it. One could amend the commit message with "to able to run on non-bash shells.", if it wasn't yet merged. Regards, Joonas
On Tue, 07 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On ma, 2017-02-06 at 17:40 +0100, Daniel Vetter wrote: >> On Mon, Feb 06, 2017 at 03:25:27PM +0200, Jani Nikula wrote: >> > >> > On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: >> > > >> > > Convert all scripts to use /bin/sh shebang and fix all shellcheck >> > > reported problems. >> > >> > Pro-tip, this is the place reserved in commit messages for describing >> > *why* you think the change is needed or for the better. ;) >> >> And this reply here seems to be the place where I'm asking why we don't >> switch to C if we go through all this effort. I don't really see what sh >> over bash buys us (and you can pretty much expect me to re-add bashism the >> next time around I touch these ...). > > Oh, totally forgot due to writing the huge RFC e-mail about it. One > could amend the commit message with "to able to run on non-bash > shells.", if it wasn't yet merged. I learned this from the kids: Why? BR, Jani.
On ti, 2017-02-07 at 15:32 +0200, Jani Nikula wrote: > > On Tue, 07 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > > > On ma, 2017-02-06 at 17:40 +0100, Daniel Vetter wrote: > > > > > > On Mon, Feb 06, 2017 at 03:25:27PM +0200, Jani Nikula wrote: > > > > > > > > On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > > > > > > > > > Convert all scripts to use /bin/sh shebang and fix all shellcheck > > > > > reported problems. > > > > > > > > Pro-tip, this is the place reserved in commit messages for describing > > > > *why* you think the change is needed or for the better. ;) > > > > > > And this reply here seems to be the place where I'm asking why we don't > > > switch to C if we go through all this effort. I don't really see what sh > > > over bash buys us (and you can pretty much expect me to re-add bashism the > > > next time around I touch these ...). > > > > Oh, totally forgot due to writing the huge RFC e-mail about it. One > > could amend the commit message with "to able to run on non-bash > > shells.", if it wasn't yet merged. > > I learned this from the kids: Why? We can branch the dialog here, I have the ready answers ;) a) So you can avoid compiling bash. So you can build faster. Because faster is better. b) Also because you can avoid including bash in initrd. Because it results in a smaller image with less dependencies. Because smaller _is_ better. Because it is faster to load over UEFI PXE. Because faster better. Did I mention faster is better? Regards, Joonas
On Tue, 07 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On ti, 2017-02-07 at 15:32 +0200, Jani Nikula wrote: >> > On Tue, 07 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: >> > >> > On ma, 2017-02-06 at 17:40 +0100, Daniel Vetter wrote: >> > > >> > > On Mon, Feb 06, 2017 at 03:25:27PM +0200, Jani Nikula wrote: >> > > > >> > > > On Mon, 06 Feb 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: >> > > > > >> > > > > Convert all scripts to use /bin/sh shebang and fix all shellcheck >> > > > > reported problems. >> > > > >> > > > Pro-tip, this is the place reserved in commit messages for describing >> > > > *why* you think the change is needed or for the better. ;) >> > > >> > > And this reply here seems to be the place where I'm asking why we don't >> > > switch to C if we go through all this effort. I don't really see what sh >> > > over bash buys us (and you can pretty much expect me to re-add bashism the >> > > next time around I touch these ...). >> > >> > Oh, totally forgot due to writing the huge RFC e-mail about it. One >> > could amend the commit message with "to able to run on non-bash >> > shells.", if it wasn't yet merged. >> >> I learned this from the kids: Why? > > We can branch the dialog here, I have the ready answers ;) > > a) So you can avoid compiling bash. So you can build faster. Because > faster is better. > > b) Also because you can avoid including bash in initrd. Because it > results in a smaller image with less dependencies. Because smaller _is_ > better. Because it is faster to load over UEFI PXE. Because faster > better. > > Did I mention faster is better? And finally we're getting at the roots of the reason, you want to reduce the size of the dependencies for environments where you have to bootstrap the entire environment, e.g. initrd. Is that it? That *might* have some merit. BR, Jani.
diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 6e07d93..dcc80c5 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -242,7 +242,6 @@ TESTS_scripts = \ debugfs_emon_crash \ drv_debugfs_reader \ sysfs_l3_parity \ - test_rte_check \ tools_test \ $(NULL) diff --git a/tests/check_drm_clients b/tests/check_drm_clients index 2a891b8..25c03b1 100755 --- a/tests/check_drm_clients +++ b/tests/check_drm_clients @@ -1,6 +1,13 @@ -#!/bin/bash +#!/bin/sh -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" + +exit "$IGT_EXIT_SUCCESS" -exit $IGT_EXIT_SUCCESS diff --git a/tests/ddx_intel_after_fbdev b/tests/ddx_intel_after_fbdev index f068209..2ca5ce0 100755 --- a/tests/ddx_intel_after_fbdev +++ b/tests/ddx_intel_after_fbdev @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh # # Testcase: Load Intel DDX after fbdev was loaded # @@ -9,7 +9,7 @@ whoami | grep -q root || { } # no other X session should be running -find /tmp/ -name .X*lock 2>/dev/null | grep -q X && { +find /tmp/ -name ".X*lock" 2>/dev/null | grep -q X && { echo "ERROR: X session already running" exit 1 } @@ -19,14 +19,14 @@ TMPDIR=$(mktemp -d /tmp/igt.XXXX) || { exit 1 } -cat > $TMPDIR/xorg.conf.fbdev << EOF +cat > "$TMPDIR/xorg.conf.fbdev" << EOF Section "Device" Driver "fbdev" Identifier "Device[fbdev]" EndSection EOF -cat > $TMPDIR/xorg.conf.intel << EOF +cat > "$TMPDIR/xorg.conf.intel" << EOF Section "Device" Driver "intel" Identifier "Device[intel]" @@ -34,40 +34,40 @@ EndSection EOF # log before fbdev -dmesg -c > $TMPDIR/dmesg.1.before.fbdev -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.1.before.fbdev +dmesg -c > "$TMPDIR/dmesg.1.before.fbdev" +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.1.before.fbdev" # run fbdev -xinit -- /usr/bin/X -config $TMPDIR/xorg.conf.fbdev & +xinit -- /usr/bin/X -config "$TMPDIR/xorg.conf.fbdev" & sleep 5 -if [ -f `which intel_reg` ]; then -`which intel_reg` dump > $TMPDIR/intel_reg_dump.1.fbdev +if [ -f "$(which intel_reg)" ]; then + intel_reg dump > "$TMPDIR/intel_reg_dump.1.fbdev" fi killall X # log after fbdev & before intel -dmesg -c > $TMPDIR/dmesg.2.after.fbdev.before.intel -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.2.after.fbdev.before.intel +dmesg -c > "$TMPDIR/dmesg.2.after.fbdev.before.intel" +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.2.after.fbdev.before.intel" sleep 5 # run intel -xinit -- /usr/bin/X -config $TMPDIR/xorg.conf.intel & -sleep 5 -if [ -f `which intel_reg` ]; then -`which intel_reg` dump > $TMPDIR/intel_reg_dump.2.intel +xinit -- /usr/bin/X -config "$TMPDIR/xorg.conf.intel" & +sleep 5 +if [ -f "$(which intel_reg)" ]; then + intel_reg dump > "$TMPDIR/intel_reg_dump.2.intel" fi killall X # log after intel -dmesg -c > $TMPDIR/dmesg.3.after.intel -cp /var/log/Xorg.0.log $TMPDIR/Xorg.0.log.3.after.intel +dmesg -c > "$TMPDIR/dmesg.3.after.intel" +cp /var/log/Xorg.0.log "$TMPDIR/Xorg.0.log.3.after.intel" -cp $0 $TMPDIR/ +cp "$0" "$TMPDIR/" -tar czf $TMPDIR.tar.gz $TMPDIR/* -if [ -f $TMPDIR.tar.gz ]; then - echo $TMPDIR.tar.gz contains this script, all configs and logs generated on this tests +tar czf "$TMPDIR.tar.gz" "$TMPDIR/" +if [ -f "$TMPDIR.tar.gz" ]; then + echo "'$TMPDIR.tar.gz' contains this script, all configs and logs generated on this tests" fi exit 0 diff --git a/tests/debugfs_emon_crash b/tests/debugfs_emon_crash index 1dbfcb2..11b1c44 100755 --- a/tests/debugfs_emon_crash +++ b/tests/debugfs_emon_crash @@ -1,16 +1,22 @@ -#!/bin/bash +#!/bin/sh # # This check if we can crash the kernel with segmentation-fault # by reading /sys/kernel/debug/dri/0/i915_emon_status too quickly # -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" for z in $(seq 1 1000); do - cat $i915_dfs_path/i915_emon_status > /dev/null 2&>1 + cat "$i915_dfs_path/i915_emon_status" > /dev/null 2>&1 done # If we got here, we haven't crashed +exit "$IGT_EXIT_SUCCESS" -exit $IGT_EXIT_SUCCESS diff --git a/tests/debugfs_wedged b/tests/debugfs_wedged index f15ac46..26b40de 100755 --- a/tests/debugfs_wedged +++ b/tests/debugfs_wedged @@ -1,10 +1,16 @@ -#!/bin/bash +#!/bin/sh -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" # Testcase: wedge the hw to check the error_state reading -# +# # Unfortunately wedged is permanent, so this test is not run by default -echo 1 > ${i915_dfs_path}/i915_wedged -cat $i915_dfs_path/i915_error_state > /dev/null 2>&1 +echo 1 > "${i915_dfs_path}/i915_wedged" +cat "$i915_dfs_path/i915_error_state" > /dev/null 2>&1 diff --git a/tests/drm_getopt.sh b/tests/drm_getopt.sh index a94a759..10f367c 100755 --- a/tests/drm_getopt.sh +++ b/tests/drm_getopt.sh @@ -1,3 +1,4 @@ +#!/bin/sh # This is required for check/distcheck target as it has to --list-subtests # for each test. Source it if you add a new test in form of a (shell) script. @@ -9,30 +10,31 @@ IGT_EXIT_SUCCESS=0 IGT_EXIT_INVALID=79 IGT_EXIT_FAILURE=99 -# hacked-up long option parsing -for arg in $@ ; do - case $arg in +PROG=$(basename "$THIS_SCRIPT") + +for arg do + case "$arg" in --list-subtests) - exit $IGT_EXIT_INVALID + exit "$IGT_EXIT_INVALID" ;; --run-subtest) - exit $IGT_EXIT_INVALID + exit "$IGT_EXIT_INVALID" ;; --debug) IGT_LOG_LEVEL=debug ;; --help-description) - echo $IGT_TEST_DESCRIPTION - exit $IGT_EXIT_SUCCESS + echo "$IGT_TEST_DESCRIPTION" + exit "$IGT_EXIT_SUCCESS" ;; --help) - echo "Usage: `basename $0` [OPTIONS]" + echo "Usage: $PROG [OPTIONS]" echo " --list-subtests" echo " --run-subtest <pattern>" echo " --debug" echo " --help-description" echo " --help" - exit $IGT_EXIT_SUCCESS + exit "$IGT_EXIT_SUCCESS" ;; esac done diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh index af104ad..2aa073d 100755 --- a/tests/drm_lib.sh +++ b/tests/drm_lib.sh @@ -1,23 +1,22 @@ -#!/bin/bash +#!/bin/sh -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_getopt.sh +. "$SOURCE_DIR/drm_getopt.sh" -NAME=$(basename "$0") +NAME=$(basename "$THIS_SCRIPT") dynamic_debug= hda_dynamic_debug_enable() { if [ -e "$dynamic_debug" ]; then - echo -n "module snd_hda_intel +pf" > $dynamic_debug - echo -n "module snd_hda_core +pf" > $dynamic_debug + echo "module snd_hda_intel +pf" > "$dynamic_debug" + echo "module snd_hda_core +pf" > "$dynamic_debug" fi } hda_dynamic_debug_disable() { if [ -e "$dynamic_debug" ]; then - echo -n "module snd_hda_core =_" > $dynamic_debug - echo -n "module snd_hda_intel =_" > $dynamic_debug + echo "module snd_hda_core =_" > "$dynamic_debug" + echo "module snd_hda_intel =_" > "$dynamic_debug" fi } @@ -31,7 +30,9 @@ KERN_INFO="<6>" KERN_DEBUG="<7>" kmsg() { - echo "$@" > /dev/kmsg + if [ -w "/dev/kmsg" ]; then + echo "$@" > /dev/kmsg + fi } finish() { @@ -46,16 +47,16 @@ kmsg "${KERN_INFO}[IGT] $NAME: executing" skip() { echo "$@" - exit $IGT_EXIT_SKIP + exit "$IGT_EXIT_SKIP" } die() { echo "$@" - exit $IGT_EXIT_FAILURE + exit "$IGT_EXIT_FAILURE" } do_or_die() { - $@ > /dev/null 2>&1 || (echo "FAIL: $@ ($?)" && exit $IGT_EXIT_FAILURE) + "$@" > /dev/null 2>&1 || ( echo "FAIL: $* ($?)" && exit "$IGT_EXIT_FAILURE" ) } if [ -d /sys/kernel/debug ]; then @@ -66,47 +67,47 @@ else skip "debugfs not found" fi -dynamic_debug=$debugfs_path/dynamic_debug/control +dynamic_debug="$debugfs_path/dynamic_debug/control" if [ ! -e "$dynamic_debug" ]; then echo "WARNING: dynamic debug control not available" fi -if [ ! -d $debugfs_path/dri ]; then +if [ ! -d "$debugfs_path/dri" ]; then skip "dri debugfs not found" fi -i915_dfs_path=x -for minor in `seq 0 16`; do - if [ -f $debugfs_path/dri/$minor/i915_error_state ] ; then - i915_dfs_path=$debugfs_path/dri/$minor +i915_dfs_path= +for minor in $(seq 0 16); do + if [ -f "$debugfs_path/dri/$minor/i915_error_state" ] ; then + i915_dfs_path="$debugfs_path/dri/$minor" break fi done -if [ $i915_dfs_path = "x" ] ; then - skip " i915 debugfs path not found." +if [ -z "$i915_dfs_path" ] ; then + skip "i915 debugfs path not found." fi # read everything we can -if [ `cat $i915_dfs_path/clients | wc -l` -gt "2" ] ; then +if [ "$(wc -l "$i915_dfs_path/clients")" -gt 2 ] ; then [ -n "$DRM_LIB_ALLOW_NO_MASTER" ] || \ die "ERROR: other drm clients running" fi -whoami | grep -q root || ( echo ERROR: not running as root; exit $IGT_EXIT_FAILURE ) +whoami | grep -q root || ( echo "ERROR: not running as root"; exit "$IGT_EXIT_FAILURE" ) i915_sfs_path= if [ -d /sys/class/drm ] ; then sysfs_path=/sys/class/drm - if [ -f $sysfs_path/card$minor/error ] ; then + if [ -f "$sysfs_path/card$minor/error" ] ; then i915_sfs_path="$sysfs_path/card$minor" fi fi # sysfs may not exist as the 'error' is a new interface in 3.11 -function drmtest_skip_on_simulation() +drmtest_skip_on_simulation() { - [ -n "$INTEL_SIMULATION" ] && exit $IGT_EXIT_SKIP + [ -n "$INTEL_SIMULATION" ] && exit "$IGT_EXIT_SKIP" } drmtest_skip_on_simulation diff --git a/tests/drv_debugfs_reader b/tests/drv_debugfs_reader index 6ea4e64..58d02de 100755 --- a/tests/drv_debugfs_reader +++ b/tests/drv_debugfs_reader @@ -1,9 +1,16 @@ -#!/bin/bash +#!/bin/sh -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" # read everything we can -cat $i915_dfs_path/* > /dev/null 2>&1 +cat "$i915_dfs_path"/* > /dev/null 2>&1 + +exit "$IGT_EXIT_SUCCESS" -exit $IGT_EXIT_SUCCESS diff --git a/tests/igt_command_line.sh b/tests/igt_command_line.sh index a20e44c..bab3a77 100755 --- a/tests/igt_command_line.sh +++ b/tests/igt_command_line.sh @@ -26,11 +26,11 @@ # if [ -z "$top_builddir" ]; then - top_builddir="$(dirname $0)" + top_builddir=$(dirname "$0") fi # allow to run this script from top directory -TESTLIST=`cat $top_builddir/test-list.txt` +TESTLIST=$(cat "$top_builddir/test-list.txt") if [ $? -ne 0 ]; then # distcheck requires this hack TESTLIST=$(cat test-list.txt) @@ -51,7 +51,7 @@ for test in $TESTLIST; do fi # top_builddir is empty for distcheck - test=$top_builddir/$test + test="$top_builddir/$test" # distcheck requires this hack if [ ! -x "$test" ]; then @@ -62,29 +62,29 @@ for test in $TESTLIST; do # check invalid option handling echo " Checking invalid option handling..." - ./$test --invalid-option 2> /dev/null && fail $test + "./$test" --invalid-option 2> /dev/null && fail "$test" # check valid options succeed echo " Checking valid option handling..." - ./$test --help > /dev/null || fail $test + "./$test" --help > /dev/null || fail "$test" # check --list-subtests works correctly echo " Checking subtest enumeration..." - LIST=`./$test --list-subtests` + LIST=$("./$test" --list-subtests) RET=$? - if [ $RET -ne 0 -a $RET -ne 79 ]; then - fail $test + if [ "$RET" -ne 0 -a $RET -ne 79 ]; then + fail "$test" fi - if [ $RET -eq 79 -a -n "$LIST" ]; then - fail $test + if [ "$RET" -eq 79 -a -n "$LIST" ]; then + fail "$test" fi - if [ $RET -eq 0 -a -z "$LIST" ]; then - fail $test + if [ "$RET" -eq 0 -a -z "$LIST" ]; then + fail "$test" fi # check invalid subtest handling echo " Checking invalid subtest handling..." - ./$test --run-subtest invalid-subtest > /dev/null 2>&1 && fail $test + "./$test" --run-subtest invalid-subtest > /dev/null 2>&1 && fail "$test" done diff --git a/tests/sysfs_l3_parity b/tests/sysfs_l3_parity index d5f3284..37a03f1 100755 --- a/tests/sysfs_l3_parity +++ b/tests/sysfs_l3_parity @@ -1,22 +1,30 @@ -#!/bin/bash +#!/bin/sh -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e || exit $? +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -e || exit $? #Check that we can remap a row -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -d -disabled=`$SOURCE_DIR/../tools/intel_l3_parity -l | grep -c 'Row 0, Bank 0, Subbank 0 is disabled'` -if [ "$disabled" != "1" ] ; then +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -d +disabled=$("$SOURCE_DIR/../tools/intel_l3_parity" -l | grep -c 'Row 0, Bank 0, Subbank 0 is disabled') +if [ "$disabled" != 1 ] ; then echo "Fail" - exit $IGT_EXIT_FAILURE + exit "$IGT_EXIT_FAILURE" fi -$SOURCE_DIR/../tools/intel_l3_parity -r 0 -b 0 -s 0 -e +"$SOURCE_DIR/../tools/intel_l3_parity" -r 0 -b 0 -s 0 -e + #Check that we can clear remaps -if [ `$SOURCE_DIR/../tools/intel_l3_parity -l | wc -l` != 1 ] ; then +can_clear=$("$SOURCE_DIR/../tools/intel_l3_parity" -l | wc -l) +if [ "$can_clear" != 1 ] ; then echo "Fail 2" - exit $IGT_EXIT_FAILURE + exit "$IGT_EXIT_FAILURE" fi diff --git a/tests/test_rte_check b/tests/test_rte_check deleted file mode 100755 index 2a891b8..0000000 --- a/tests/test_rte_check +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/bash - -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh - -exit $IGT_EXIT_SUCCESS diff --git a/tests/tools_test b/tests/tools_test index a27fb87..dd10531 100755 --- a/tests/tools_test +++ b/tests/tools_test @@ -1,16 +1,21 @@ -#!/bin/bash +#!/bin/sh # Test some of the most critical tools we have accidentally broken before. # TODO: Possibly make tests parse output -SOURCE_DIR="$( dirname "${BASH_SOURCE[0]}" )" -. $SOURCE_DIR/drm_lib.sh +if [ -n "$BASH_SOURCE" ]; then + THIS_SCRIPT=$BASH_SOURCE +else + THIS_SCRIPT=$0 +fi + +SOURCE_DIR=$(dirname "$THIS_SCRIPT") +. "$SOURCE_DIR/drm_lib.sh" # ARB_MODE has existed for many gens -PATH=$SOURCE_DIR/../tools:$PATH +PATH="$SOURCE_DIR/../tools:$PATH" do_or_die "intel_reg read 0x4030" do_or_die "intel_reg dump" # TODO: Add more tests -exit $IGT_EXIT_SUCCESS - +exit "$IGT_EXIT_SUCCESS"