4
\$\begingroup\$

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

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see What should I do when someone answers my question? as well as what you may and may not do after receiving answers. \$\endgroup\$ Jun 10 at 0:01
  • 1
    \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I am very sorry. I do hope to clarify though. My intent was only to allow future users to see the difference in the code so they could better understand why the answers were better than what I had. I will not do this in the future. \$\endgroup\$
    – Riv
    Jun 10 at 20:50
  • \$\begingroup\$ We understand and it happens to many users. Like the Help center page mentions - "If you incorporate advice from one or more answers, but are still unsure that the code is as good as it should be, then post a new question with your revised code. For the benefit of other users, add mutual links: mention the previous question in the new question, and add a comment on the old question linking to the follow-up question." though you could put the link in the post instead of adding a comment. \$\endgroup\$ Jun 10 at 21:00
  • \$\begingroup\$ Or you could also take the route of #3 i.e. Sharing your code on an external site. \$\endgroup\$ Jun 10 at 21:15
5
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ I have a lot of work here. First changing IT to it. I wasn't sure if it would trip on a double \\. I can add the tar.gz to the end of the BACK_UP_PATH. I was lazy when I wrote the second write function as it was 2am. I will double check them the variables for double quoted. I will also correct the basename command. I got that one as an example from the internet. I will eliminate the $date variable. Thank you for the spell check on the echo line. Then when I looked I saw it in was some code that hasn't executed yet as I don't have more than 7 days of files. . \$\endgroup\$
    – Riv
    Jun 9 at 21:28
  • \$\begingroup\$ had to 2 comments to cover everything - I KNEW there was a better way to get the files array built, but I couldn't find a better way. Thanks, I will change that as well. The reason I use cd / is because when I tested tar with absolute paths I got an error. However, I do not get the error now. I was probably doing something wrong. I will change the variables to absolute that will make it much easier, and what I wanted to do to begin with - Thank you very much Sara. I will also check out shellcheck. \$\endgroup\$
    – Riv
    Jun 9 at 21:28
5
\$\begingroup\$
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
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for this. I will change all the upper case variable names to lower case. This is a hold over from other languages I have experience with. I will change the date format to match the ISO. I think I will use || exit 1 as I need it to exit if it doesn't change to root. About not storing in a local variable, I wasn't sure if the variables would be still in scope as they are defined further down the page. so I can change that too. If I do that then I wont have a reason to pass arguments at all. Again thanks a lot Toby \$\endgroup\$
    – Riv
    Jun 9 at 20:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.