Bug#502352: cleaning usbmount

[Cristian Ionescu-Idbohrn]

On Sun, 25 Oct 2009, Rogério Brito wrote:

> On Oct 25 2009, Cristian Ionescu-Idbohrn wrote:
> > On Sun, 25 Oct 2009, Rogério Brito wrote:
> > >    
> > > http://svn.debian.org/viewsvn/usbmount/usbmount/trunk/usbmount?revision=63
> >
> > I'll have a look.
>
> Great. I appreciate.

Here's some more (see attached complete patch against svn trunk).
Main points are:

* trap shouldn't need to be run in a subshell

* there's a difference between [:blank:] and [:space:] character classes:

       [:blank:]
              all horizontal whitespace
       [:space:]
              all horizontal or vertical whitespace

  I think you want [:blank:].

* sed is superfluous when it comes to stripping whitespace in a string:

        vendor=$(echo $vendor)

  produces better and cheaper results than:

        vendor="$(echo "$vendor" | sed 's/^[[:space:]]\+//; 
s/[[:space:]]\+$//')"


Cheers,

-- 
Cristian
Index: usbmount/trunk/usbmount
===================================================================
--- usbmount/trunk/usbmount	(revision 73)
+++ usbmount/trunk/usbmount	(working copy)
@@ -20,18 +20,15 @@
 # Auxiliary functions
 
 # Log a string via the syslog facility.
-log()
-{
+log() {
     if [ $1 != debug ] || expr "$VERBOSE" : "[yY]" > /dev/null; then
 	logger -p user.$1 -t "usbmount[$$]" -- "$2"
     fi
 }
 
-
 # Test if the first parameter is in the list given by the second
 # parameter.
-in_list()
-{
+in_list() {
     for v in $2; do
 	[ "$1" != "$v" ] || return 0
     done
@@ -74,23 +71,23 @@
 
 umask 022
 
-
 if [ "$1" = add ]; then
-
     # Acquire lock.
     log debug "trying to acquire lock /var/run/usbmount/.mount.lock"
-    lockfile-create --retry 3 /var/run/usbmount/.mount || \
-	{ log err "cannot acquire lock /var/run/usbmount/.mount.lock"; exit 1; }
-    trap '( lockfile-remove /var/run/usbmount/.mount )' 0
+    lockfile-create --retry 3 /var/run/usbmount/.mount || {
+	log err "cannot acquire lock /var/run/usbmount/.mount.lock"
+	exit 1
+    }
+    trap 'lockfile-remove /var/run/usbmount/.mount' EXIT
     log debug "acquired lock /var/run/usbmount/.mount.lock"
 
     # Grab device information from device and "divide it"
     #   FIXME: improvement: implement mounting by label (notice that labels
     #   can contain spaces, which makes things a little bit less comfortable).
     DEVINFO=$(/sbin/blkid -p $DEVNAME)
-    FSTYPE=$(echo "$DEVINFO" | sed 's/.*TYPE="\([^"]*\)".*/\1/g; s/[[:space:]]*//g;')
-    UUID=$(echo "$DEVINFO"   | sed 's/.*UUID="\([^"]*\)".*/\1/g; s/[[:space:]]*//g;')
-    USAGE=$(echo "$DEVINFO"  | sed 's/.*USAGE="\([^"]*\)".*/\1/g; s/[[:space:]]*//g;')
+    FSTYPE=$(echo "$DEVINFO" | sed 's/.*TYPE="\([^"]*\)".*/\1/g; s/[[:blank:]]*//g;')
+    UUID=$(echo "$DEVINFO"   | sed 's/.*UUID="\([^"]*\)".*/\1/g; s/[[:blank:]]*//g;')
+    USAGE=$(echo "$DEVINFO"  | sed 's/.*USAGE="\([^"]*\)".*/\1/g; s/[[:blank:]]*//g;')
 
     if ! echo $USAGE | egrep -q "(filesystem|disklabel)"; then
 	log info "$DEVNAME does not contain a filesystem or disklabel"
@@ -98,14 +95,12 @@
     fi
 
     # Try to use specifications in /etc/fstab first.
-    if egrep -q "[[:space:]]*$DEVNAME" /etc/fstab; then
+    if egrep -q "[[:blank:]]*$DEVNAME" /etc/fstab; then
 	log info "executing command: mount $DEVNAME"
 	mount $DEVNAME
-
     elif grep -q $UUID /etc/fstab; then
-        log info "executing command: mount -U $UUID"
+	log info "executing command: mount -U $UUID"
 	mount -U $UUID
-
     else
 	log debug "$DEVNAME contains filesystem type $FSTYPE"
 
@@ -115,13 +110,13 @@
 	if in_list "$fstype" "$FILESYSTEMS"; then
 	    # Search an available mountpoint.
 	    for v in $MOUNTPOINTS; do
-		if [ -d "$v" ] && ! grep -q "^[^ ][^ ]*  *$v " /proc/mounts; then
+		if [ -d "$v" ] && ! egrep -q "^[^[:blank:]]+[[:blank:]]+$v[[:blank:]]+" /proc/mounts; then
 		    mountpoint="$v"
 		    log debug "mountpoint $mountpoint is available for $DEVNAME"
 		    break
 		fi
 	    done
-	    if [ -n "$mountpoint" ]; then
+	    if [ "$mountpoint" ]; then
 		# Determine mount options.
 		options=
 		for v in $FS_MOUNTOPTIONS; do
@@ -130,7 +125,7 @@
 			break
 		    fi
 		done
-		if [ -n "$MOUNTOPTIONS" ]; then
+		if [ "$MOUNTOPTIONS" ]; then
 		    options="$MOUNTOPTIONS${options:+,$options}"
 		fi
 
@@ -149,7 +144,7 @@
 		elif [ -r "/sys$DEVPATH/../device/../manufacturer" ]; then
 		    vendor="`cat \"/sys$DEVPATH/../device/../manufacturer\"`"
 		fi
-		vendor="$(echo "$vendor" | sed 's/^[[:space:]]\+//; s/[[:space:]]\+$//')"
+		vendor=$(echo $vendor)
 
 		model=
 		if [ -r "/sys$DEVPATH/device/model" ]; then
@@ -161,7 +156,7 @@
 		elif [ -r "/sys$DEVPATH/../device/../product" ]; then
 		    model="`cat \"/sys$DEVPATH/../device/../product\"`"
 		fi
-		model="$(echo "$model" | sed 's/^[[:space:]]\+//; s/[[:space:]]\+$//')"
+		model=$(echo $model)
 
 		# Run hook scripts; ignore errors.
 		export UM_DEVICE="$DEVNAME"
@@ -180,7 +175,6 @@
 	fi
     fi
 elif [ "$1" = remove ]; then
-
     # A block or partition device has been removed.
     # Test if it is mounted.
     while read device mountpoint fstype remainder; do