Linux backup script written in bash for tar

Question

This is my first attempt at scripting with bash. I am running Ubuntu server, so I don't get to use all the gui back ups out there. Besides, tar works fine for personal use. Please let me know of any improvements and why.

#!/bin/bash

# PURPOSE:
# TO BACK UP ALL FILES AND FOLDERS FROM A GIVEN DIRECTORY
# Directory to back up is set with the constant BACK_UP_LOCATION
# Directory to save the back up to is /IT/backup/ and is set with the constant BACK_UP_DIR
# Directory to save the log files to is /IT/backup/log/ and is set by changing the tail end of the constant BACK_UP_LOG_DIR
# Obviously additional directories can be added to the back up by adding additional constants and incorporating that tar command(s) to go with them

# Written by Rowland Holden

# This script could have used the find command to delete the old files
# but that wouldn't have allowed for the logging of each deleted file so
# I chose to do it the long way to get the logging I wanted

# File Info: tar-share-backup @ cron.daily




# Set constants

NOW=$(date +%m-%d-%Y)
BACK_UP_DIR="IT/backup/"
BACK_UP_PATH="${BACK_UP_DIR}ShareBackup${NOW}"
BACK_UP_LOG_DIR="${BACK_UP_DIR}log/"
BACK_UP_LOG_PATH="${BACK_UP_LOG_DIR}ShareBackupLog${NOW}"
BACK_UP_LOCATION="srv/share/"

cd /

# check if script is already running

write_pid_running_error () {

  local  errorText="${0} is already running: canceling backup operation"

  if [[ -f "${BACK_UP_LOG_PATH}" ]]; then
    echo "" >> "${BACK_UP_LOG_PATH}"
    echo "${errorText}" >> "${BACK_UP_LOG_PATH}"
  else
    echo "${errorText}" > "${BACK_UP_LOG_PATH}"
  fi


}

for pid in $(pidof -x `basename $0`); do
  if [[ $pid != $$ ]]; then
    write_pid_running_error
    exit 1
  fi
done


# Check to see if the back up file already exists and write an error in the log if it does

# write error to log file if back up file exists
write_file_exists_error () {

  local  errorText="backup file already exists: canceling backup operation"

  if [[ -f "${BACK_UP_LOG_PATH}" ]]; then
    echo "" >> "${BACK_UP_LOG_PATH}"
    echo "${errorText}" >> "${BACK_UP_LOG_PATH}"
  else
    echo "${errorText}" > "${BACK_UP_LOG_PATH}"
  fi

}

# check to see if th back up tar file already exists - if so let the user know in the log and exit -
# do not continue because we don't know if the user wants to rm the file or not
check_file () {

  if [[ -f "${BACK_UP_PATH}.tar.gz" ]]; then
    write_file_exists_error
    exit 1
  fi

}

del_files () {

  local filepath="${1}"
  local modDate="${2}"
  local file="${3}"
  local dir="${4}"

  local delCutOffDate=$(date -d "$date -7 days" +%m-%d-%Y) # sets date to 7 days ago

  if [[ -f "${filepath}" ]]; then # make sure $file is an actual file and not a directory
    if [[ "${modDate}" -ot "${delCutOffDate}" ]]; then  # Compares the file modified date to the cut off date for deletion - anything older than 7 days
      echo "  ${file} deleted from ${dir}" >> "${BACK_UP_LOG_PATH}"
      echo "    ModDate: ${modDate}" >> "${BACK_UP_LOG_PATH}"
      echo "    Cut Off: ${delCutOffDate}" >> "${BACK_UP_LOG_PATH}"
      echo "    test: ${modDate} -ot ${delCustOffDate}" >> "${BACK_UP_LOG_PATH}"
      rm "${filepath}" >> "${BACK_UP_LOG_PATH}"
    fi
  fi
}

cycle_files () {

  local files=("$@")
  local dir="${files[-1]}" # $ is set on the number of arguments, so this wouldn't be $1, it would be $ whatever the last index in the argument array is
         # This is because the number of files are dynamic, making the array indexing dynamic. making this way the best to obtain the dir we want

  unset 'files[-1]' # remove the directory as filepath prefixes the file with the directory - prefixing a directory to a directory will throw an error

  for file in "${files[@]}"; do # for each file in the current directory do the following loop
    filepath="${dir}${file}" # prefix the filename with the directory path for file manipulation
    modDate=$(date -r "${filepath}" +%m-%d-%Y) # Gets modified date of a file

    del_files "${filepath}" "${modDate}" "${file}" "${dir}"
  done

}

cycle_dirs () {

  local dirs=("$@")

  for dir in "${dirs[@]}"; do # for each directory do the following loop
    files=($(ls "${dir}")) # list all files in $dir and put them in an array
    echo "Enterying: ${dir}" >> "${BACK_UP_LOG_PATH}"

    cycle_files "${files[@]}" "${dir}"
  done

}

delete_old_files () {

  echo "" >> "${BACK_UP_LOG_PATH}"
  echo "" >> "${BACK_UP_LOG_PATH}"
  echo "The folowing files were deleted:" >> "${BACK_UP_LOG_PATH}"
  echo "" >> "${BACK_UP_LOG_PATH}"

  dirs=( "${BACK_UP_DIR}" "${BACK_UP_LOG_DIR}" ) # create an array of directories to cycle through for deleting old back up and log files
  cycle_dirs  "${dirs[@]}"

}

check_file

# Back up files

echo "The following files were backed up:" > "${BACK_UP_LOG_PATH}" # if we have gotten this far we have a clean back up file so lets also do a clean log
echo "" >> "${BACK_UP_LOG_PATH}"

tar -czvf "${BACK_UP_PATH}.tar.gz" "${BACK_UP_LOCATION}" >> "${BACK_UP_LOG_PATH}"

# Delete old log and back up files
  delete_old_files

exit 0

The revised code and file for use is available on github: https://github.com/rowlandholden76/Bash-Backup

Answer 1

It might be better to define BACK_UP_PATH and BACK_UP_LOG_PATH as "${BACK_UP_DIR}/ShareBackup${NOW}" and "${BACK_UP_LOG_DIR}/ShareBackupLog${NOW}" -- notice the slashes. It is safe to have multiple slashes in a row in a path, so explicitly adding them might be clearer and also helps make sure the script works as intended if someone accidentally changes BACK_UP_DIR from IT/backup/ to something like /it/backup (with no trailing /). Same for the filepath="${dir}${file} line later -- it works fine but filepath="${dir}/${file}" is slightly clearer and slightly more robust

On a similar note, you don't seem to ever use $BACK_UP_PATH without manually adding ".tar.gz" to the file name in all locations. Why not just make that part of the BACK_UP_PATH variable?

The write_*_error functions are very similar. It seems like you could save yourself some repetition by just having one write_error function that takes the error message as a parameter

You have some unquoted variables in a few places. Most of them should be safe, like the $$ on line 49, but in my experience it's generally easier to just quote everything than to keep track of what's safe and what isn't. You never know, maybe some day you'll accidentally (or intentionally for... some reason) do something like export IFS=1 and end up splitting your PID into multiple words which seems like it'd be awkward. Not very like to happen, sure, but few phrases are as dangerous as "nobody would ever do that"

On line 48, you use backticks for command substitution, which doesn't really have any benefits over $() but can't be nested and is generally considered harder to read. You may also want to quote "$0" and the entire "$(basename "$0")" call. Coming from languages where string literals are a thing and tend to be very clear about ending as soon as they see a ", it can feel a bit weird to be able to nest quoted strings inside "$()", but it works and works well

On line 90, you're referencing a $date variable which doesn't seem to have ever been defined anywhere. I think you can just remove that -- date -d '-7 days' should work just fine

In one of the echos in del_files, $delCutOffDate is misspelled as $delCustOffDate

In the loop in cycle_dirs you set files from the output of ls, which is just a series of bytes with no unambiguous indication of where file names begin or end. The shell will happily do its usual thing of splitting based on where it finds whitespace, which can be awkward since file names can contain spaces, or even line breaks (they usually don't, but they can). But when * is expanded, each file name is its own word, so something like `files=("${dir}"/*) may work more reliably

I'm not entirely sure why you cd / at the start. If you want to operate on files relative to /, you could just have BACK_UP_PATH and BACK_UP_LOG_PATH be absolute paths, no?

Finally, as a general tip, bash (all the POSIX shells, really) is full of subtle pitfalls -- its rules are simple but not intuitive, and it's usually less likely to fail than to succeed in the wrong way. Having a program check your scripts for you is usually very useful. I'm a fan of shellcheck myself, and used it to find some of the things I pointed out in this review

Answer 2

NOW=$(date +%m-%d-%Y)

Prefer lower-case for variable names. This reduces the likelihood of conflict with environment variables used to change programs' behaviour (which are generally upper-case).

That odd date format (with the day in the middle) is going to make it hard to find your data. It's best to stick with standard (ISO 8601) formats, which naturally sort in order:

today=$(date +%F)

---

cd /

Never assume that cd will be successful. Get into the habit of always checking, either with explicit || exit 1 or implicitly by adding set -e at the beginning of the script.

---

  if [[ $pid != $$ ]]; then

No reason not to use plain portable [ here.

---

  local dirs=("$@")

  for dir in "${dirs[@]}"; do

There's not really any benefit in copying arguments to the local variable. Simplify:

for dir
do

(yes, that's all, since for iterates over "$@" unless in is given).

---

  local files=("$@")
  local dir="${files[-1]}"
  unset 'files[-1]'
  for file in "${files[@]}"; do

This would be easier if we passed the single dir argument first:

local dir=$1
shift
for file
do