Bash partitioning script

Question

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

Answer 1

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?

Answer 2

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.