#549 Issue closed: Check rear *.sh scripts for lonely words

Labels: enhancement, bug, fixed / solved / done

jsmeix opened issue at 2015-02-16 12:59:

This is an enhancement proposal for better syntax check of the scripts.

Currently only "bash -n" is run for the *.sh scripts which cannot detect many problems like misspelled variables, functions, commands and many more.

My personal reason is that I did misspell a variable (that was never commited to rear upstream) and I found it only by debugging a failure during my own testing.

I don't know a good bash syntax checker.

In particular for rear we would neeed a bash syntax checker that can check the overall syntax of all *.sh scripts.

Therefore as a first attempt to do a little better syntax check I like to propse hereby to check for words that exist only once in all *.sh scripts.

What I did right now as a very first proof of concept in a rear source directory:

$ rm /tmp/rear_all_scripts.sh

$ find . -name '*.sh' | xargs cat >>/tmp/rear_all_scripts.sh

$ cat /tmp/rear_all_scripts.sh | grep -v '^[[:space:]]*#' | tr -c -s '[:space:][:alnum:]_-' ' ' | tr -s '[:space:]' '\n' | sort | uniq -c | grep '^[[:space:]]*1 [[:alpha:]]' | cut -b9- >/tmp/rear_all_scripts.lonely_words

$ for w in $( cat /tmp/rear_all_scripts.lonely_words ) ; do echo $w ; find . -name '*.sh' | xargs egrep -h "[^[:alpha:]]$w[^[:alpha:]]|^$w[^[:alpha:]]" ; echo ------------------------------------------------------------------------------------------------------------------ ; done | grep -v '^[[:space:]]*#' | less

This way I found 1159 lonely words in all *.sh scripts.

Now I use my own brain and imagination to view that long "less" output for suspicious cases of lonely words that could be real bugs...

jsmeix commented at 2015-02-16 13:20:

I found for example:

The functions:

add_default_route ()

add_module () {

seem to be lonely functions (seems to be nowhere called).

The variable alias_prefix in

ip -4 addr add ${alias_ip_address}/${alias_prefix} dev ${interface}:0

seems to be used but nowhere defined.

I do not understand hwo disk_used-old_disk_used is used in

let disk_used="$(get_disk_used "$backuparchive")" size=disk_used-old_disk_used

it seems disk_used-old_disk_used is used but nowhere defined.

It seems ${fragmentsize} in usr/share/rear/layout/prepare/GNU/Linux/13_include_filesystem_code.sh is used but nowhere defined.

It seems $grub_name-mkconfig in usr/share/rear/finalize/Linux-i386/22_install_grub2.sh is used but nowhere defined.

It seems "$HPACUCLI_BIN_INSTALLATION_DIR" in usr/share/rear/layout/save/GNU/Linux/27_hpraid_layout.sh is used but nowhere documented (I guess it should be added to usr/share/rear/conf/default.conf).

It seems "${PEERDNS}" in usr/share/rear/lib/network-functions.sh is used but nowhere defined and/or documented (I guess it should be added to usr/share/rear/conf/default.conf).

It seems "${PROGS_DP[@]}" in usr/share/rear/prep/DP/default/40_prep_dp.sh is used but nowhere defined and/or documented (I guess it should be added to usr/share/rear/conf/default.conf).

It seems "${PROGS_SESAM[@]}" in usr/share/rear/prep/SESAM/default/40_prep_sesam.sh is used but nowhere defined and/or documented (I guess it should be added to usr/share/rear/conf/default.conf).

It seems "$SKIP_CFG2HTML" in usr/share/rear/rescue/GNU/Linux/95_cfg2html.sh is used but nowhere defined and/or documented (I guess it should be added to usr/share/rear/conf/default.conf).

It seems $SYSLINX_DIR in usr/share/rear/lib/bootloader-functions.sh is a misspelled variable name and should be $SYSLINUX_DIR

I do not undrstand how $VAR_LIB is used in usr/share/rear/prep/GNU/Linux/30_include_grub_tools.sh I think

[ ! -d $VAR_LIB/recovery ] && mkdir -p $VAR_DIR/recovery

should be

[ ! -d $VAR_DIR/recovery ] && mkdir -p $VAR_DIR/recovery

gdha commented at 2015-02-17 15:32:

I'll add my comments piece per piece to keep the overview readable for me (and the rest of our community):

  • disk_used-old_disk_used is used as follow:
        (rsync)
                let old_disk_used="$(get_disk_used "$backuparchive")"
                while sleep 1 ; kill -0 $BackupPID 2>&8; do
                        let disk_used="$(get_disk_used "$backuparchive")" size=disk_used-old_disk_used
                        #echo -en "\e[2K\rArchived $((size/1024/1024)) MiB [avg $((size/1024/(SECONDS-starttime))) KiB/sec]"
                        ProgressInfo "Archived $((size/1024/1024)) MiB [avg $((size/1024/(SECONDS-starttime))) KiB/sec]"

First we initialize the variable old_disk_used and in a while loop we recalculate the disk_used and size (so the disk_used-old_disk_used is a calculation executed by the let statement)

gdha commented at 2015-02-17 20:09:

  • It seems ${fragmentsize} in usr/share/rear/layout/prepare/GNU/Linux/13_include_filesystem_code.sh is used but nowhere defined
    =>
    it seems you are right (it was forgotten unfortunately) - I fixed it in the meantime

gdha commented at 2015-02-17 20:15:

  • It seems $grub_name-mkconfig in usr/share/rear/finalize/Linux-i386/22_install_grub2.sh is used but nowhere defined

Script ./usr/share/rear/finalize/Linux-i386/22_install_grub2.sh defines
grub_name="grub2" so $grub_name-mkconfig becomes grub2-mkconfig

I agree ${grub_name}-mkconfig would make it more readable

gdha commented at 2015-02-18 15:20:

  • HPACUCLI_BIN_INSTALLATION_DIR

is automatic filled by the eval call in the script (ugly hack I agree)

gdha commented at 2015-02-18 15:23:

  • PEERDNS

Is not used anywhere - removed it.

gdha commented at 2015-02-18 15:25:

  • prep/DP/default/40_prep_dp.sh : removed the "${PROGS_DP[@]}" as it is obsolete

gdha commented at 2015-02-18 15:27:

  • PROGS_SESAM

removed the line in ./prep/SESAM/default/40_prep_sesam.sh

gdha commented at 2015-02-24 09:15:

  • alias_prefix: looks ok to me
$ grep -r alias_prefix .
./skel/default/bin/dhclient-script:alias_prefix="$(get_prefix ${alias_ip_address} ${alias_subnet_mask})"
./skel/default/bin/dhclient-script:            ip -4 addr add ${alias_ip_address}/${alias_prefix} dev ${interface}:0
./lib/network-functions.sh:        ip -4 addr add ${alias_ip_address}/${alias_prefix} dev ${interface}:0

gdha commented at 2015-03-06 15:51:

reverted the fragmentsize change - see issue #558

gdha commented at 2015-03-25 19:20:

@jsmeix is this ok we close this issue?

jsmeix commented at 2015-03-26 09:00:

@gdha
my actual itent of this issue is to find out whether or not checking rear *.sh scripts for lonely words makes sense for better syntax check of the scripts.

Therefore I would like to know if you (and also the other rear upstream authors) think that such a check was actually helpful to find hidden actual issues or did it in the end result too many false positives so that it was not really worth the effort?

My question is:
Should I continue with it and further develop my above "very first proof of concept" into a (hopefully) more and more useful check or is it not worth the effort?

schlomo commented at 2015-03-26 10:27:

@jsmeix I think that what you do is very valuable and improves our software quality. Some day I would really like to enforce that with a set -e -E -u (and maybe others). The reason why I am not doing that right away is that ReaR will stop to work and that we don't have automated testing to help us get it back to work.

So what you do is actually the soft approach and much appreciated, at least by me.

jsmeix commented at 2015-04-10 09:46:

@gdha
you can close this issue.

When I enhance my current generic basic btrfs support (see #556) I will also enhance my above "very first proof of concept" and if I find suspicious items I will open a new issue.

jsmeix commented at 2015-04-10 09:46:

I found out that I can close the issue myself which I did.

[Export of Github issue for rear/rear.]