5
\$\begingroup\$

What can I improve in this script? Do I read user's yes/no answers by right way? Should I write bash scripts so that to be compatible with POSIX?

#!/usr/bin/env bash

set -o errexit
set -o pipefail
set -o nounset
# set -o xtrace

if ! command -v zpool &> /dev/null || ! command -v zfs &> /dev/null; then
    printf "ZFS binary does not exist. Exiting...\n"
    exit 1
fi

LOCK_FILE="/tmp/prepare_zfs.lock"

if [[ -f ${LOCK_FILE} ]]; then
    printf "Lock file already exists. Exiting...\n"
    exit 1
fi

touch "${LOCK_FILE}"
trap 'stty echo; rm -f "${LOCK_FILE}"; exit $?' INT TERM EXIT

while true; do
    read -rp "Enter disk device path: " DRIVE
    if [[ ! -b "${DRIVE}" ]]; then
        printf "Error: not a device.\n"
    else
        break
    fi
done

while true; do
    read -rp "Enter mount path (leave blank for /mnt): " MNT_PATH
    MNT_PATH=${MNT_PATH:-/mnt}
    if [[ ! -d "${MNT_PATH}" ]]; then
        printf "Error: not a directory.\n"
    elif [[ $(findmnt -M /mnt/"${MNT_PATH}") ]]; then
        printf "Error: already mounted.\n"
    else
        break
    fi
done

while true; do
    read -rp "Do you want to use encryption (answer y or n)? " ZFS_ENC_ENABLED
    if [[ ${ZFS_ENC_ENABLED} == "y" || ${ZFS_ENC_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

readonly ZFS_ENC_PASSWD="p"
readonly ZFS_ENC_KEYFILE="k"
if [[ "${ZFS_ENC_ENABLED}" ]]; then
    while true; do
        read -rp "Do you want to use (p)assword or generate (k)eyfile? " ZFS_ENC_TYPE
        if [[ "${ZFS_ENC_TYPE}" == "${ZFS_ENC_PASSWD}" || "${ZFS_ENC_TYPE}" == "${ZFS_ENC_KEYFILE}" ]]; then
            break
        fi
        printf "Error: invalid answer, valid choices are p and k.\n"
    done

    case ${ZFS_ENC_TYPE} in
        "${ZFS_ENC_PASSWD}")
            while true; do
                stty -echo
                read -rp "Password: " PASSWORD
                printf "\n"
                read -rp "Confirm password: " CONFIRM_PASSWORD
                printf "\n"
                stty echo
                if [[ "${PASSWORD}" != "${CONFIRM_PASSWORD}" ]]; then
                    printf "Error: passwords don't match.\n"
                else
                    break
                fi
            done
            ;;
        "${ZFS_ENC_KEYFILE}")
            ZFS_INSECURE_KEY="n"
            while true; do
                [[ "${ZFS_INSECURE_KEY}" == "y" ]] && break
                read -rp "Do you want to generate 128-bit, 192-bit or 256-bit keyfile? (default: 256) " ZFS_ENC_KEYLEN
                if [[ "${ZFS_ENC_KEYLEN}" == "128" ]]; then
                    while true; do
                        read -rp "Are you sure you want to use insecure key size (answer y or n)? " ZFS_INSECURE_KEY
                        if [[ ${ZFS_INSECURE_KEY} == "y" || ${ZFS_INSECURE_KEY} == "n" ]]; then
                            break
                        fi
                        printf "Error: invalid answer, valid choices are y and n.\n"
                    done
                elif [[ "${ZFS_INSECURE_KEY}" == "n" ]]; then
                    continue
                elif [[ "${ZFS_ENC_KEYLEN}" == "192" || "${ZFS_ENC_KEYLEN}" == "256" ]]; then
                    break
                else
                    printf "Error: invalid answer, valid answers are 128, 192 and 256.\n"
                fi
            done
            while true; do
                read -rp "Do you want to generate (h)ex or (r)aw keyfile? " ZFS_ENC_KEYTYPE
                if [[ "${ZFS_ENC_KEYTYPE}" == "h" || "${ZFS_ENC_KEYTYPE}" == "r" ]]; then
                    break
                else
                    printf "Error: invalid answer, valid answers are h and r.\n"
                fi
            done
            ;;
    esac
fi

read -rp "Enter zroot name (leave blank for zroot): " ZROOT
ZROOT=${ZROOT:-zroot}
read -rp "Enter username (leave blank for user): " USERNAME
USERNAME=${USERNAME:-user}

while true; do
    read -rp "Will you be using libvirt (answer y or n)? " LIBVIRT_ENABLED
    if [[ ${LIBVIRT_ENABLED} == "y" || ${LIBVIRT_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

while true; do
    read -rp "Will you be using LXC (answer y or n)? " LXC_ENABLED
    if [[ ${LXC_ENABLED} == "y" || ${LXC_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

while true; do
    read -rp "Will you be using LXD (answer y or n)? " LXD_ENABLED
    if [[ ${LXD_ENABLED} == "y" || ${LXD_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

while true; do
    read -rp "Will you be using Docker (answer y or n)? " DOCKER_ENABLED
    if [[ ${DOCKER_ENABLED} == "y" || ${DOCKER_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

while true; do
    read -rp "Will you be using NFS (answer y or n)? " NFS_ENABLED
    if [[ ${NFS_ENABLED} == "y" || ${NFS_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

while true; do
    read -rp "Will you be using systemd (answer y or n)? " SYSTEMD_ENABLED
    if [[ ${SYSTEMD_ENABLED} == "y" || ${SYSTEMD_ENABLED} == "n" ]]; then
        break
    fi
    printf "Error: invalid answer, valid choices are y and n.\n"
done

DISK_SECTOR_SIZE=$(lsblk -S -o PHY-SEC --raw --noheadings "${DRIVE}")
[[ "${DISK_SECTOR_SIZE}" == "512" ]] && ZPOOL_ASHIFT=9 || ZPOOL_ASHIFT=12
ZPOOL_ARGS="-f -o ashift=${ZPOOL_ASHIFT}           \
            -o cachefile=/etc/zfs/zpool.cache
            -O acltype=posixacl       \
            -O relatime=on            \
            -O xattr=sa               \
            -O dnodesize=legacy       \
            -O normalization=formD    \
            -O mountpoint=none        \
            -O canmount=off           \
            -O devices=off            \

            -R /mnt                   "

# zpool create -o ashift=12 -o cachefile=/etc/zfs/zpool.cache -m none -R "${MNT_PATH}" "${ZROOT}" "${DRIVE}"
# zfs create -o mountpoint=none -o com.sun:auto-snapshot=true -o compression=lz4 "${ZROOT}"/ROOT
# zfs create -o mountpoint=/ -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/default
# zfs create -o canmount=off -o mountpoint=/var -o com.sun:auto-snapshot=true -o xattr=sa "${ZROOT}"/ROOT/var
# zfs create -o canmount=off -o mountpoint=/var/lib -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/var/lib
# test "${LIBVIRT_ENABLED}" == "y" && zfs create -o mountpoint=/var/lib/libvirt -o com.sun:auto-snapshot=false "${ZROOT}"/ROOT/var/lib/libvirt
# test "${LXC_ENABLED}" == "y" && zfs create -o mountpoint=/var/lib/lxc -o com.sun:auto-snapshot=false "${ZROOT}"/ROOT/var/lib/lxc
# test "${DOCKER_ENABLED}" == "y" && zfs create -o mountpoint=/var/lib/docker -o com.sun:auto-snapshot=false "${ZROOT}"/ROOT/var/lib/docker
# test "${NFS_ENABLED}" == "y" && zfs create -o mountpoint=/var/lib/nfs -o com.sun:auto-snapshot=false "${ZROOT}"/ROOT/var/lib/nfs
# test "${SYSTEMD_ENABLED}" == "y" && zfs create -o canmount=off -o mountpoint=/var/lib/systemd -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/var/lib/systemd
# zfs create -o canmount=off -o mountpoint=/usr -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/usr
# zfs create -o mountpoint=/usr/local -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/usr/local
# zfs create -o mountpoint=/opt -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/opt
# test "${SYSTEMD_ENABLED}" == "y" && zfs create -o mountpoint=/var/lib/systemd/coredump -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/var/lib/systemd/coredump
# zfs create -o mountpoint=/var/log -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/var/log
# test "${SYSTEMD_ENABLED}" == "y" && zfs create -o acltype=posixacl -o mountpoint=/var/log/journal -o com.sun:auto-snapshot=true "${ZROOT}"/ROOT/var/log/journal
# zfs create -o mountpoint=/home -o com.sun:auto-snapshot=true "${ZROOT}"/home
# zfs create -o mountpoint=/root -o com.sun:auto-snapshot=true "${ZROOT}"/home/root
# zfs create -o mountpoint=/home/"${USERNAME}" -o com.sun:auto-snapshot=true "${ZROOT}"/home/"${USERNAME}"
# zpool set bootfs="${ZROOT}" "${ZROOT}"
# zfs set relatime=on "${ZROOT}"
# zfs set com.sun:auto-snapshot=true "${ZROOT}"
# zpool export "${ZROOT}"
# zpool import -o cachefile=/etc/zfs/zpool.cache -R "${MNT_PATH}" "${ZROOT}"

# clean up after yourself, and release your trap
rm -f "${LOCK_FILE}"
trap - INT TERM EXIT
\$\endgroup\$
0
7
\$\begingroup\$

I'm just reviewing the code, not the meaning behind it.


Get out of the habit of using ALLCAPS variable names, leave those as reserved by the shell. One day you'll write PATH=something and then wonder why your script is broken.


LOCK_FILE="/tmp/prepare_zfs.lock"

if [[ -f ${LOCK_FILE} ]]; then
    printf "Lock file already exists. Exiting...\n"
    exit 1
fi

touch "${LOCK_FILE}"
trap 'stty echo; rm -f "${LOCK_FILE}"; exit $?' INT TERM EXIT

There's a race condition there. Either use flock or use a lock directory -- mkdir is atomic:

readonly lockdir=/tmp/prepare_zfs.lock
if ! mkdir "$lockdir"; then
    printf "Lock file already exists. Exiting...\n"
    exit 1
fi
trap 'stty echo; rmdir "${lockdir}"; exit $?' INT TERM EXIT

The "yes/no" prompts: You do this a lot, use a function not cut'n'paste code. I'm assuming you have bash 4.3+, this uses a nameref to set the caller's variable

yesno() {
    local question=$1
    local -n answer=$2
    while true; do
        read -rp "$question (answer y or n)? " answer
        case $answer in
            y|n) return;;
            *) printf "Error: invalid answer, valid choices are y and n.\n" >&2 ;;
        esac
    done
}

yesno "Do you want to use encryption" zfsEncEnabled

USERNAME=${USERNAME:-user}

An alternative:

: ${USERNAME:=user}

ZPOOL_ARGS="-f -o ashift=${ZPOOL_ASHIFT}           \
            -o cachefile=/etc/zfs/zpool.cache
            -O acltype=posixacl       \
            -O relatime=on            \
            -O xattr=sa               \
            -O dnodesize=legacy       \
            -O normalization=formD    \
            -O mountpoint=none        \
            -O canmount=off           \
            -O devices=off            \

            -R /mnt                   "

This is the wrong way to stash the args. Use an array: it's the appropriate data structure and much more forgiving about whitespace. See http://mywiki.wooledge.org/BashFAQ/050 for more details

zpoolArgs=(
    -f
    -o ashift="$zpoolAshift"
    -o cachefile=/etc/zfs/zpool.cache
    -O acltype=posixacl
    -O relatime=on
    -O xattr=sa
    -O dnodesize=legacy
    -O normalization=formD
    -O mountpoint=none
    -O canmount=off
    -O devices=off
    -R /mnt
)

Then you'll use the array like "${zpoolArgs[@]}" -- with the quotes.


Should I write bash scripts so that to be compatible with POSIX?

IMO, no. Take advantage of bash-specific features to make your life easier. Will this script reside on multiple servers, or in a place where you know that an appropriate version of bash is installed?

\$\endgroup\$
5
  • \$\begingroup\$ Really nice answer, especially catching the race condition and the proposed solution. \$\endgroup\$
    – Jonah
    Jun 15 at 3:31
  • \$\begingroup\$ I'm getting an error rmdir: failed to remove '/tmp/prepare_zfs.lock': No such file or directory after modifying my code to follow your suggestion when I exit script with Ctrl+C. \$\endgroup\$
    – user243725
    Jun 15 at 18:35
  • \$\begingroup\$ Dunno. Stick an echo statement into the trap to see if it's being triggered twice (maybe once for INT and again for EXIT) \$\endgroup\$ Jun 15 at 18:38
  • \$\begingroup\$ The trap is triggered twice, what should I change in my script? \$\endgroup\$
    – user243725
    Jun 15 at 18:59
  • \$\begingroup\$ INT and TERM are real signals, and EXIT is a pseudo-signal triggered by bash. You might want to just trap EXIT since int and term will exit the script. I don't know if that's best practice though. \$\endgroup\$ Jun 15 at 20:29
4
\$\begingroup\$

This while true loop can be simplified:

while true; do
    read -rp "Enter disk device path: " DRIVE
    if [[ ! -b "${DRIVE}" ]]; then
        printf "Error: not a device.\n"
    else
        break
    fi
done

Having true as the condition should be rare. Here, we want to loop until "$DRIVE" is a block device, so that's what we should write:

until
    read -p "Enter disk device path: " drive
    [ -b "${drive}" ]
do
    echo >&2 "$drive: not a device."
done

Some other changes in that rewrite: don't disable input editing (read -r), since we expect this to be used interactively. Don't use all-caps for shell variables, to avoid collision with environment variables. There's no reason to use Bash-specific [[ command, which makes it harder to re-use the code in other shell scripts. Error messages should go to the error stream.

There are many loops similar to these, and at least a few that all ask yes/no questions - easily refactored into a function.

A lot of loops ask for information that's never used (Docker, systemd). These should simply be removed.

The final lines

rm -f "${LOCK_FILE}"
trap - INT TERM EXIT

are unnecessary - simply exit; that will execute the trap, which will remove the lock file.

I'm not convinced it's a good idea to prevent multiple instances running concurrently against different block devices. Probably better to make the lock per-target if we have one at all.

\$\endgroup\$

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.