Log Retrieval Script

Question

I wrote a simple script to retrieve the logs from my website, and place them in a subdirectory called logs. It fetches the logs using sftp. I would like help with the error handling. I'm sure that there are errors I did not see, and I would like to make it so that it fails gracefully.

#!/bin/bash
# gets the logs


# Location to place downloaded logs
PlaceLogs=./logs
# ssh connection to sftp files from
SSHLocation=hjsblog
# Location and pattern of logs
GetLogs=../logs/access*


#######################################
#######################################

CWD=$(pwd)

# move to get location
if [ "$(ls -A $PlaceLogs)" ] 
then
    echo $PlaceLogs is not empty
    return
fi
mkdir -p $PlaceLogs
cd $PlaceLogs

# perform the get operation
sftp $SSHLocation << EOF
cd $(dirname $GetLogs)
get $(basename $GetLogs)
EOF

# uncompress files
bunzip2 *.bz2

# return to script dir
cd $CWD

Answer 1

handle more possibilities

Anything touching the file system can fail.

• mkdir You could have permission problems or something else. Do mkdir foo || exit 3. Feel free to replace the exit with your own function for error handling.
• cd is the same deal as mkdir
• check for the existance of your directory before lsing it: if [[ -d $PlaceLogs ]]

Also, do any bz2 exist after the sftp? You should check before bunzip'ing them.

You can safely ignore all of these things too, but since you specifically asked about error handling it seemed good to point them out. If this is a critical piece of software then I would build a way for these failures to raise alerts with your DevOps team.

shellcheck.net

Run all of your scripts through http://www.shellcheck.net/ until you get in better habits.

use double square brackets

Double square brackets are better than single square brackets. They have less surprises. The primary one is that if you have an empty variable you can do

if [[ $FOO == "" ]]

where single brackets require a placeholder so none of the arguments disappear if missing:

if [ x$FOO == "x" ]

Without the xs the $FOO would disappear as a shell argument when empty.

Answer 2

Suspicious return

A return statement can only be used in functions or sourced script. I suspect the posted code is neither. So I think you want to use exit instead. If not having logs is considered an error/failure, then you should exit with non-zero, for example exit 1.

Avoid cd within scripts

Whenever possible, it's good to avoid to change the working directory within scripts. It's easy to lose track of where you are, and it's easy to forget to cd back to the previous dir, not to mention additional concerns about error checking of cd.

In this example it seems you don't need to cd at all. You could use sftp with appropriate source and destination parameters to copy files from and to the right places, for example:

sftp $SSHLocation "$remotepath" "$localpath"

This way you don't need to cd at all, no more need for CWD=$(pwd), and overall fewer concerns.

Avoid unnecessary work

It seems that you are using this variable to store two different things:

# Location and pattern of logs
GetLogs=../logs/access*

Later in the script you split this to parts with:

sftp $SSHLocation << EOF
cd $(dirname $GetLogs)
get $(basename $GetLogs)
EOF

It would have been better to use two variables, one for the directory name and another for the filename, so you don't need to split, and simply use the variables directly.