Bash script using rsync to backup (another one)

Question

I created a backup script in bash to basically backup my webservers using cron commands.

The script reads one or multiple config file(s), downloads a target directory, and can send by mail the rsync log. It can also keep a number of backup increments.

https://github.com/kokmok/my-simple-backup

#!/bin/bash
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
CONFIG_DIR="$SCRIPT_DIR/config.d"
CONFIG_FILES=$(ls "$CONFIG_DIR"/*)
CONFIG_NAME_REGEX='backup_name[[:space:]]([a-zA-Z0-9_./\/-]+)'
CONFIG_REPORTING='mail_report[[:space:]](YES|NO)'
CONFIG_REPORTING_ADDRESS='mail_report_address[[:space:]]([a-zA-Z0-9_./\/-]+@[a-zA-Z0-9_./\/-]+)'
USER_REGEX='user[[:space:]]([a-zA-Z0-9_-]+)'
HOST_REGEX='host[[:space:]]([a-zA-Z0-9_.-]+)'
SOURCE_REGEX='source_folder[[:space:]]([a-zA-Z0-9_./\/-]+)'
DEST_REGEX='dest_folder[[:space:]]([a-zA-Z0-9_./\/-]+)'
LIMIT_BACKUP_NUMBER_REGEX='limit_backup_number[[:space:]]([0-9]+)'
COMPRESS_REGEX='compress_backup[[:space:]](YES|NO)'
RSYNC_ERROR_REGEX='rsync[[:space:]]error'
get_config_part() {
 if [[ "1ドル" =~ 2ドル ]]
 then
 local value="${BASH_REMATCH[1]}"
 echo "$value"
 fi
}
run_config() {
 content=$(cat "1ドル")
 configName=$(get_config_part "$content" "$CONFIG_NAME_REGEX")
 reporting=$(get_config_part "$content" "$CONFIG_REPORTING")
 if [[ $reporting == "YES" ]]
 then
 reporting_address=$(get_config_part "$content" "$CONFIG_REPORTING_ADDRESS")
 fi
 user=$(get_config_part "$content" "$USER_REGEX")
 host=$(get_config_part "$content" "$HOST_REGEX")
 source=$(get_config_part "$content" "$SOURCE_REGEX")
 dest=$(get_config_part "$content" "$DEST_REGEX")
 limit_backup_number=$(get_config_part "$content" "$LIMIT_BACKUP_NUMBER_REGEX")
 compress=$(get_config_part "$content" "$COMPRESS_REGEX")
 if [[ ! -d "$SCRIPT_DIR/results" ]]
 then
 eval "mkdir $SCRIPT_DIR/results"
 fi
 result_file="$SCRIPT_DIR/results/result_$configName"
 if [[ ! -f "$result_file" ]]
 then
 eval "touch $result_file"
 fi
 eval "> $result_file"
 if [[ ${#user} == 0 || ${#host} == 0 || ${#source} == 0 || ${#dest} == 0 ]]
 then
 eval "echo \"[ERROR] bad configuration\" > $result_file"
 exit 1
 fi
# If not exist try to create it
 if [[ ! -d "$dest" ]]
 then
 eval "mkdir $dest > $result_file"
 fi
# if still not exists, exit
 if [[ ! -d "$dest" ]]
 then
 eval "echo \"[ERROR] bad configuration: dest directory not exists\" > $result_file"
 exit 1
 fi
 bkpFolderDate=$(date +"%Y-%m-%d-%H-%M-%S")
 eval "mkdir $dest/$bkpFolderDate > $result_file"
 command="rsync -avve ssh $user@$host:$source $dest/$bkpFolderDate --log-file=$result_file --timeout=10"
 eval "$command"
 if [[ $reporting == "YES" ]]
 then
 eval "cat $result_file | mail -s \"backup status of $configName\" $reporting_address"
 fi
 if [[ $compress == "YES" ]]
 then
 eval "tar -zcf $dest/$bkpFolderDate.tgz $dest/$bkpFolderDate"
 eval "rm -r $dest/$bkpFolderDate"
 fi
 if [[ $(cat "$result_file") =~ $RSYNC_ERROR_REGEX ]]
 then
 echo "rsync failed"
 eval "rm $dest/$bkpFolderDate -r"
 else
 echo "rsync succeeded"
 limit_backup_number=$((limit_backup_number+1))
 eval "(cd $dest && ls -tp | tail -n +$limit_backup_number | xargs -I {} rm -r -- {})"
 fi
}
if [[ 1ドル != "" ]]
then
 echo "running configuration of 1ドル"
 run_config "$CONFIG_DIR/1ドル"
else
 for entry in $CONFIG_FILES
 do
 if [[ "$entry" =~ 'sample' ]]
 then
 continue
 fi
 run_config $entry
 done
fi

I'm not really used to bash scripting, so what do you think about mine? Less syntactic, do you observe some weakness or missing points in this script?

Personally I don't like having dependencies to services the script cannot manage like msmtp but I don't know how I can manage that.

Answer 1

Unfortunately, because it is rather difficult for anyone else other than you to actually execute this code and test it, much of my advice will be cursory observations.

The first thing that catches my eye are the many if/then checks to see if certain files or directories exist. With directories, you can just use mkdir -p <dir>, which will handle that check. With files, you could use test -f.

The next thing is a minor stylistic choice, but most bash scripters prefer the if <>; then syntax style rather than having then on a new line.

Lastly, the biggest problem with this program would be the eval statements. Security concerns aside, you could just do touch "$result_file" instead of eval "touch $result_file" for example. Again, I'd use test here instead. Most of your variables are strings, so encapsulating variables in double quotes everywhere shouldn't be necessary.

• Toby Speight: I'm not convinced that most authors join if and then onto one line. If it's a majority, then probably only a small majority rather than an overwhelming one.
• Toby Speight: Yes, the evals are actively harmful, in that they remove a layer of quoting. You'd need eval "touch '${result_file//'/\\'}'", for example, to deal with arguments properly.

Answer 2

As T145 says, fix the avoidable if tests and evals first.

---

CONFIG_FILES=$(ls "$CONFIG_DIR"/*)

This won't work correctly when any of the file names contain whitespace. Since we're using Bash, we can expand into an array variable instead:

CONFIG_FILES=("$CONFIG_DIR"/*)

We then use it as

for entry in "${CONFIG_FILES[@]}"

---

if [[ ${#user} == 0 || ${#host} == 0 || ${#source} == 0 || ${#dest} == 0 ]]
then
 eval "echo \"[ERROR] bad configuration\" > $result_file"
 exit 1
fi

It's good to see configuration checking, and correct use of non-zero exit status. We could improve this by being more specific, and saying which value was wrong. Users would also expect the error message on standard output rather than (or as well as) having to rummage in the result file:

for v in user host source dest
do
 if [ -z "${!v}" ]
 then
 printf '[ERROR] configuration missing value for "%s"\n' "$v" |
 tee "$result_file" >&2
 exit 1
 fi
done

We could be even kinder, and build up a list of missing values, and print them all before we exit:

missing=()
for v in user host source dest
do test "${!v}" || missing+=("$v")
done
if [ "${missing[*]}" ]
then
 printf '[ERROR] configuration missing value for "%s"\n' "${missing[@]}" |
 tee "$result_file" >&2
 exit 1
fi

---

We can simplify this:

 do
 if [[ "$entry" =~ 'sample' ]]
 then
 continue
 fi
 run_config $entry
 done

Instead of using if and continue, we could just join the test to the single command with ||:

 do
 [[ "$entry" =~ 'sample' ]] || run_config $entry
 done

---

Minor (style): choose an indent level, and stick with it throughout. At present, some regions are indented by 4 characters and others by just 2 - sometimes in the same block, which is very hard to read.

Answer 3

Following all advices I edited my script. I removed all eval; removed a lot of if statements and fix my indent issue.

I also added some feature like the size of the transfert and the error status in the mail subject.

#!/bin/bash
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
CONFIG_DIR="$SCRIPT_DIR/config.d"
CONFIG_FILES=("$CONFIG_DIR"/*)
CONFIG_NAME_REGEX='backup_name[[:space:]]([a-zA-Z0-9_./\/-]+)'
CONFIG_REPORTING='mail_report[[:space:]](YES|NO)'
CONFIG_REPORTING_ADDRESS='mail_report_address[[:space:]]([a-zA-Z0-9_./\/-]+@[a-zA-Z0-9_./\/-]+)'
USER_REGEX='user[[:space:]]([a-zA-Z0-9_-]+)'
HOST_REGEX='host[[:space:]]([a-zA-Z0-9_.-]+)'
SOURCE_REGEX='source_folder[[:space:]]([a-zA-Z0-9_./\/-]+)'
DEST_REGEX='dest_folder[[:space:]]([a-zA-Z0-9_./\/-]+)'
LIMIT_BACKUP_NUMBER_REGEX='limit_backup_number[[:space:]]([0-9]+)'
COMPRESS_REGEX='compress_backup[[:space:]](YES|NO)'
RSYNC_ERROR_REGEX='rsync[[:space:]]error'
RECEIVED_REGEX='received[[:space:]]([0-9,]+)[[:space:]]bytes'
ERROR=false
get_config_part() {
 if [[ "1ドル" =~ 2ドル ]]
 then
 local value="${BASH_REMATCH[1]}"
 echo "$value"
 fi
}
run_config() {
 content=$(cat "1ドル")
 configName=$(get_config_part "$content" "$CONFIG_NAME_REGEX")
 reporting=$(get_config_part "$content" "$CONFIG_REPORTING")
 if [[ $reporting == "YES" ]]
 then
 reporting_address=$(get_config_part "$content" "$CONFIG_REPORTING_ADDRESS")
 fi
 user=$(get_config_part "$content" "$USER_REGEX")
 host=$(get_config_part "$content" "$HOST_REGEX")
 source=$(get_config_part "$content" "$SOURCE_REGEX")
 dest=$(get_config_part "$content" "$DEST_REGEX")
 limit_backup_number=$(get_config_part "$content" "$LIMIT_BACKUP_NUMBER_REGEX")
 compress=$(get_config_part "$content" "$COMPRESS_REGEX")
 mkdir -p "$SCRIPT_DIR/results"
 result_file="$SCRIPT_DIR/results/result_$configName"
 if [ ! -f "$result_file" ]; then touch "$result_file"; fi
 echo "" > "$result_file"
 missing_config_values=()
 for config_values in user host source dest
 do test "${!config_values}" || missing_config_values+=("$config_values")
 done
 if [ "${missing_config_values[*]}" ]
 then
 printf '[ERROR] configuration missing value for "%s"\n' "${missing_config_values[@]}" |
 tee "$result_file" >&2
 exit 1
 fi
# If not exist try to create it
 mkdir -p "$SCRIPT_DIR/results"
 mkdir -p "$dest"
 if [[ ! -d "$dest" ]]
 then
 echo "[ERROR] bad configuration: dest directory does not exists and cannot be created" | tee "$result_file" >&2
 exit 1
 fi
 bkpFolderDate=$(date +"%Y-%m-%d-%H-%M-%S")
 mkdir "$dest/$bkpFolderDate" > "$result_file" #not sure it returns something
 rsync -avve ssh "$user"@"$host":"$source" "$dest"/"$bkpFolderDate" --log-file="$result_file" --timeout=10
 if [ $compress == "YES" ]
 then
 tar -zcf "$dest/$bkpFolderDate.tgz $dest/$bkpFolderDate"
 rm -r "$dest/$bkpFolderDate"
 fi
 if [[ $(cat "$result_file") =~ $RSYNC_ERROR_REGEX ]]
 then
 echo "rsync failed"
 rm -r "$dest/$bkpFolderDate"
 ERROR=true
 else
 echo "rsync succeeded"
 limit_backup_number=$((limit_backup_number+1))
 cd "$dest" && ls -tp | tail -n +"$limit_backup_number" | xargs -I {} rm -r -- {}
 fi
 if [ $reporting == "YES" ]
 then
 if [ $ERROR == true ];then error_title="[ERROR]";else error_title="";fi
 if [[ $(cat "$result_file") =~ $RECEIVED_REGEX ]];then received="${BASH_REMATCH[1]}";else received="0";fi
 received=$(echo "$received" | sed 's/,//g')
 receivedInMo=$((received/1048576))
 cat "$result_file" | mail -s "$error_title backup status of $configName ($receivedInMo Mo)" "$reporting_address"
 fi
}
if [[ 1ドル != "" ]]
then
 echo "running configuration of 1ドル"
 run_config "$CONFIG_DIR/1ドル"
else
 for entry in "${CONFIG_FILES[@]}"
 do
 [[ "$entry" =~ 'sample' ]] || run_config $entry
 done
fi