This is really nice Bash code, you're off to a really great start!
Repeated >> $LOG_FILE 2>&1
The first thing that catches the eye is the repeated redirections.
Not only this is not fun to write,
a lot of noise to read,
and when you add something to the script later,
you might forget to consistently add the redirection.
One easy solution is to wrap the entire code in a { ... }
block, for example:
{
dt=$(date '+%d/%m/%Y %H:%M:%S')
echo "--- USB AutoLoader --- $dt"
# test that this device isn't already mounted
device_is_mounted=`grep ${DEVICE} /etc/mtab`
if [ -n "$device_is_mounted" ]; then
echo "Error: seems /dev/${DEVICE} is already mounted"
exit 1
fi
# ... and so on ...
} >> $LOG_FILE 2>&1
Another, a bit cleaner solution is to wrap the code in a proper function, for example:
automount() {
dt=$(date '+%d/%m/%Y %H:%M:%S')
echo "--- USB AutoLoader --- $dt"
# test that this device isn't already mounted
device_is_mounted=`grep ${DEVICE} /etc/mtab`
if [ -n "$device_is_mounted" ]; then
echo "Error: seems /dev/${DEVICE} is already mounted"
exit 1
fi
# ... and so on ...
}
automount >> $LOG_FILE 2>&1
Guard statements
The beginning of usb-automount.sh
uses guard statements consistently, for example:
if [ -n "$device_is_mounted" ]; then
echo "Error: seems /dev/${DEVICE} is already mounted" >> $LOG_FILE 2>&1
exit 1
fi
# ... normal execution continues here ...
But then you break that pattern here:
# test mountpoint - it shouldn't exist
if [ ! -e "$MOUNT_DIR/${DEVICE}" ]; then
# ...
# all done here, return successful
exit 0
fi
exit 1
The earlier pattern is better.
It's consistent with the code above it,
and it reduces the indentation level,
resulting in flatter code that's often easier to read.
That is, like this:
# test mountpoint - it shouldn't exist
if [ -e "$MOUNT_DIR/${DEVICE}" ]; then
exit 1
fi
# ... normal execution continues here ...
In this case you don't even need the exit 0
at the end of the script,
as the exit code of the script will be the exit code of the last statement.
Use $(...)
instead of `...`
$(...)
is the modern, recommended syntax,
use it everywhere consistently.
Use the exit code of commands directly
A really cool thing about Bash conditionals is that they can use the exit code of a command directly. For example instead of this:
device_is_mounted=`grep ${DEVICE} /etc/mtab`
if [ -n "$device_is_mounted" ]; then
echo "Error: seems /dev/${DEVICE} is already mounted" >> $LOG_FILE 2>&1
exit 1
fi
You can write like this:
if grep -q ${DEVICE} /etc/mtab; then
echo "Error: seems /dev/${DEVICE} is already mounted" >> $LOG_FILE 2>&1
exit 1
fi
This works, because grep
exits with success if it matched something.
The -q
flag is to make it quiet, otherwise the matched line would be part of the output, which you don't need and would be just noise.
Use more functions
The logic of checking if the device is mounted is repeated twice,
and it's not exactly trivial to write.
In such situations it's always good to introduce a helper function, for example:
is_mounted() {
grep -q "$1" /etc/mtab
}
Notice how the function doesn't return anything explicitly:
the exit code of the last command, in this case the grep
,
becomes the exit code of the function,
and you can use the function in an if
statement just like the grep
itself earlier:
if is_mounted ${DEVICE}; then
echo "Error: seems /dev/${DEVICE} is already mounted" >> $LOG_FILE 2>&1
exit 1
fi
Indentation
It's a minor thing, but about halfway through usb-automount.sh
the indentation is slightly off. It's good to keep it consistent.