2
\$\begingroup\$

Very new to Raspberry Pi and bash, but working on a headless server that will run a streaming app from USB audio card.

The plan is to take a USB card with settings file, and when inserted, the usb will automount, check for settings file, load the streamer using config file and wait for the audio input stream to start

All I have to date is generic udev rules and mount/unmount scripts ready to add code to see if the config file exists and start processing. I heavily borrowed ideas, code and udev manual SuperUser Answer for udev as well as the Mount Manager example

Many examples used serial numbers and volume labels - I do NOT want anything hardcoded. The goal was to be able to mount any loaded USB drive on to a script defined /media/ and use same script to unmount it if it was removed.

This has only been tested on Raspberry Pi 3 running Raspbian Jessie Lite. The only prep is to create the log and script folders defined by the config parameters at the top of the init script (and chmod 0755 scripts(

Barebones comments so far, but building documentation as I go along.

Any feedback, warnings and suggestions are much appreciated. I am very new at this and have been relying on others' examples and many man pages

/etc/udev/rules.d/85-usb-loader.rules

# /etc/udev/rules.d/85-usb-loader.rules
# ADD rule: 
#       if  USB inserted, 
#       and flash drive loaded as sd#
#           pass on dev id and device formatting type
#       run short script to fork another processing script
#       run script to initiate another script (first script must finish quickly)
#             to mkdir and mount, process file
#
# REMOVE rule: 
#       if  USB removed, 
#           unmount devices loaded by ADD rule
#           remove mount folder created by ADD rule 
# 
# ** NOTE ** only add initial action parameter to ADD rule
#
# reload rules on PI by: 
#       > sudo udevadm control -R
#
ACTION=="add", KERNEL=="sd*[0-9]", SUBSYSTEMS=="usb", RUN+="/home/pi/scripts/usb-initloader.sh ADD %k $env{ID_FS_TYPE}"
ACTION=="remove", KERNEL=="sd*[0-9]", SUBSYSTEMS=="usb", RUN+="/home/pi/scripts/usb-initloader.sh %k"

/home/pi/scripts/usb-initloader.sh

#!/bin/bash

# /home/pi/scripts/usb-initloader.sh
#
# make sure to chmod 0755 on these scripts
#
# script that runs when usb device is inserted
# checks to see if file is on USB drive and copies it over
# eventually run a python script that loads the config file
#
# ** DEVICE ADDED **
# should be called from a udev rule like:
# passes kernel device and filesystem type
# ACTION=="add", KERNEL=="sd*[0-9]", SUBSYSTEMS=="usb", RUN+="/home/pi/scripts/usb-initloader.sh ADD %k %E{key}"
# Mounts usb device on /media/<dev>
# Logs changes to /var/log/syslog
# use tail /var/log/syslog to look at latest events in log
#
# ** DEVICE REMOVED **
# or remove - where we only need the kernel device
# should be called from a udev rule like:
# ACTION=="remove", KERNEL=="sd*[0-9]", SUBSYSTEMS=="usb", RUN+="/home/pi/scripts/usb-initloader.sh %k"
#
#
# CONFIGURATION
#
LOG_DIR=/home/pi/logs
LOG_FILE="${LOG_DIR}/usb-automount.log"
MOUNT_DIR=/media

# Call new script and leave this one (with trailing "&")

if [ "$1" == "ADD" ]; then
    DEVICE="$2"    # USB device name (formed with kernel passed from rule)
    DEVTYPE="$3"   # USB device formatting type
    /home/pi/scripts/usb-automount.sh "$LOG_FILE" "$MOUNT_DIR" "$DEVICE" "$DEVTYPE" &
else
    DEVICE="$1"    # USB device name (formed with kernel passed from rule)
    /home/pi/scripts/usb-unloader.sh "$LOG_FILE" "$MOUNT_DIR" "$DEVICE" &
fi

/home/pi/scripts/usb-unloader.sh

#!/bin/bash

# /home/pi/scripts/usb-unloader.sh

# Called from /home/pi/scripts/usb-initloader.sh

# make sure to chmod 0755 on file

# UnMounts usb device on /media/<device>
# Logs changes to /var/log/syslog and local log folder
# use tail /var/log/syslog to look at latest events in log

# CONFIGURATION

LOG_FILE="$1"
MOUNT_DIR="$2"
DEVICE="$3"  # USB device name (from kernel parameter passed from rule)

# check input
if [ -z "$LOG_FILE" ]; then
    exit 1
fi
if [ -z "$MOUNT_DIR" ]; then
    exit 1
fi
if [ -z "$DEVICE" ]; then
    exit 1
fi


dt=$(date '+%d/%m/%Y %H:%M:%S')
echo "--- USB Auto UnLoader --- $dt" >> $LOG_FILE

sudo umount "/dev/$DEVICE" >> $LOG_FILE 2>&1
sudo rmdir "$MOUNT_DIR/$DEVICE" >> $LOG_FILE 2>&1

device_is_mounted=`grep ${DEVICE} /etc/mtab`
if [ "$device_is_mounted" == "" ]; then
    echo "/dev/${DEVICE} successfully UnMounted" >> $LOG_FILE 2>&1
fi

/home/pi/scripts/usb-automount.sh

#!/bin/sh
#
# Script: /home/pi/scripts/usb-automount.sh
# make sure to chmod 0755 on this script

# In case process of mounting takes too long for udev
# we call this script from /home/pi/scripts/usb-initloader.sh

#
# USAGE: usb-automount.sh DEVICE FILESYSTEM
#   DEVICE      is the actual device node at /dev/DEVICE (returned by udev rules %k parameter)
#   FILESYSTEM  is the FileSystem type returned by rules (returned by udev rules %E{ID_FS_TYPE} or $env{ID_FS_TYPE}
#
# This script takes a device name, file system type, creates /media/DEVICE and mounts the partition.  
#
# Adapted for Raspberry Pi - Raspbian O/S
# from previous code found at:
# https://superuser.com/questions/53978/automatically-mount-external-drives-to-media-label-on-boot-without-a-user-logge
# and mount manager example at
# http://solvedforhome.com/?p=2806&v=3a52f3c22ed6

LOG_FILE="$1"
MOUNT_DIR="$2"
DEVICE="$3"  # USB device name (from kernel parameter passed from rule)
FILESYSTEM="$4"

# check input
if [ -z "$LOG_FILE" ]; then
    exit 1
fi
if [ -z "$MOUNT_DIR" ]; then
    exit 1
fi
if [ -z "$DEVICE" ]; then
    exit 1
fi
if [ -z "$FILESYSTEM" ]; then
    exit 1
fi

# Allow time for device to be added
sleep 1

dt=$(date '+%d/%m/%Y %H:%M:%S')
echo "--- USB AutoLoader --- $dt" >> $LOG_FILE

# 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" >> $LOG_FILE 2>&1
    exit 1
fi

# test mountpoint - it shouldn't exist
if [ ! -e "$MOUNT_DIR/${DEVICE}" ]; then

    # make the mountpoint
    sudo mkdir "$MOUNT_DIR/${DEVICE}" >> $LOG_FILE 2>&1

    # make sure the Pi user owns this folder
    sudo chown -R pi:pi "$MOUNT_DIR/${DEVICE}" >> $LOG_FILE 2>&1

   # mount the device base on USB file system

   case "$FILESYSTEM" in

       vfat)  sudo mount -t vfat -o utf8,uid=pi,gid=pi "/dev/${DEVICE}" "$MOUNT_DIR/${DEVICE}" >> $LOG_FILE 2>&1
              ;;

              # use locale setting for ntfs
       ntfs)  sudo mount -t auto -o uid=pi,gid=pi,locale=en_US.UTF-8 "/dev/${DEVICE}" "$MOUNT_DIR/${DEVICE}" >> $LOG_FILE 2>&1
              ;;

              # ext2/3/4 do not like uid option
       ext*)  sudo mount -t auto -o sync,noatime "/dev/${DEVICE}" "$MOUNT_DIR/${DEVICE}" >> $LOG_FILE 2>&1
              ;;
   esac

   # Allow time for device to be mounted
   sleep 2

   device_is_mounted=`grep ${DEVICE} /etc/mtab`
   if [ -n "$device_is_mounted" ]; then
      echo "/dev/${DEVICE} successfully  mounted" >> $LOG_FILE 2>&1
   fi

   # all done here, return successful
   exit 0
fi

exit 1
\$\endgroup\$
  • \$\begingroup\$ Hi, I rolled back to the previous version. It is our policy to roll back changes that invalidate answers. If you would like another round of code review of the revised code, the recommended practice is to post that as a new question, linking back to the previous version, with a brief summary of what has changed \$\endgroup\$ – Stop ongoing harm to Monica Jul 6 '16 at 5:39
  • \$\begingroup\$ Ahh okay - that makes sense. I'll do that. I'm going to add some file processing to it to extend it a bit. \$\endgroup\$ – dbmitch Jul 6 '16 at 16:20
  • \$\begingroup\$ Revised code and review at codereview.stackexchange.com/questions/134233/… \$\endgroup\$ – dbmitch Oct 18 '18 at 18:02
1
\$\begingroup\$

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.

That is, instead of this:

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

Write like this:

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

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks so much. I'm going to print off your reply and have a closer look. Really appreciated. \$\endgroup\$ – dbmitch Jul 5 '16 at 20:11
  • \$\begingroup\$ doing some editing now and REALLY like the ideas of removing the repeated redirections. Besides being easier to read it helps me understand how bash functions are constructed and used. One clarification - when you say to use the more modern $(...) inseatd of quotes - would that being like when I was using "$DEVICE" instead of ${DEVICE}? \$\endgroup\$ – dbmitch Jul 6 '16 at 0:02
  • \$\begingroup\$ I added an example to clarify. My comment concerns subshells, ${...} is something entirely different, it's variable substitution, and there is no strong recommendation regarding that \$\endgroup\$ – Stop ongoing harm to Monica Jul 6 '16 at 5:43
  • \$\begingroup\$ Had to post a question in StackOverflow - I can't seem to get the function defined properly using your suggestion even though it passes ShellCheck. Get error is on line 24 syntax error near unexpected token ${\r'' autounload() { - right where I define the function - anyways. it's at stackoverflow.com/questions/38234756/bash-syntax-error \$\endgroup\$ – dbmitch Jul 6 '16 at 22:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.