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.

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