Shell script that verifies checksums when moving between filesystems

Question

I coded this because it seemed like a fun project, would be awesome if someone reviewed this.

Thanks!

#! /bin/sh
#function that checks if dependencies are installed
check_software() {
 #check if md5sum is installed
 if [ $(command -v md5sum) > /dev/null 2>&1 ]; then
 echo 'md5sum'
 #check if md5 is installed
 elif [ $(command -v md5) > /dev/null 2>&1 ]; then
 echo 'md5'
 #if neither are installed, quit
 else 
 echo "Neither md5sum nor md5 are installed. Quitting." 1>&2
 exit 1
 fi
}
#function to check if we're moving to same filesystem type
check_filesystem() {
 #df gives us detailed information about the file, use awk to get only the disk
 DISK1=$(df "$SOURCE" | tail -1 | head -1 | awk '{print 1ドル}')
 #run dirname to remove the last bit of the path as the file does not exist yet
 DISK2=$(df $(dirname "$DESTINATION") | tail -1 | head -1 | awk '{print 1ドル}')
 if [ "$DISK1" == "$DISK2" ]; then
 echo 1
 else
 echo 0
 fi
}
main() {
 CHECKSOFT=$(check_software)
 #if file is being moved on the same filesystem
 if [ "$(check_filesystem)" == "1" ]; then
 #move normally
 if [ "$FLAGS" != '' ]; then
 mv "-""$FLAGS" "$SOURCE" "$DESTINATION"
 else
 mv "$SOURCE" "$DESTINATION"
 fi
 else
 echo "Starting bettermv"
 #copy the file
 cp -p "$SOURCE" "$DESTINATION"
 #this bit of code extracts only the hashes from the output produced
 if [ "$CHECKSOFT" = "md5sum" ]; then
 CHECKSUMSOURCE="$($CHECKSOFT "$SOURCE" | awk '{print 1ドル}')"
 CHECKSUMDEST="$($CHECKSOFT "$DESTINATION" | awk '{print 1ドル}')"
 else
 CHECKSUMSOURCE="$($CHECKSOFT "$SOURCE" | awk '{print 4ドル}')"
 CHECKSUMDEST="$($CHECKSOFT "$DEST" | awk '{print 4ドル}')"
 fi
 #compare checksums and if they match up
 if [ "$CHECKSUMSOURCE" == "$CHECKSUMDEST" ]; then
 #remove original and exit
 rm "$SOURCE"
 echo "Move completed successfuly"
 return 0
 #if they don't match up 
 else
 #display error message and quit
 echo "Checksums did not match, please try again" 1>&2
 rm "$DESTINATION"
 exit 1
 fi
 fi
}
FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
 case $opt in 
 f)
 FLAGS="$FLAGS""f"
 ;;
 i)
 FLAGS="$FLAGS""i"
 ;;
 n)
 FLAGS="$FLAGS""n"
 ;;
 v)
 FLAGS="$FLAGS""v"
 ;;
 \?)
 echo "Invalid argument $OPTARG"
 ;;
 esac
done
#shift optindex past the flags
shift $(( OPTIND-1 ))
#iterate through files to move and move them to the destination (last argument supplied)
for i in $@; do
 if [ $i != ${BASH_ARGV[0]} ]; then
 SOURCE=$i
 DESTINATION="${BASH_ARGV[0]}""$(basename $SOURCE)"
 main
 fi
done
exit 0

• Rahul Gopinath: The convention in codereview.se is that the code being reviewed is pasted. We insist on that so that we can always see the history of changes even if an external site goes down.

Answer 1

I'll start with your entry point and will go through you code step by step. The last code block will contain an updated version.

The key elements in my changes are split your functions and only do one specific task, e.g. extra function for getting a checksum of a file and another one comparing two checksums etc. A function should also get all necessary information by PARAMETER not by an environment variable set outside. This simplifies testing a lot.

The getopts part is fine but you should just store the flag with the additional "-" as it will simplify things later in your code. You may also want to exit if an unknown parameter was provided. I would also rename FLAGS to MV_FLAGS as it describes better what the flags are for. You would normally also print a usage if an invalid parameter was provided. After parsing the options you should also check if enough parameters were provided and print a message otherwise but this is just a UX thing.

MV_FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
 case $opt in
 f) MV_FLAGS="$MV_FLAGS"f ;;
 i) MV_FLAGS="$MV_FLAGS"i ;;
 n) MV_FLAGS="$MV_FLAGS"n ;;
 v) MV_FLAGS="$MV_FLAGS"v ;;
 ?)
 echo "Invalid argument 1ドル" >&2
 exit 1
 ;;
 esac
done
[ -n "$MV_FLAGS" ] && MV_FLAGS="-$MV_FLAGS"
#shift optindex past the flags
shift $(( OPTIND-1 ))
if [ $# -lt 1 ] ; then
 echo "You must specify additional filenames" >&2
 exit 1
fi

For the next part you should definitely quote $@ as otherwise you won't handle parameters with spaces correctly. There is also no need to compare $i with ${BASH_ARGV[0]} as $@ contains the parameter starting at 1. Instead of specifying environment variables to call your main function you should specify them as a parameter instead. BASH_ARGV is according to man bash also only set if you have debugging enabled, so it is not portable. I also don't really understand what DESTINATION should be, but you should change it as well. You should also either quote your parameters with " or use ${ I wouldn't mix them and there is no need to use " as well as ${}. You should also check if the source file is a valid file before pushing calling your function. The function name could also be improved, e.g move_file

for i in "$@" ; do
 SOURCE="$i"
 DESTINATION="0ドル""$(basename "$SOURCE")"
 if [ ! -f "$SOURCE" ] ; then
 echo "$SOURCE does not exist, ignoring." >&2
 continue
 fi
 move_file "$SOURCE" "$DESTINATION"
done

In your main and my move_file function you always call check_software but there is no need for it. It is enough to call it once at the beginning. You should also create a function handling verifying the filenames so your code doesn't get clustered with the different program parameters etc:

#function that checks if dependencies are installed
check_software() {
 for name in md5sum md5 openssl ; do
 if [ $(type "$name") > /dev/null 2>&1 ] ; then
 return 0
 fi
 done
 return 1
}
if ! check_software ; then
 echo "Neither md5sum, md5 nor openssl are installed. Quitting." >&2
 exit 1
fi

Your check_filesystem can also be simplified a lot if you use a device number instead of parsing df, I'd also rename it to same_filesystem

same_filesystem() {
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 FS1=$(stat -c "%d" "1ドル")
 FS2=$(stat -c "%d" "$(dirname "2ドル")" )
 [ "$FS1" -eq "$FS2" ]
 return $?
}

You can also simplify your main/move_file function a bit, e.g:

move_file() {
 # we already checked if the necessary programs are available no
 # need to check again
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 SOURCE="1ドル"
 DESTINATION="2ドル"
 if [ -f "$DESTINATION" ] ; then
 # do something if file already exists?!
 :
 fi
 #if file is being moved on the same filesystem
 if same_filesystem "$SOURCE" "$DESTINATION" ; then
 mv $MV_FLAGS -- "$SOURCE" "$DESTINATION"
 return 0
 fi
 echo "Starting bettermv" >&2
 #copy the file
 cp -p "$SOURCE" "$DESTINATION"
 if verify_files "$SOURCE" "$DESTINATION" ; then
 rm "$SOURCE"
 return 0
 else
 echo "Error moving $SOURCE to $DESTINATION, abort." >&2
 rm "$DESTINATION"
 return 1
 fi
}
get_checksum() {
 if [ $# -ne 1 ] ; then
 echo "Usage: 0ドル source" >&2
 return 1
 fi
 if type openssl >/dev/null 2>&1 ; then
 echo $(openssl md5 "1ドル" | awk '{print 2ドル}')
 elif type md5sum >/dev/null 2>&1 ; then
 echo $(md5sum "1ドル" | awk '{print 1ドル}')
 elif type md5 >/dev/null 2>&1 ; then
 echo $(md5 "1ドル" | awk '{print 4ドル}')
 fi
 return 1
}
verify_files() {
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 CHECKSUM1=$(get_checksum "1ドル")
 CHECKSUM2=$(get_checksum "2ドル")
 [ $CHECKSUM1 = $CHECKSUM2 ]
 return $?
}

The complete script with all changes (i tested it):

#!/bin/sh
#function that checks if dependencies are installed
check_software() {
 for name in md5sum md5 openssl ; do
 if type "$name" 1>/dev/null 2>&1 ; then
 return 0
 fi
 done
 return 1
}
if ! check_software ; then
 echo "Neither md5sum, md5 nor openssl are installed. Quitting." >&2
 exit 1
fi
move_file() {
 # we already checked if the necessary programs are available no
 # need to check again
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 SOURCE="1ドル"
 DESTINATION="2ドル"
 if [ -f "$DESTINATION" ] ; then
 # do something if file already exists?!
 :
 fi
 #if file is being moved on the same filesystem
 if same_filesystem "$SOURCE" "$DESTINATION" ; then
 mv $MV_FLAGS -- "$SOURCE" "$DESTINATION"
 return 0
 fi
 echo "Starting bettermv" >&2
 #copy the file
 cp -p "$SOURCE" "$DESTINATION"
 if verify_files "$SOURCE" "$DESTINATION" ; then
 rm "$SOURCE"
 return 0
 else
 echo "Error moving $SOURCE to $DESTINATION, abort." >&2
 rm "$DESTINATION"
 return 1
 fi
}
same_filesystem() {
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 FS1=$(stat -c "%d" "1ドル")
 FS2=$(stat -c "%d" "$(dirname "2ドル")" )
 [ "$FS1" -eq "$FS2" ]
 return $?
}
get_checksum() {
 if [ $# -ne 1 ] ; then
 echo "Usage: 0ドル source" >&2
 return 1
 fi
 if type md5sum >/dev/null 2>&1 ; then
 echo $(md5sum -- "1ドル" | awk '{print 1ドル}')
 elif type md5 >/dev/null 2>&1 ; then
 echo $(md5 "1ドル" | awk '{print 4ドル}')
 elif type openssl >/dev/null 2>&1 ; then
 echo $(openssl md5 "1ドル" | awk '{print 2ドル}')
 else
 return 1
 fi
}
verify_files() {
 if [ $# -ne 2 ] ; then
 echo "Usage: 0ドル source target" >&2
 return 1
 fi
 CHECKSUM1=$(get_checksum "1ドル")
 CHECKSUM2=$(get_checksum "2ドル")
 [ $CHECKSUM1 = $CHECKSUM2 ]
 return $?
}
MV_FLAGS=''
#get all the move flags and store them in a variable
while getopts "finv" opt; do
 case $opt in
 f) MV_FLAGS="$MV_FLAGS"f ;;
 i) MV_FLAGS="$MV_FLAGS"i ;;
 n) MV_FLAGS="$MV_FLAGS"n ;;
 v) MV_FLAGS="$MV_FLAGS"v ;;
 ?)
 echo "Invalid argument 1ドル" >&2
 exit 1
 ;;
 esac
done
[ -n "$MV_FLAGS" ] && MV_FLAGS="-$MV_FLAGS"
#shift optindex past the flags
shift $(( OPTIND-1 ))
if [ $# -lt 1 ] ; then
 echo "You must specify additional filenames" >&2
 exit 1
fi
for i in "$@" ; do
 SOURCE="$i"
 DESTINATION="0ドル""$(basename "$SOURCE")"
 if [ ! -f "$SOURCE" ] ; then
 echo "$SOURCE does not exist, ignoring." >&2
 continue
 fi
 move_file "$SOURCE" "$DESTINATION"
done

• infinite code: Whoa, that is an incredibly detailed response. Thank you for taking the time to type all that up, I've learned a lot.