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.