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?

• Jonah: Really nice answer, especially catching the race condition and the proposed solution.
• user243725: 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.
• glenn jackman: Dunno. Stick an echo statement into the trap to see if it's being triggered twice (maybe once for INT and again for EXIT)
• user243725: The trap is triggered twice, what should I change in my script?
• glenn jackman: 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.

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.