[LEDE-DEV] [PATCH] package/utils/ct-bugcheck/src/bugcheck.sh * Double quote to prevent globbing and word splitting. * { cmd1; cmd2; } >> file instead of individual redirects. * fix formating
Mathias Kresin
dev at kresin.me
Mon Oct 3 23:55:58 PDT 2016
04.10.2016 03:23, Jan-Tarek Butt:
Hey Jan-Tarek please use a proper commit title and message. Something like:
ct-bugcheck: fix globbing, word splitting and formatting
Double quote to prevent globbing and word splitting. Use { cmd1; cmd2; }
>> file instead of individual redirects because of XXXXX.
Find more comments inline.
> Signed-off-by: Jan-Tarek Butt <tarek at ring0.de>
> ---
> package/utils/ct-bugcheck/src/bugcheck.sh | 154 +++++++++++++++---------------
> 1 file changed, 77 insertions(+), 77 deletions(-)
>
> diff --git a/package/utils/ct-bugcheck/src/bugcheck.sh b/package/utils/ct-bugcheck/src/bugcheck.sh
> index 85f70c5..a930440 100755
> --- a/package/utils/ct-bugcheck/src/bugcheck.sh
> +++ b/package/utils/ct-bugcheck/src/bugcheck.sh
> @@ -9,49 +9,51 @@ FOUND_BUG=0
>
> # set -x
>
> -bugcheck_generic()
> -{
> - echo "LEDE crashlog report" > $CRASHDIR/info.txt
> - date >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "uname" >> $CRASHDIR/info.txt
> - uname -a >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "os-release" >> $CRASHDIR/info.txt
> - cat /etc/os-release >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "os-release" >> $CRASHDIR/info.txt
> - cat /etc/os-release >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "dmesg output" >> $CRASHDIR/info.txt
> - dmesg >> $CRASHDIR/info.txt
> - if [ -x /usr/bin/lspci ]
> - then
> - echo >> $CRASHDIR/info.txt
> - echo "lspci" >> $CRASHDIR/info.txt
> - lspci >> $CRASHDIR/info.txt
> +bugcheck_generic() {
> + {
> + echo "LEDE crashlog report"
Space vs tab. I know the script heavily mixes spaces and tabs. If not
already done, would you please take care of this as well?
> + date
> + echo
> + echo "uname"
> + uname -a
> + echo
> + echo "os-release"
> + cat /etc/os-release
> + echo
> + echo "os-release"
> + cat /etc/os-release
> + echo
> + echo "dmesg output"
> + dmesg
> + } > $CRASHDIR/info.txt
> + if [ -x /usr/bin/lspci ]; then
> + {
> + echo
> + echo "lspci"
> + lspci
> + } >> $CRASHDIR/info.txt
> fi
What about using the short syntax here:
[ -x /usr/bin/lspci ] && {
echo
echo "lspci"
lspci
} > test.txt
I would prefer to have all conditions in the script using the the short
syntax as it seam to be the standard in the LEDE shell scripts.
> - echo >> $CRASHDIR/info.txt
> - echo "cpuinfo" >> $CRASHDIR/info.txt
> - cat /proc/cpuinfo >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "meminfo" >> $CRASHDIR/info.txt
> - cat /proc/cpuinfo >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "cmdline" >> $CRASHDIR/info.txt
> - cat /proc/cmdline >> $CRASHDIR/info.txt
> - echo >> $CRASHDIR/info.txt
> - echo "lsmod" >> $CRASHDIR/info.txt
> - lsmod >> $CRASHDIR/info.txt
> + {
> + echo
> + echo "cpuinfo"
> + cat /proc/cpuinfo
> + echo
> + echo "meminfo"
> + cat /proc/cpuinfo
> + echo
> + echo "cmdline"
> + cat /proc/cmdline
> + echo
> + echo "lsmod"
> + lsmod
> + } >> $CRASHDIR/info.txt
> }
>
> roll_crashes()
> {
> # Roll any existing crashes
> - if [ -d $CRASHDIR ]
> - then
> - if [ -d $CRASHDIR.1 ]
> - then
> + if [ -d $CRASHDIR ]; then
> + if [ -d $CRASHDIR.1 ]; then
Can't this simplified by using:
[ -d $CRASHDIR.1 ] && rm -fr $CRASHDIR.1
[ -d $CRASHDIR ] && mv $CRASHDIR $CRASHDIR.1
IMHO way better to read.
> rm -fr $CRASHDIR.1
> fi
> mv $CRASHDIR $CRASHDIR.1
> @@ -62,48 +64,46 @@ roll_crashes()
> }
>
> # ath10k, check debugfs entries.
> -for i in /sys/kernel/debug/ieee80211/*/ath10k/fw_crash_dump
> -do
> - #echo "Checking $i"
> - if cat $i > $TMPLOC/ath10k_crash.bin 2>&1
> - then
> - FOUND_BUG=1
> -
> - #echo "Found ath10k crash data in $i"
> - roll_crashes
> -
> - ADIR=${i/fw_crash_dump/}
> -
> - CTFW=0
> - if grep -- -ct- $TMPLOC/ath10k_crash.bin > /dev/null 2>&1
> - then
> - CTFW=1
> - fi
> -
> - echo "Send bug reports to:" > $CRASHDIR/report_to.txt
> - if [ -f $ADIR/ct_special -o $CTFW == "1" ]
> - then
> - # Looks like this is CT firmware or driver...
> - echo "greearb at candelatech.com" >> $CRASHDIR/report_to.txt
> - echo "and/or report or check for duplicates here:" >> $CRASHDIR/report_to.txt
> - echo "https://github.com/greearb/ath10k-ct/issues" >> $CRASHDIR/report_to.txt
> - else
> - # Not sure who would want these bug reports for upstream...
> - echo "https://www.lede-project.org/" >> $CRASHDIR/report_to.txt
> - fi
> - echo >> $CRASHDIR/report_to.txt
> - echo "Please attach all files in this directory to bug reports." >> $CRASHDIR/report_to.txt
> -
> - mv $TMPLOC/ath10k_crash.bin $CRASHDIR
> -
> - # Add any more ath10k specific stuff here.
> -
> - # And call generic bug reporting logic
> - bugcheck_generic
> - fi
> +for i in /sys/kernel/debug/ieee80211/*/ath10k/fw_crash_dump; do
> + #echo "Checking $i"
> + if cat "$i" > $TMPLOC/ath10k_crash.bin 2>&1; then
Shouldn't this something like:
cat "$i" > $TMPLOC/ath10k_crash.bin 2>&1 || continue
It's better to get what is done here if we exit/continue early here
> + FOUND_BUG=1
Space vs tab.
> +
> + #echo "Found ath10k crash data in $i"
> + roll_crashes
> +
> + ADIR=${i/fw_crash_dump/}
> +
> + CTFW=0
> + if grep -- -ct- $TMPLOC/ath10k_crash.bin > /dev/null 2>&1; then
> + CTFW=1
> + fi
This can be simplified to:
grep -q -- -ct- $TMPLOC/ath10k_crash.bin && CTFW=1
> +
> + echo "Send bug reports to:" > $CRASHDIR/report_to.txt
> + if [ -f "$ADIR"/ct_special ] || [ $CTFW = "1" ]; then
> + # Looks like this is CT firmware or driver...
> + {
> + echo "greearb at candelatech.com"
> + echo "and/or report or check for duplicates here:"
> + echo "https://github.com/greearb/ath10k-ct/issues"
> + } >> $CRASHDIR/report_to.txt
> + else
> + # Not sure who would want these bug reports for upstream...
> + echo "https://www.lede-project.org/" >> $CRASHDIR/report_to.txt
> + fi
> + echo >> $CRASHDIR/report_to.txt
> + echo "Please attach all files in this directory to bug reports." >> $CRASHDIR/report_to.txt
> +
> + mv $TMPLOC/ath10k_crash.bin $CRASHDIR
> +
> + # Add any more ath10k specific stuff here.
> +
> + # And call generic bug reporting logic
> + bugcheck_generic
> + fi
> done
>
> -if [ $FOUND_BUG == "1" ]
> +if [ $FOUND_BUG = "1" ]
please us an integer comparison here ([ "$a" -eq "$b" ] && { do something})
> then
> # Notify LUCI somehow?
> echo "bugcheck.sh found an issue to be reported" > /dev/kmsg
>
More information about the Lede-dev
mailing list