Setup shellcheck and fix all issues

.github/workflows/pr-review.yml

@@ -0,0 +1,41 @@
+# PR Review workflows.
+
+name: pr-review
+on:
+  workflow_dispatch:
+  pull_request:
+  push:
+jobs:
+  shellcheck-code:
+    name: shellcheck main code
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: shellcheck
+        uses: reviewdog/action-shellcheck@v1
+        with:
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review
+          path: "."
+          pattern: |
+            grml-live
+            scripts/*.sh
+            remaster/grml-live-remaster
+            etc/grml/fai/config/hooks/*
+            etc/grml/fai/config/scripts/*
+          check_all_files_with_shebangs: "false"
+
+  shellcheck-tests:
+    name: shellcheck test scripts
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: shellcheck
+        uses: reviewdog/action-shellcheck@v1
+        with:
+          github_token: ${{ secrets.github_token }}
+          reporter: github-pr-review
+          path: tests
+          pattern: |
+            *.sh
+          check_all_files_with_shebangs: "false"

etc/grml/fai/config/hooks/instsoft.GRMLBASE

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets ${target}, but shellcheck does not know that.
+target=${target:?}
+
 # if hooks/updatebase.GRMLBASE fails for whatever reason
 # and can't skip instsoft.GRMLBASE we have to make sure
 # we exit here as well
@@ -24,16 +27,16 @@ if [ "$FAI_ACTION" = "softupdate" ] ; then
    # /etc/resolv.conf is usually a symlink, pointing out of the chroot.
    # Make it a file with known contents.
    rm -f "${target}"/etc/resolv.conf
-   cat /etc/resolv.conf >> "$target"/etc/resolv.conf
+   cat /etc/resolv.conf >> "${target}"/etc/resolv.conf
 
-   if [ -r $target/etc/policy-rc.d.conf ] ; then
-      sed -i "s/EXITSTATUS=.*/EXITSTATUS='101'/" $target/etc/policy-rc.d.conf
+   if [ -r "${target}"/etc/policy-rc.d.conf ] ; then
+      sed -i "s/EXITSTATUS=.*/EXITSTATUS='101'/" "${target}"/etc/policy-rc.d.conf
    fi
 
    # we definitely don't want to fail running fai sofupdate just
    # because of some well known bugs:
-   [ -d $target/etc/apt/apt.conf.d ] || mkdir $target/etc/apt/apt.conf.d
-   cat > $target/etc/apt/apt.conf.d/10apt-listbugs << EOF
+   [ -d "${target}"/etc/apt/apt.conf.d ] || mkdir "${target}"/etc/apt/apt.conf.d
+   cat > "${target}"/etc/apt/apt.conf.d/10apt-listbugs << EOF
 // Check all packages whether they has critical bugs before they are installed.
 // If you don't like it, comment it out.
 //DPkg::Pre-Install-Pkgs {"/usr/sbin/apt-listbugs apt || exit 10"};
@@ -87,19 +90,22 @@ EOF
 
    if $ROOTCMD test -x /usr/bin/aptitude ; then
       if $ROOTCMD aptitude --help | grep -q safe-upgrade ; then
+         # shellcheck disable=SC2086 # APTITUDE_OPTS needs word-splitting.
          APT_LISTCHANGES_FRONTEND=none APT_LISTBUGS_FRONTEND=none $ROOTCMD aptitude -y $APTITUDE_OPTS safe-upgrade
       else
+         # shellcheck disable=SC2086 # APTITUDE_OPTS needs word-splitting.
          APT_LISTCHANGES_FRONTEND=none APT_LISTBUGS_FRONTEND=none $ROOTCMD aptitude -y $APTITUDE_OPTS upgrade
       fi
    else
+      # shellcheck disable=SC2086 # APTGET_OPTS needs word-splitting.
       APT_LISTCHANGES_FRONTEND=none APT_LISTBUGS_FRONTEND=none $ROOTCMD apt-get -y $APTGET_OPTS --force-yes upgrade
    fi
 
    exit # make sure we don't continue behind the following "fi"
 fi
 
 # no softupdate but fresh installation
-echo "Action $FAI_ACTION of FAI (hooks/instsoft.GRMLBASE) via grml-live running"
+echo "Action ${FAI_ACTION} of FAI (hooks/instsoft.GRMLBASE) via grml-live running"
 
 # work around /etc/kernel/postinst.d/zz-update-grub failing
 # inside openvz environment, see #597084
@@ -120,8 +126,8 @@ fi
 
 # we definitely don't want to fail running fai dirinstall just
 # because of some well known bugs:
-[ -d $target/etc/apt/apt.conf.d ] || mkdir $target/etc/apt/apt.conf.d
-cat > $target/etc/apt/apt.conf.d/10apt-listbugs << EOF
+[ -d "${target}"/etc/apt/apt.conf.d ] || mkdir "${target}"/etc/apt/apt.conf.d
+cat > "${target}"/etc/apt/apt.conf.d/10apt-listbugs << EOF
 // Check all packages whether they has critical bugs before they are installed.
 // If you don't like it, comment it out.
 //DPkg::Pre-Install-Pkgs {"/usr/sbin/apt-listbugs apt || exit 10"};
@@ -130,13 +136,13 @@ cat > $target/etc/apt/apt.conf.d/10apt-listbugs << EOF
 EOF
 
 # make sure /dev/MAKEDEV is available:
-if [ -x "$target"/sbin/MAKEDEV ] && ! [ -r "$target"/dev/MAKEDEV ] ; then
-   ln -s /sbin/MAKEDEV "$target"/dev/MAKEDEV
+if [ -x "${target}"/sbin/MAKEDEV ] && ! [ -r "${target}"/dev/MAKEDEV ] ; then
+   ln -s /sbin/MAKEDEV "${target}"/dev/MAKEDEV
 fi
 
 # we don't need the invoke-rc.d.d diversion (we have grml-policyrcd :)):
-if [ -L "$target"/usr/sbin/invoke-rc.d ] ; then
-   rm -f "$target"/usr/sbin/invoke-rc.d
+if [ -L "${target}"/usr/sbin/invoke-rc.d ] ; then
+   rm -f "${target}"/usr/sbin/invoke-rc.d
    $ROOTCMD dpkg-divert --package fai --rename --remove /usr/sbin/invoke-rc.d
 fi
 

etc/grml/fai/config/hooks/instsoft.ZFS

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 # We don't want to install build-essential, dkms et al via package_config
 # because they will end up bloating the iso; it seems cleaner to install
 # them, build the zfs modules, then remove them.
@@ -29,9 +32,10 @@ echo "$0: Installing latest kernel and its headers, as well as build-essential."
 # keeping track of what gets installed. This is an ugly hack and should not
 # be needed, but without it the resulting ISO is hundreds of megabytes
 # larger. I hope this kludge can go away eventually.
-extra_packages=($($ROOTCMD apt-get --assume-no --download-only --mark-auto -u install \
+mapfile -t extra_packages < <($ROOTCMD \
+    apt-get --assume-no --download-only --mark-auto -u install \
 	build-essential linux-image-amd64 linux-headers-amd64 \
-	| sed '0,/The following NEW packages will be installed/d;/^[^ ]/,$d'))
+	| sed -e '0,/The following NEW packages will be installed/d;/^[^ ]/,$d' -e 's/\s/\n/g' | sed '/^$/d')
 $ROOTCMD apt-get --yes --mark-auto -u install build-essential linux-image-amd64 linux-headers-amd64
 
 # Remove all but the latest kernel (TODO: support passing in the desired
@@ -54,7 +58,11 @@ else
 fi
 
 echo "$0: Installing zfs-dkms itself."
-extra_packages=(${extra_packages[@]} $($ROOTCMD apt-get --assume-no --download-only --mark-auto -u install zfs-dkms | sed '0,/The following NEW packages will be installed/d;/^[^ ]/,$d'))
+mapfile -t zfs_packages < <($ROOTCMD \
+    apt-get --assume-no --download-only --mark-auto -u install \
+	zfs-dkms \
+	| sed -e '0,/The following NEW packages will be installed/d;/^[^ ]/,$d' -e 's/\s/\n/g' | sed '/^$/d')
+extra_packages=("${extra_packages[@]}" "${zfs_packages[@]}")
 $ROOTCMD apt-get --yes --mark-auto -u install zfs-dkms
 
 # Now invoke the dkms kernel postinst script for the only kernel that's left
@@ -66,16 +74,16 @@ $ROOTCMD /etc/kernel/postinst.d/dkms "$kernelversion"
 
 tempfile=$(mktemp)
 echo "$0: Saving built modules into a backup file (removing the dkms package will remove them, but we'll put them back)."
-$ROOTCMD tar cf - /lib/modules/$kernelversion/updates/dkms >$tempfile
+$ROOTCMD tar cf - "/lib/modules/$kernelversion/updates/dkms" >"$tempfile"
 
 echo "$0: Removing packages only needed to build zfs modules."
-remove_packages=($(echo "${extra_packages[@]}" zfs-dkms '^linux-headers-.*' build-essential $pahole | tr ' ' '\n' | sort -u))
-$ROOTCMD apt-get --yes --purge --autoremove remove ${remove_packages[@]}
+remove_packages=("${extra_packages[@]}" zfs-dkms '^linux-headers-.*' build-essential "$pahole")
+$ROOTCMD apt-get --yes --purge --autoremove remove "${remove_packages[@]}"
 echo "$0: Trying extra hard to get rid of auto-installed packages. This is a hack that is one of the ways we're trying to work around a perceived bug in apt autoremove and should be a no-op."
 $ROOTCMD apt-get --yes --purge autoremove
 
 echo "$0: Restoring backed-up kernel modules."
-$ROOTCMD tar xf - <$tempfile
-rm $tempfile
-$ROOTCMD depmod -a $kernelversion
+$ROOTCMD tar xf - <"$tempfile"
+rm "$tempfile"
+$ROOTCMD depmod -a "$kernelversion"
 echo "$0: Completed successfully. Enjoy your zfs."

etc/grml/fai/config/hooks/savelog.LAST.source

@@ -4,7 +4,7 @@
 # print errors and warnings found to error.log
 # WARNING: This will only work with english error messages!
 
-errfile=$LOGDIR/error.log
+errfile="$LOGDIR"/error.log
 
 # Define grep patterns. Do not start or end with an empty line!
 globalerrorpatterns="error
@@ -109,15 +109,15 @@ $myerrorpatterns"
 ignorepatterns="$globalignorepatterns
 $myignorepatterns"
 
-cd $LOGDIR || exit 3
-if [ -s $errfile ]; then
+cd "$LOGDIR" || exit 3
+if [ -s "$errfile" ]; then
     echo "Errorfile already exists. Aborting."
     exit
 fi
 
-grep -i "$errorpatterns" *.log | grep -vi "$ignorepatterns" > $errfile
+grep -i "$errorpatterns" ./*.log | grep -vi "$ignorepatterns" > "$errfile"
 
-if [ -s $errfile ]; then
+if [ -s "$errfile" ]; then
    echo "ERRORS found in log files. See $errfile."
 else
    echo "Congratulations! No errors found in log files."

etc/grml/fai/config/hooks/updatebase.GRMLBASE

@@ -8,15 +8,19 @@
 
 set -u
 set -e
+# shellcheck source=/dev/null
 . "$GRML_LIVE_CONFIG"
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 # visualize chroot inside zsh:
-echo grml_chroot > $target/etc/debian_chroot
+echo grml_chroot > "${target}"/etc/debian_chroot
 
-echo "$HOSTNAME" > $target/etc/hostname
+echo "$HOSTNAME" > "${target}"/etc/hostname
 
 if [ -n "${APT_PROXY:-}" ] ; then
-  cat > $target/etc/apt/apt.conf.d/90grml-apt-proxy.conf <<EOF
+  cat > "$target"/etc/apt/apt.conf.d/90grml-apt-proxy.conf <<EOF
 Acquire::http { Proxy "$APT_PROXY"; };
 EOF
 fi
@@ -29,24 +33,24 @@ if [ "$FAI_ACTION" = "softupdate" ] ; then
 
    ## based on FAI's lib/updatebase:
    # some packages must access /proc even in chroot environment
-   if ! [ -d $FAI_ROOT/proc/1 ] ; then
-      mount -t proc proc $FAI_ROOT/proc || true
+   if ! [ -d "$FAI_ROOT"/proc/1 ] ; then
+      mount -t proc proc "$FAI_ROOT"/proc || true
    fi
    # some packages must access /sys even in chroot environment
-   if ! [ -d $FAI_ROOT/sys/kernel ] ; then
-      mount -t sysfs sysfs $FAI_ROOT/sys || true
+   if ! [ -d "$FAI_ROOT"/sys/kernel ] ; then
+      mount -t sysfs sysfs "$FAI_ROOT"/sys || true
    fi
    # if we are using udev, also mount it into $FAI_ROOT
    if [ -f /etc/init.d/udev ] ; then
-      mount --bind /dev $FAI_ROOT/dev || true
+      mount --bind /dev "$FAI_ROOT"/dev || true
    fi
 
-   if [ -d $FAI_ROOT/run ] ; then
-      mount -t tmpfs tmpfs $FAI_ROOT/run || true
-      mkdir $FAI_ROOT/run/lock
+   if [ -d "$FAI_ROOT"/run ] ; then
+      mount -t tmpfs tmpfs "$FAI_ROOT"/run || true
+      mkdir "$FAI_ROOT"/run/lock
    fi
 
-   mount -t devpts devpts $FAI_ROOT/dev/pts || true
+   mount -t devpts devpts "$FAI_ROOT"/dev/pts || true
 
    # skip the task if we want to build a new ISO only,
    # this means we do NOT update any packages
@@ -61,9 +65,9 @@ if [ -n "$BOOTSTRAP_ONLY" ] ; then
 fi
 
 # work around #632624: udev fails to install on systems with old kernel versions
-if ! [ -e ${target}/etc/udev/kernel-upgrade ] ; then
+if ! [ -e "${target}"/etc/udev/kernel-upgrade ] ; then
   echo "Working around udev package bug, creating /etc/udev/kernel-upgrade"
-  echo "# installed via updatebase.GRMLBASE" > ${target}/etc/udev/kernel-upgrade
+  echo "# installed via updatebase.GRMLBASE" > "${target}"/etc/udev/kernel-upgrade
 fi
 
 # install all apt related files

etc/grml/fai/config/scripts/DEBORPHAN/10-whitelist

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 # workaround for dnsutils transitional package, we can drop this as soon as the
 # bind9-dnsutils package is available in all our supported Debian releases
 if [[ -r "${target}/usr/share/doc/dnsutils" ]] && [ -x "${target}/usr/bin/deborphan" ] ; then

etc/grml/fai/config/scripts/DEBORPHAN/98-clean-chroot

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 # remove all packages not necessary anymore:
 echo "Executing apt-get -y --purge autoremove"
 $ROOTCMD apt-get -y --purge autoremove
@@ -20,6 +23,7 @@ PURGE_PACKAGES=$($ROOTCMD dpkg --list | awk '/^rc/ {print $2}')
 
 if [ -n "$PURGE_PACKAGES" ] ; then
   echo "Getting rid of packages which have been removed but not yet purged: $PURGE_PACKAGES"
+  # shellcheck disable=SC2086 # PURGE_PACKAGES needs word-splitting.
   $ROOTCMD dpkg --purge $PURGE_PACKAGES
 fi
 
@@ -29,6 +33,7 @@ if [ -x "$target"/usr/bin/deborphan ] ; then
   # remove packages until deborphan does not find anymore:
   while [ "$($ROOTCMD deborphan)" != "" ] ; do
     echo "Executing deborphan"
+    # shellcheck disable=SC2046 # deborphan result needs word-splitting.
     $ROOTCMD apt-get -y --purge remove $($ROOTCMD deborphan)
   done
 fi

etc/grml/fai/config/scripts/GRMLBASE/01-packages

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 echo -n > "${LOGDIR}"/package_errors.log  # ensure we start with an empty file
 
 if ! [ -e "${LOGDIR}"/software.log ] ; then

etc/grml/fai/config/scripts/GRMLBASE/02-run

@@ -9,6 +9,9 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 # This is what initscripts would do if everything would be fine.
 if [ -L "$target/run" ] ; then
    rm -f "$target/run"

etc/grml/fai/config/scripts/GRMLBASE/03-get-sources

@@ -6,6 +6,9 @@
 # License:       This file is licensed under the GPL v2 or any later version.
 ################################################################################
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
 if ifclass SOURCES ; then
   echo "Class SOURCES set, retrieving source packages."
 else
@@ -27,6 +30,7 @@ mkdir -p "${target}${SOURCES_PATH}"
 $ROOTCMD apt-get update
 
 # Collect *source* package names
+# shellcheck disable=SC2016 # Embedded $ is correct.
 $ROOTCMD dpkg-query -W -f='${Source} ${Package}\n' | sed -e 's/^ //' | awk '{ print $1 }' | sort -u | \
   chroot "${target}" /bin/bash -c "cd \"${SOURCES_PATH}\" && xargs --max-args=32 --max-procs=12 apt-get --download-only source" 2> "${ERRORS_LOG}"
 

etc/grml/fai/config/scripts/GRMLBASE/05-hostname

@@ -9,18 +9,22 @@
 set -u
 set -e
 
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
+# shellcheck source=/dev/null
 . "$GRML_LIVE_CONFIG"
 
 # the hostname of the chroot usually isn't the same as the one for the live-system
 BUILD_HOSTNAME="$($ROOTCMD hostname)"
 [ -n "$BUILD_HOSTNAME" ] || BUILD_HOSTNAME="grml"
 
-echo "$HOSTNAME" > $target/etc/hostname
-echo "$HOSTNAME" > $target/etc/mailname
+echo "$HOSTNAME" > "$target"/etc/hostname
+echo "$HOSTNAME" > "$target"/etc/mailname
 
-if [ -r $target/etc/postfix/main.cf ] ; then
-  sed -i "s/^mydestination = .*/mydestination = $HOSTNAME, localhost, localhost.localdomain/" $target/etc/postfix/main.cf
-  sed -i "s/^myhostname = .*/myhostname = $HOSTNAME/" $target/etc/postfix/main.cf
+if [ -r "$target"/etc/postfix/main.cf ] ; then
+  sed -i "s/^mydestination = .*/mydestination = $HOSTNAME, localhost, localhost.localdomain/" "$target"/etc/postfix/main.cf
+  sed -i "s/^myhostname = .*/myhostname = $HOSTNAME/" "$target"/etc/postfix/main.cf
   echo "postfix postfix/mailname     string $HOSTNAME" | $ROOTCMD debconf-set-selections
   echo "postfix postfix/destinations string ${HOSTNAME}, localhost.grml.org, localhost" \
   | $ROOTCMD debconf-set-selections

etc/grml/fai/config/scripts/GRMLBASE/15-initsetup

@@ -8,6 +8,11 @@
 
 set -u
 set -e
+
+# FAI sets $target, but shellcheck does not know that.
+target=${target:?}
+
+# shellcheck source=/dev/null
 . "$GRML_LIVE_CONFIG"
 
 systemd_setup() {