Skip to main content
added 156 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

That is, instead of this:

device_is_mounted=`grep ${DEVICE} /etc/mtab`

Write like this:

device_is_mounted=$(grep ${DEVICE} /etc/mtab)

That is, instead of this:

device_is_mounted=`grep ${DEVICE} /etc/mtab`

Write like this:

device_is_mounted=$(grep ${DEVICE} /etc/mtab)
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

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.