Re: [PATCH v5 2/6] expand-on-first-boot: support resizing a btrfs

["Roberto A. Foglietta" <roberto.foglietta@gmail.com>]

On Thu, 15 Dec 2022 at 15:28, <henning.schild@siemens.com> wrote:

> +MOUNT_POINT=$(mktemp -d --tmpdir "$(basename "$0").XXXXXXXXXX")

Good choice to use the long option --tmpdir but it is still less
readable than -p "$TMPDIR"

> +if [ ! -d "${MOUNT_POINT}" ]; then
> +       echo "Cannot create temporary mount point ${MOUNT_POINT}." >&2
> +       exit 1
> +fi

This is useless because set -e detected the failure also in child shells.

Just try these two commands:

roberto$ set -e
roberto$ test=$(false)

The terminal / shell will be closed immediately after pressing ENTER

> +mount "${LAST_PART}" "${MOUNT_POINT}"

If mktemp fails then also the mount will fail so whatever the shell
takes in account or not the subshell errors it is useless explicitly
check the mount point existence

> +
> +ret=0
> +# Determine the filesystem type and perform the appropriate resize function
> +FS_TYPE=$(findmnt -fno FSTYPE "${MOUNT_POINT}" )
> +case ${FS_TYPE} in
> +ext*)

Using ext*) seems legit but what about the introduction of extmyfs? A
completely different filesystem from ext[234]? I suggest ext[234]),
instead.

> +       # Do not fail resize2fs if no mtab entry is found, e.g.,
> +       # when using systemd mount units.
> +       export EXT2FS_NO_MTAB_OK=1
> +       resize2fs "${LAST_PART}"
> +       ;;
> +btrfs)
> +       btrfs filesystem resize max "${MOUNT_POINT}"Using ext*) seems legit but what about the introduction of extmyfs? A completely different filesystem from ext[234]? I suggest ext[234]), instead.
> +       ;;
> +*)
> +       echo "Unrecognized filesystem type ${FS_TYPE} - no resize performed"
> +       ret=1

Looking at your other patches this script has been added by default in
many templates. So, why consider a failure the execution of something
that the user might not even choose to use but just accepted by
default? Would it not be better to have a simple warning and nothing
else? Moreover, if the failure of this script does not deactivate its
service [as I hope] this script will run at every boot without any
reason. Adding an exit 0 here will also remove the use of ret variable
simplifying the code.

> +       ;;
> +esac
>
> -resize2fs "${LAST_PART}"
> +umount "${MOUNT_POINT}"
> +rmdir "${MOUNT_POINT}"
> +exit $ret
> --
> 2.37.4
>

Best regards, R-