4
\$\begingroup\$

I have a shell script to mount different remote systems. While it does work, the area I struggle with (not being a programmer) is the logic of the script: it feels quite stilted and there seems to be some unnecessary duplication.

I would appreciate any comments as to how I could improve it - both in terms of the logic, but also any other tips about style or approach.

#!/bin/sh
# mount boxes by SSHFS

usage () {
    cat <<EOF
    sshmnt -[c,s,u,h]
      -c Box1
      -s Box2
      -u unmount
      -h print this message

EOF
}

mnt(){
    sshfs jason@"$host":/home/jason /media/"$dir" \
    -C -p "$port" \
    -o reconnect,IdentityFile=/home/jason/.ssh/id_rsa \
    && ls /media/"$dir"
}

umnt(){
    fusermount -u /media/"$dir"
}

# unmounting 
if [ "$1" = "-u" ]; then
    box=$(mount | grep Box1)
    if [ -n "$box" ]; then
        dir=Box1
    else
        dir=Box2
    fi
    umnt && exit
fi

# check if on LAN
lan="$(ip addr | grep .102/)"

if [ -n "$lan" ]; then 
    case "$1" in
      -c) int=100
          ;;
      -s) int=200 
          ;;
    esac
    host="192.168.1.$int"
else
    host="XXX.XXX.XXX.XXX"
fi

# box specifics
case "$1" in
  -c) dir=Box1 port="XXXX"
      mnt
      ;;
  -s) dir=Box2  port="XXXX"
      mnt
      ;;
   *) usage && exit
       ;;
esac

I have obscured the relevant ports and hostnames.

\$\endgroup\$
8
\$\begingroup\$

Your functions shouldn't depend on variables but use specified parameters. You always specified variables before calling your function instead of just specifying them as parameters, e.g:

DIR=Box1
Host=192.168.1.100
mnt

but you can easily write it as

mnt Box1 192.168.1.100

I would also only parse your command line arguments in one place and create a dedicated function for determine the appropriate ip address for your hosts.

I would also remove the ls call from your mnt function as mnt doesn't do what the name suggests. But this is a rather minor issue and I didn't change it in my code.

#!/bin/sh
# mount boxes by SSHFS

usage () {
    cat <<EOF
sshmnt -[c,s,u,h]
      -c Box1
      -s Box2
      -u unmount
      -h print this message

EOF
}

mnt(){

    if [ $# -ne 3 ] ; then
       echo "Wrong parameters for mnt - host dir port" >&2
       return 1
    fi

    host="$1"
    dir="$2"
    port="$3"
    sshfs jason@"$host":/home/jason /media/"$dir" \
    -C -p "$port" \
    -o reconnect,IdentityFile=/home/jason/.ssh/id_rsa \
    && ls /media/"$dir"
}

umnt(){
    dir="$1"
    fusermount -u /media/"$dir"
}

get_host(){
    # check if on LAN
    lan="$(ip addr | grep .102/)"

    if [ -z "$lan" ] ; then
        echo XXX.XXX.XXX.XXX
        return 0
    fi

    case "$1" in
        Box1) echo 192.168.1.100 ;;
        Box2) echo 192.168.1.200 ;;
        *) echo "Unknown Parameter" >&2 && return 1 ;;
    esac
}

case "$1" in
    -c) mnt $(get_host Box1) Box1 port ;;
    -s) mnt $(get_host Box2) Box2 port ;;
    -u)
        mount | grep Box1 && umnt Box1
        mount | grep Box2 && umnt Box2
        ;;
     *) usage && exit 1;;
esac

Another suggestion is to get rid of all your hard coded paths and texts like, /media/, /home/jason/ and jason and replace them with variables, e.g:

MOUNT_PREFIX=/media/
SSH_KEY=/home/jason/.ssh/id_rsa
MOUNT_SOURCE=/home/jason

and just specify the user for your hosts in ~/.ssh/config

\$\endgroup\$
  • \$\begingroup\$ Thanks Ulrich: that is very helpful. Would you mind expanding on your comment that "functions shouldn't depend on environment variables but use specified parameters"? I don't quite follow. \$\endgroup\$ – jasonwryan Aug 8 '12 at 0:12
  • \$\begingroup\$ @jasonwryan ok updated the answer - also have a look at the last paragraph about your hard coded values \$\endgroup\$ – Ulrich Dangel Aug 8 '12 at 0:20
  • \$\begingroup\$ thank you for the added detail; it was a lightbulb moment. The extra variables are also a good suggestion. \$\endgroup\$ – jasonwryan Aug 8 '12 at 0:37

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.