Please check a script I wrote to e-mail files from my server easier

Mark1986 · 06-13-2012, 06:41 AM

Hi everyone,

Below is a script I created to e-mail files from my server to me. I created this as there is no way to use ftp and to save time since I need scripts from that server local.

Could you take a look and see if there are any best practices I should use or any major flaws in this script? This is my first script I am creating to use by peers.

Code:

#!/bin/bash

# Set paramaters and variables
EMAIL=<my_email>
HOST=`hostname`
I_ARGS=$# #Variable that show amount of arguments given
V_FLAG= #Variable that marks if the -v option was used
F_FLAG= #Variable that marks if the -f option was used
PREV_FLAG="" #Variable that contains the last seen flag
file= #Variable that contains the file used with the -f flag if given
flags=( ) #Array of all flags used
files=( ) #Array of all files to be e-mailed
i=0 #Variable that contains the index of the files array
a=0 #Variable that contains the index of the flags array

# Set paths and filenames

BASEDIR=/home/<my_user>/apps/mailen
TMPFILE=$BASEDIR/tmp_mailen.tmp

# Functions

# Function to print usage
function PRINT_USE(){
  echo "Usage: mailen [-h] [-v] [-f file] [file ...]"
}

# Function to keep track of flags used in arguments given
function ADD_FLAG(){
  flags[a]=$1
  let a=a+1
}

# Function to keep track of files to send in e-mail
function ADD_FILE(){
  files[i]=$1
  let i=i+1
}

# Function to determine the filenames and add them into a temporary file to send in e-mail
function ADD_MAIL_FILE(){
  BASEFILE=`basename $1`
  uuencode $1 $BASEFILE >> $TMPFILE
}

# Get arguments, if there are no arguments then print the usage and exit with an error code
if [[ ${I_ARGS} == 0 ]]
then
  PRINT_USE
  exit 1
fi

# Check if temporary file already exists
if [[ -e ${TMPFILE} ]]
then
  echo "Temporary file already exists, overwrite? (y/n)"
  read DELETE
  if [[ ${DELETE} == 'y' ]]
  then
    rm ${TMPFILE}
  else
    echo "Could not use temporary file: ${TMPFILE}"
    exit 1
  fi
fi

# Loop through all arguments given
while [[ $# -gt 0 ]]
do
  opt=${1}
  case $opt in
    -h )
      echo "This is the help function for mailen.sh."
      echo ""
      echo "FAQ:"
      echo "1. Why are my files not found that are listed in my input file?"
      echo ""
      echo "If you use an input file that contains files that are not in the current working directory, then you need to add the full path of those files in the input file."
      exit 0
      ;;
# If there was an argument -v, mark the flag
    -v ) 
      V_FLAG=1
      PREV_FLAG="v"
      ;;  
# If there was an argument -f, mark the flag
    -f ) 
      F_FLAG=1
      PREV_FLAG="f"
      ;;  
# If the argument was not a -v or -f act as below
    * )
# The argument -f should be the last argument given, if not then exit with the usage details
      if [[ $PREV_FLAG = "f" ]] && [[ $i > 0 ]]
      then
        PRINT_USE
        exit 1
      fi
# If the previous argument was -f, then this argument is the file first mentioned
      if [[ $PREV_FLAG = "f" ]]
      then
        file=$opt
      else
# If none of the above apply, then add argument into the files array
        ADD_FILE $opt
      fi
# Unmark the flag for previous argument
      PREV_FLAG=
      ;;
    esac
# If the previous flag was not empty, then add the flag to the flags array
  if [[ ! $PREV_FLAG = "" ]]
  then
      ADD_FLAG $PREV_FLAG
  fi
  shift
done

# Check if there was a file given with a list of files to add in e-mail
if [[ ! $file = '' ]]
then
# Check if file given already exists
  if [[ ! -e $file ]]
  then
    echo "File does not exist: $file."
    exit 1
  fi
# Add all files in the 'file' file to the files array
  for tempfile in `cat $file`
  do
    ADD_FILE $tempfile
  done
fi

# Check if there are any files to send
if [[ ${files[*]} = '' ]]
then
  PRINT_USE
  exit 1
fi

# Check if all files exist and truely are files
for FILE in ${files[*]}
do
if [[ ! -e ${FILE} ]]
then
  if [[ ! -e `pwd`/${FILE} ]]
  then
    echo "File does not exist: ${FILE}"
    exit 1
  else
    if [[ -f `pwd`/${FILE} ]]
    then
      FILE_TO_ADD="`pwd`/${FILE}"
# File exists in the current directory so add it to the TMPFILE
      ADD_MAIL_FILE FILE_TO_ADD
    fi
  fi
else
  if [[ ! -f ${FILE} ]]
  then
    echo "This is not a file: ${FILE}"
    exit 1
  fi
# File exists in given path so add it to the TMPFILE
  ADD_MAIL_FILE ${FILE}
fi
done

# Mail all files
echo "Mail sent from UNIX" |  mail -s "Files sent from ${HOST}" ${EMAIL} < $TMPFILE

# Check if sending mail finished succesfully
if [ $? == 0 ]
then
  echo "E-mail is sent."
# Cleanup after sending e-mail
  rm $TMPFILE
# In case it didn't...
else
 echo "File is not sent due to technical error."
 echo "Tempfile can be found: $TMPFILE."
 echo "You can try to re-send it. Copy line below and paste it in your command line."
 echo "mail -s \"Files send from `hostname`\" $EMAIL < $TMPFILE"
 echo "Afterwards you can remove the tempfile by copying and pasting the line below in your command line."
 echo "rm $TMPFILE"
 echo ""
 echo "Date time: `date`"
fi

exit 0

############################################
#  Author:  <me>
#  Date:    19-Apr-2012
#
#  Version: 1.0 19-Apr-2012
#  -Added features: mail files as per input given (with or without flags)
#  -Added help feature (-h flag)
#  -Added input argument and flag (-f) so a file with a list of files can be read. The listed files will be e-mailed
#  -Added extra flag for use in future (-v) so there can be a verbose mode to run this script
#  -Added checks to check user input and if files are present
#  -Added functions to make the script more maintainable
#
############################################

Some notes:

I am not using the v flag at the moment. That will be used for verbose later on.
I am using a set emailaddress to mail to, since I am currently the only one using this script.

Any pointers or does this look ok?

Thank you in advance for your efforts and time

David the H. · 06-13-2012, 11:38 AM

I'm not going to bother to look deeply at the operational flow right now, but I will go line-by-line and offer advice on the common issues I spot in the code itself.

1)

Code:

EMAIL=<my_email>
HOST=`hostname`

Since environment variables are generally all upper-case, it's good practice to keep your own user variables in lower-case or mixed-case to help differentiate them.

$(..) is highly recommended over `..`

2)

Code:

TMPFILE=$BASEDIR/tmp_mailen.tmp

Make sure you double-quote all variable and command substitutions, unless you want them to be subsequently word-split and filename expanded. (This isn't the best example line, but this was the first place it popped up, and it's so important it needs to be mentioned right away.)

http://mywiki.wooledge.org/Arguments
http://mywiki.wooledge.org/WordSplitting
http://mywiki.wooledge.org/Quotes

3)

Code:

flags[a]=$1
let a=a+1

let is unnecessary here. The index fields of regular arrays operate in an arithmetic environment, so you can increment the counter variable directly.

You can also use the "+=()" pattern to append directly to an array, without having to deal with a counter.

Code:

flags[a++]=$1
#or
flags+=( "$1" )

4)

Code:

BASEFILE=`basename $1`
uuencode $1 $BASEFILE >> $TMPFILE

See #1 and 2 above.

But also, basename is superfluous. parameter substitution can do the job.

Code:

basefile=${1##*/}
uuencode "$1" "$basefile" >> "$tmpfile"

5)

Code:

if [[ ${I_ARGS} == 0 ]]
then
  PRINT_USE
  exit 1
fi

The full bracketed form of the variable just adds unnecessary clutter here. I recommend leaving them off.

When doing integer comparisons, it's better to use ((..)) arithmetic evaluation brackets.

Error messages should generally be output to stderr instead of stdout.

Many scripters feel that it's more readable to place the "do/then" keywords on the same line as the "for/while/until/if" keywords, as it more clearly separates the outside block from the inside block.

Code:

if (( i_args == 0 )); then
	print_use >&2
	exit 1
fi

6)

Code:

echo "Temporary file already exists, overwrite? (y/n)"
read DELETE

if [[ ${DELETE} == 'y' ]]
then
  rm ${TMPFILE}
else
  echo "Could not use temporary file: ${TMPFILE}"
  exit 1
fi

To be more robust, you should test for multiple input variations. A case statement is often more useful for doing that. Also, you can embed the whole thing in a loop to force re-reads on improper input.

Code:

while true; do
	read -p "Temporary file already exists, overwrite? (y/n): " reply 

	case $reply in

		[yY]*)	rm "$tmpfile"
			echo "Previous Temporary file removed."
			break		;;
		[nN]*)	echo "Could not use temporary file: $tmpfile"
			exit 1		;;
		*)	echo "Unknown option.  Please enter yes or no."	;;
	esac

done >&2

7)

Code:

# Loop through all arguments given

Consider using getopts for parsing options, instead of doing all the work yourself.

http://wiki.bash-hackers.org/howto/getopts_tutorial

8)

Code:

for FILE in ${files[*]}

The "*" index outputs the entire array as a single string, which since you didn't quote it, is then subsequently re-word-split. If any of the array values contain spaces, they will not be read properly.

Instead, use the "@" index, which outputs the array as individual strings, and when quoted, acts as if each entry is quoted separately.

Code:

for file in "${files[@]}"; do

9)

Code:

if [[ ! -e ${FILE} ]]
then
  if [[ ! -e `pwd`/${FILE} ]]

Instead of using "-e" (file exists), use "-r" (file exists and is readable).

Don't use the pwd command, use the built-in $PWD environment variable (there's also $HOME, $USER, and a few others. See the bash manpage for a full list).

10)

Code:

echo "Mail sent from UNIX" |  mail -s "Files sent from ${HOST}" ${EMAIL} < $TMPFILE

This doesn't look right to me. You have mail receiving input from both a pipe and a file redirection. I don't think it can work that way.

And again, quote your variables.

11)

Code:

# Cleanup after sending e-mail
  rm $TMPFILE

No problem here, really. But it would be wise to set up a trap instead to handle the cleanup. That way it won't leave tempfiles around if the script aborts suddenly for some reason or is ctrl+c cancelled.

(Sorry no link offhand. Search the web.)

See these links for more general advice on common scripting errors:
http://mywiki.wooledge.org/BashFAQ
http://mywiki.wooledge.org/BashPitfalls
http://wiki.bash-hackers.org/scripting/newbie_traps

Overall, I think you've made a good start at shell scripting. Keep up the good work!

Mark1986 · 06-14-2012, 04:08 AM

Thank you very much for your input. At this moment I have only read through your comments, but rest assured that I will use your advice to create version 0.2.

You made some excellent remarks!

Only 1 thing at the moment, getopts was not completely doing what I wanted. That is the reason I made this all by hand (see also: http://www.linuxquestions.org/questi...in-aix-937869/ ).