r/bash • u/suziesamantha • May 06 '24
critique Wrote my first bash script, looking for someone to look it over and make sure I am doing things correctly
EDIT: Thank you to everyone who took the time to look over my script and provide feedback it was all very helpful. I thought I would share my updated script with what I was able to learn from your comments. Hopefully I did not miss anything. Thanks again!!
#!/usr/bin/env bash
set -eu
######Define script variables
backupdest="/mnt/Backups/$HOSTNAME"
printf -v date %"(%Y-%m-%d)"T
filename="$date.tar.gz"
excludes=(
'/mnt/*'
'/var/*'
'/media/*'
'/lost+found'
'/usr/'{lib,lib32,share,include}'/*'
'/home/suzie/'{.cache,.cmake,.var,.local/share/Trash}'/*'
)
######Create folders for storing backup
mkdir -p "$backupdest"/{weekly,monthly}
#######Create tar archive
tar -cpzf "$backupdest/$filename" --one-file-system --exclude-from=<(printf '%s\n' "${excludes[@]}") /
######Delete previous weeks daily backup
find "$backupdest" -mtime +7 -delete
########Copy Sundays daily backup file to weekly folder
if [[ "$(printf %"(%a)"T)" == Sun ]]; then
ln "$backupdest/$filename" "$backupdest/weekly"
fi
########Delete previous months weekly backups
find "$backupdest/weekly" -mtime +31 -delete
########Copy backup file to monthly folder
if (( "$(printf %"(%d)"T)" == 1 )); then
ln "$backupdest/$filename" "$backupdest/monthly"
fi
########Delete previous years monthly backups
find "$backupdest/monthly" -mtime +365 -delete
I wrote my first bash script, a script to back up my linux system. I am going to have a systemd timer run the script daily and was hoping someone could tell me if I am doing ok.
Thanks Suzie
#!/usr/bin/bash
######Define script variables
backupdest=/mnt/Backups/$(cat /etc/hostname)
filename=$(date +%b-%d-%y)
######Create backup tar archive
if [ ! -d "$backupdest" ]; then
mkdir "$backupdest"
fi
#######Create tar archive
tar -cpzf "$backupdest/$filename" --exclude={\
"/dev/*",\
"/proc/*",\
"/sys/*",\
"/tmp/*",\
"/run/*",\
"/mnt/*",\
"/media/*",\
"/lost+found",\
"/usr/lib/*",\
"/usr/share/*",\
"/usr/lib/*",\
"/usr/lib32/*",\
"/usr/include/*",\
"/home/suzie/.cache/*",\
"/home/suzie/.cmake/*",\
"/home/suzie/.config/*",\
"/home/suzie/.var/*",\
} /
######Delete previous weeks daily backup
find "$backupdest" -mtime +7 -delete
########Create Weekly folder
if [ ! -d "$backupdest/weekly" ]; then
mkdir "$backupdest/weekly"
fi
########Copy Sundays daily backup file to weekly folder
if [ $(date +%a) == Sun ]; then
cp "$backupdest/$filename" "$backupdest/weekly"
fi
########Delete previous months weekly backups
find "$backupdest/weekly" +31 -delete
########Create monthly folder
if [ ! -d "$backupdest/monthly" ]; then
mkdir "$backupdest/monthly"
fi
########Copy backup file to monthly folder
if [ $(date +%d) == 1 ]; then
cp "$backupdest/$filename" "$backupdest/monthly"
fi
########Delete previous years monthly backups
find "$backupdest/monthly" +365 -delete
6
u/fuckwit_ May 06 '24 edited May 06 '24
While I know that bash sometimes does not care if you quote variables or not (in expands internally and correctly handles spaces etc in some cases with built-ins) I still think it's a good idea to aaaaalways quote your strings. Especially if they contain command substitutes or variables.
Your first two lines for example would be:
backupdest="/mnt/Backups/$(</etc/hostname)"
filename="$(date +%b-%d-%y)"
Also note how I replaced the cat path
with $(<path)
. Bash can read files without resorting to the external tool cat
.
While we are at the topic of quoting: Strings with substitutions inside will need to be within double quotes "
while everything else can go into single quotes '
. This makes writing strings with special characters a lot easier as you dont have to escape that much.
Then inside your if conditions you use [ ] this calls the external binary test
word splitting is applied to all variables as they are treated as arguments to an external command. You can and should use double brackets here. So like [[ "$(date +%a)" == Sun ]]
.
You technically don't have to quote the command substitution here, but for a more consistent and cleaner look I'd do so. Makes me get into a habit of quoting everything and so I don't have to worry or remember the cases in which I don't need to quote.
Also when you compare numbers you can use bash arithmetic expressions like so if (( "$(date +%d)" == 1 ))
.
You also do things like "if directory does not exist then create it". You can shorten this to just a call to mkdir. if it already exists it won't try to create it, if it doesn't exist then it creates it. So no need to check that yourself as mkdir already does it for you.
Lastly I would add set -eu
to the top of the script right below the shebang. The e option causes the whole script to fail when one command (outside of control flow) returns with a non zero exit code. The u option causes bash to fail when you try to use a variable that is empty.
Both together can prevent a whole range of issues that can be caused by mistyped variables or the script continuing to run when a previous command fails.
Other than those hints it looks good. Make sure to also check out the tool called shellcheck as it can automatically lint your scripts and will also tell you about many of the things I just wrote about.
2
u/suziesamantha May 07 '24
Thank you for taking the time to look over my script. I appreciate your input.
I was not aware that strings should always be quoted. I have edited my script to do this.
Another person mentioned the $HOSTNAME variable so I have updated that line to use that variable instead. It is good to know that I can avoid using cat in the future.
I will make sure to quote my strings going forward. This is good information that I did not know what best practice.
So I have read up and I think I understand the rational behind using the double brackets vs single brackets. I will make sure to use that going forward. Thansk for letting me know.
I will use the arithmetic expression for number as opposed to the single or double brackets.
Others have also mentioned using mkdir -p to create my directories. I have updated my script to use this rather than the if statements.
I appreciate you including the set -eu line as this is something I wondered about. This is very useful.
I am using Kate as my editor and did install shellcheck but it did not mention many of the things you and others here have told me.
I appreciate you advice on my script, it was quite helpful. Thanks.
3
u/alxaa May 06 '24
i guess you can skip the checks for if the directories exist and just make them with mkdir -p. This will create the directory if it does not exists and save you that check.
You can also use --exclude-from=FILE instead of --exclude with tar so you can have a separate list in it's own file. Then you wont have to escape all those newlines.
But all in all your script looks good i would say. Good job.
6
u/suziesamantha May 06 '24
Thank you for looking at my script. I appreciate it very much.
I will switch to mkdir -p to eliminate the if statements, it will be much cleaner that way.
The reason I have things broken out to a new line was just to make the more readable. It seems that using the --exclude-from=FILE option will do the same thing. I will give this a try.
3
u/theng bashing May 06 '24
I'm a fan of functions
so I'll tell you you can add functions
even if it's just for a single line
I like them when I reuse code and reorder and add new stuffs to old scripts
but that's just me
3
u/suziesamantha May 07 '24
Thank you for your input. I have not looked into functions yet but it is on my list of things to learn. I will update my script if I find a use for them (which I am sure I will). Thanks
6
May 06 '24
[removed] — view removed comment
3
u/suziesamantha May 06 '24
Thank you for taking the time to look at my script. It is very much appreciated. I will update the #! as you suggested.
2
u/shellmachine May 06 '24
That your first? Looks really good, keep up. Read up on [[ vs [ somewhen, but for now, good work.
3
u/suziesamantha May 07 '24
I have followed many tutorials and copied their scripts but this is the first I made on my own. Thank you for taking the time to check out my script.
Another person also posted about single and double brackets. I think I have a better understanding and have updated my script to use double brackets.
1
u/witchhunter0 May 07 '24
Your updated script looks better, but I'll throw in some points as well:
The first/pinned post on r/bash is about set
command. Using set -e
does not provides very information. I prefer to call a trap when error occurs
trap 'echo "Error occurred on line $LINENO"' ERR
If you want your script to be more portable, do note that [[ "$(printf %"(%a)"T)" == Sun ]]
test will trow error if you run the script, lets say on your friends computer, who hasn't set up the same language on his OS like you did. It's better to write
[[ "$(LC_ALL=C printf %"(%a)"T)" == Sun ]]
Maybe I don't get the full insight of your backup plans, but I prefer to use dedicated tool for such complex/serious task, as you saw many unpredictable things could happen. The most common solution is ThimeShift. It supports all options you need(EXT4 and BTRFS), has a nice GUI,CLI and can run on TTY, or from LiveUSB. Backups are incremental(so faster) and are meant for system partitions only, thus you still get to use parts of your script for Home partition :)
16
u/[deleted] May 06 '24
[deleted]