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
asked Mar 26, 2019 at 20:19
\$\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\$ Commented Mar 26, 2019 at 20:24
  • 2
    \$\begingroup\$ Folders have completely different names. Thank you for welcoming me :) \$\endgroup\$ 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
answered Mar 27, 2019 at 21:08
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.