4
\$\begingroup\$

The following code copy a complete folder (that has 3 folders inside), then check if errors are generated and send email if there's errors, the it proceeds to create 3 different tar files and finally move this tar files to a mounted network drive, every name has been changed or somehow altered in order to comply with my company security policy.

#! /usr/bin/env bash

readonly SUBJECT="BACKUP"
readonly TO="[email protected]"
readonly MESSAGE="~/backupMessageError.txt"
readonly TOALL="[email protected], [email protected]"
readonly MESSAGEALL="~/backupMessageSuccess.txt"

backup () {
    if grep -qs '/mount/dir/' /proc/mounts; then
        rsync --exclude /a/folder/ -ravz /backup/source /backup/destination/ 2> ~/backupMessageError.txt
        if checker $1; then
            mail
        else
            compression
        fi
    else
        mount -o hard,nolock IP:/volume1/folder /mount/dir/ 2> ~/backupMessageError.txt
        if checker $1; then
            mail
        else
            rsync --exclude /a/folder/ -ravz /backup/source/ /backup/destination/ 2>> ~/backupMessageError.txt
            if checker $1; then
                mail
            else
                compression
            fi
        fi
    fi
}

compression (){
    tar -I pigz -cf backupFolder1.tar.gz folder1 2>> ~/backupMessageError.txt
    tar -I pigz -cf backupFolder2.tar.gz folder2 2>> ~/backupMessageError.txt
    tar -I pigz -cf backupFolder3.tar.gz folder3 2>> ~/backupMessageError.txt
    if checker $1; then
        mail
    else
        storage
    fi
}

storage (){
    mv /a/folder/backupFolder1.tar.gz /mount/dir/ 2>> ~/backupMessageError.txt
    mv /a/folder/backupFolder2.tar.gz /mount/dir/ 2>> ~/backupMessageError.txt
    mv /a/folder/backupFolder3.tar.gz /mount/dir/ 2>> ~/backupMessageError.txt
    if checker $1; then
        mail
    fi
}

checker (){
    if [ "$(wc -l < ~/backupMessageError.txt)" -ge 1 ];then
       return 0;
    else
       return 1;
    fi
}

mail () {
    if checker $1;then
        mail -s "$SUBJECT" "$TO" < $MESSAGE
    else
        mail -s "$SUBJECT" "$TOALL" < $MESSAGEALL
    fi
}
backup
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to CR. Are the folder names completely different in the uncensored version or is it like folder1, folder2, .. (A repeatable pattern)? \$\endgroup\$
    – JaDogg
    Commented Mar 26, 2019 at 20:24
  • 2
    \$\begingroup\$ Folders have completely different names. Thank you for welcoming me :) \$\endgroup\$
    – Mateo Guty
    Commented Mar 26, 2019 at 20:29

1 Answer 1

2
\$\begingroup\$

What is $1 ?

All the functions except checker include a call checker $1. But none of those functions are called with a parameter, so $1 is never actually defined. The only function that is called with a parameter is checker, but it doesn't actually use a parameter.

As such, you could remove all the $1 without changing the behavior of the program.

More importantly, when you want to use $1 for something, it's good to assign it to a variable with a descriptive name. That way the reader can understand the purpose. In the current program it's hard to tell if $1 is simple negligence and oversight, or a bug waiting to explode. If it had a descriptive name, I could make a more educated guess.

Always quote variables used in command parameters

Instead of checker $1 always write checker "$1".

Checking if a file is empty

checker checks if a file is empty by counting lines. A simpler way exists using the [ -s ... ] builtin:

checker() {
    [ -s ~/backupMessageError.txt ]
}

Notice that there's no need to write if [ -s ... ]; then return 0; else return 1; fi, since the exit code of a function is the exit code of the last command, so we can simply use the command without the if-else.

Use better names

The current function names don't help understand what the program is doing. In fact they are all nouns, when the natural choice would be verbs, or questions. For example checker checks if there were any errors. A more natural naming would be seenAnyErrors. Notice how the code could read like prose:

if seenAnyErrors; then
    sendErrorReport
else
    createBackups
fi

Improve error handling

The current error handling is not so good. Let's take a closer look at for example:

tar -I pigz -cf backupFolder1.tar.gz folder1 2>> ~/backupMessageError.txt
tar -I pigz -cf backupFolder2.tar.gz folder2 2>> ~/backupMessageError.txt
tar -I pigz -cf backupFolder3.tar.gz folder3 2>> ~/backupMessageError.txt

if seenAnyErrors; then ...; fi

What if the first tar command fails? Is your intention to continue with the others anyway?

The current way of checking for errors expects that a failing command writes something to stderr. That's not necessarily the case always, therefore it would be fragile to rely on that. A more reliable way is using the exit code.

A better way to write the above, relying on exit code, would be:

failures=0

tar -I pigz -cf backupFolder1.tar.gz folder1 2>> "$errors"
((failures += $?))

tar -I pigz -cf backupFolder2.tar.gz folder2 2>> "$errors"
((failures += $?))

tar -I pigz -cf backupFolder3.tar.gz folder3 2>> "$errors"
((failures += $?))

if [ "$failures" != 0 ]; then ...; fi

Don't double-quote ~

I believe this is an error:

readonly MESSAGE="~/backupMessageError.txt"

When ~ is double-quoted, the shell won't expand it to $HOME. As it is, I think the command mail -s "$SUBJECT" "$TO" < $MESSAGE will fail with "No such file or directory" error. That variable definition should have been written as:

readonly MESSAGE=~/backupMessageError.txt
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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