3
\$\begingroup\$

I wrote a bash script for backing up my LAMP server. To be honest that's my first experience in bash so I want to ask you for help in its improvement.

#!/bin/bash

date=`(date +%Y%m%d%H%M)`

declare -A users=(
    ["user1"]="password1"
    ["user2"]="password2"
)

for user in ${!users[@]}
do
    directory=$user

    directories=`find /var/www/$directory -maxdepth 1 -mindepth 1 -type d`

    for directory in $directories
    do
        cd $directory

        if [ ! -d backups ]
        then
            mkdir backups
        fi

        tar --exclude ./backups -czf backups/$date.tar.gz .

        password=${users[$user]}

        databases=`mysql -u $user -p$password -Bse "SHOW DATABASES;"`

        for database in $databases
        do
            if [ $database = ${directory##*/} ]
            then
                mysqldump -u $user -p$password $database | gzip > backups/$date.sql.gz
            fi
        done

        chmod 770 backups -R
        chown $user:$user backups -R
    done
done
\$\endgroup\$
2
\$\begingroup\$

Use $(...) instead of `...`

`...` is archaic, error-prone, and harder to read than $(...).

For example, instead of date=`(date +%Y%m%d%H%M)`, write date=$(date +%Y%m%d%H%M). Note that the extra (...) around the date command was unnecessary.

Double-quote variables used in command arguments

For example instead of for user in ${!users[@]}, write for user in "${!users[@]}".

This way, if the users array has keys with spaces in it, the program will still work correctly. In the current example none of the keys contain spaces (they are user1 and user2), so you don't strictly need it, but it's a good habit to build.

Don't parse the output of find

This line shows a very bad practice:

directories=`find /var/www/$directory -maxdepth 1 -mindepth 1 -type d`

Let's first correct the form:

directories=$(find "/var/www/$directory" -maxdepth 1 -mindepth 1 -type d)

The problem here is that if any of the files printed by the command contain a newline character, then it will be impossible to tell complete filenames from partial filenames in $directories. If any of the filenames contain whitespace, then parsing the value of $directories will be complicated.

It's best to avoid parsing the output of find when possible. Even if this code works in your example because you know that the filenames will be strictly alphanumeric, it's good to avoid such bad practices if possible.

In this case there's a fairly simple alternative:

for directory in "/var/www/$directory"/*/

This is basically a simple glob expansion, with the small trick that the trailing / will make it match only directories.

Notice that */ is outside the double-quoted part. This is because * would not be expanded within double-quotes. For the record, this is an equivalent way to write the above:

for directory in /var/www/"$directory"/*/

Finally, there is a small caveat: this is not exactly equivalent to the find command, because that would also find directories whose name starts with ., and the glob expansion won't, by default. If you need to support such filenames, then write shopt -s dotglob before the loop statement. That will enable matching filenames starting with . in glob expansions.

Avoid changing the working directory (with cd) in scripts

Changing directories with cd in scripts is error-prone:

  • It may become confusing what is the current working directory, at any point in the script

  • The cd command may fail, for example due to permission issues or filesystem errors. If such failures are not handled (as in the posted script), the program may happily continue executing the rest of the commands, which can have serious consequences. Consider for example what will happen to cd "$foo"; rm -fr * if the cd command fails: important stuff could get deleted!

In the posted code, instead of this:

cd $directory

if [ ! -d backups ]
then
    mkdir backups
fi

tar --exclude ./backups -czf backups/$date.tar.gz .

You could write without using cd like this:

mkdir -p "$directory/backups"
tar -C "$directory" --exclude ./backups -czf "backups/$date.tar.gz" .

That is, use the -C flag of tar to specify its effective directory, and use the correct paths in other commands appropriately. Keep in mind that the rest of the commands in the loop body that rely on the path of backups will also need to be adjusted accordingly. For example change gzip > backups/$date.sql.gz to gzip > "$directory/backups/$date.sql.gz".

Notice that I double-quoted the parameters using the $date variable, as explained earlier in this posted. And I replaced a conditional statement by taking advantage of the -p flag of mkdir.

Don't give permissions willy-nilly

Using chmod with unnecessarily relaxed permissions is a very bad practice. Question every permission bit if it's really needed and why.

Let's take a closer look at this:

chmod 770 backups -R
chown $user:$user backups -R

The chmod gives read-write-execute permission to the user and the group of files and directories under backups, recursively. Why? The loop body creates .gz files under backups. These files don't need to be executable. Only the backups directory needs to be executable, so that the user can access its contents.

It would be better to set an appropriate umask before the loop. You will be able to drop the chmod command, because the umask, combined with the chown will be enough to ensure correct permissions.

Strictly speaking, the chown is only necessary for the newly created files and directories, instead of a blanket chown -R on the parent directory. But this level of laziness is probably acceptable in a Bash script, so until the chown -R becomes a practical concern (for example due to an excessively large number of files in the directory tree), I wouldn't complicate the script with more precise treatment.

Test your backup solutions

Can you spot the bug in this code?

for database in $databases
do
    if [ $database = ${directory##*/} ]
    then
        mysqldump -u $user -p$password $database | gzip > backups/$date.sql.gz
    fi
done

The $database variable is not used in the output filename. If there are multiple databases, only the last one will be saved. A few months later, if you try to fix a corrupted database by recovering from backups, you would be in for a terrible surprise, discovering that some database backups are completely missing.

Be sure to test thoroughly your backup solution, accounting for the most complex supported scenarios.

(And as pointed out multiple times earlier, double-quote variables used in command arguments.)

\$\endgroup\$

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.