On Thu, 3 Feb 2022 18:21:03 +0100
Pierre-Louis Bonicoli <pierre-louis.bonic...@libregerbil.fr> wrote:
> The logical sector size used by LUKS1 is 512 bytes. LUKS2 uses 512 to
> 4069 bytes.
I like that this has been added here. However there's no test that
actually runs this new code. Please add another two fs tests one for
LUKS and one for LUKS2.
>
> Signed-off-by: Pierre-Louis Bonicoli <pierre-louis.bonic...@libregerbil.fr>
> ---
> tests/util/grub-fs-tester.in | 58 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index aa72b2174..7ed275fd0 100644
> --- a/tests/util/grub-fs-tester.in
> +++ b/tests/util/grub-fs-tester.in
> @@ -15,7 +15,12 @@ XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset
> UTF-8"
>
> # This wrapper is to ease insertion of valgrind or time statistics
> run_it () {
> - LC_ALL=C "$GRUBFSTEST" "$@"
> + case x"$fs" in
> + xluks*)
> + LC_ALL=C echo -n pass | "$GRUBFSTEST" -C "$@";;
Why do this here instead of leaving this function alone and doing the
pipe and -C arg below when calling run_grubfstest?
> + *)
> + LC_ALL=C "$GRUBFSTEST" "$@";;
> + esac
> }
>
> range() {
> @@ -48,6 +53,8 @@ run_grubfstest () {
> MINLOGSECSIZE=9
> MAXLOGSECSIZE=9
> case x"$fs" in
> + xluks2*)
> + MAXLOGSECSIZE=12;;
> xntfs*)
> MINLOGSECSIZE=8
> MAXLOGSECSIZE=12;;
> @@ -335,7 +342,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> #FSLABEL="g;/_é莭莽😁кит u"
> ;;
> # FS LIMITATION: reiserfs, extN and jfs label is at most 16 UTF-8
> characters
> - x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"mdraid"*
> | x"jfs" | x"jfs_caseins")
> + x"reiserfs_old" | x"reiserfs" | x"ext"* | x"lvm"* | x"luks"* |
> x"mdraid"* | x"jfs" | x"jfs_caseins")
> FSLABEL="g;/éт 莭😁";;
> # FS LIMITATION: No underscore, space, semicolon, slash or
> international characters in UFS* in label. Limited to 32 UTF-8 characters
> x"ufs1" | x"ufs1_sun" | x"ufs2")
> @@ -804,6 +811,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> MOUNTDEVICE="/dev/mapper/grub_test-testvol"
> MOUNTFS=ext2
> "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}" ;;
> + x"luks"*)
> + echo -n pass | cryptsetup luksFormat --type $fs
> --sector-size $SECSIZE --pbkdf pbkdf2 $LODEVICE
> + echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> + MOUNTDEVICE="/dev/mapper/grub_luks_test"
> + MOUNTFS=ext2
> + "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}" ;;
> xf2fs)
> "mkfs.f2fs" -l "$FSLABEL" -q "${MOUNTDEVICE}" ;;
> xnilfs2)
> @@ -916,6 +929,20 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> GRUBDEVICE="mduuid/`mdadm --detail --export $MOUNTDEVICE |
> grep MD_UUID=|sed 's,MD_UUID=,,g;s,:,,g'`";;
> xlvm*)
> GRUBDEVICE="lvm/grub_test-testvol";;
> + xluks*)
> + if test x"$fs" = xluks2 && ! (cryptsetup luksDump
> --dump-json-metadata $LODEVICE | grep "\"sector_size\":$SECSIZE"); then
> + echo "Unexpected sector size for $LODEVICE
> (expected: $SECSIZE)"
> + exit 1
> + fi
> +
> + uuid=$(cryptsetup luksUUID $LODEVICE | tr -d '-')
> + probe_uuid=$(@builddir@/grub-probe --device $MOUNTDEVICE
> --target=cryptodisk_uuid)
> + if [ x"$uuid" != x"$probe_uuid" ]; then
> + echo "$fs probe command FAIL"
> + exit 1
> + fi
> + GRUBDEVICE="cryptouuid/${uuid}"
> + ;;
> esac
> GRUBDIR="($GRUBDEVICE)"
> case x"$fs" in
> @@ -1071,6 +1098,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> sleep 1
> vgchange -a n grub_test
> ;;
> + xluks*)
> + sleep 1
Bad indention. Why is this sleep desirable if there's a sleep in the
loop below?
> + for try in $(range 0 20 1); do
> + if umount "$MNTPOINTRW" ; then
> + break;
> + fi
> + sleep 1;
> + done
> + UMOUNT_TIME=$(date -u "+%Y-%m-%d %H:%M:%S")
> + sleep 1
Why sleeping here again? We already slept in the loop above. We need
more time?
> + cryptsetup close grub_luks_test
> + ;;
> xmdraid*)
> sleep 1
> for try in $(range 0 20 1); do
> @@ -1117,6 +1156,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> vgchange -a y grub_test
> sleep 1
> mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;
> + xluks*)
> + echo -n pass | cryptsetup open $LODEVICE grub_luks_test
> + sleep 1
Is this sleep really needed? Seems like you're trying to avoid a race,
but is the dm device ever never fully setup after cryptsetup returns?
> + mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
> ${MOUNTOPTS}${SELINUXOPTS}ro ;;
> xmdraid*)
> mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
> sleep 1
> @@ -1281,7 +1324,12 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> exit 1
> fi
>
> - LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`
> + case x"$fs" in
> + x"luks"*)
> + LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"|tail -n 1`;;
Here is where I'm think if would be better to do the pipe and add -C.
And why is this tail needed?
> + *)
> + LSOUT=`run_grubfstest ls -- -l "($GRUBDEVICE)"`;;
> + esac
> if [ x"$NOFSLABEL" = xy ]; then
> :
> elif echo "$LSOUT" | grep -F "Label \`$FSLABEL'" > /dev/null; then
> @@ -1558,6 +1606,10 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
> "$MAXLOGSECSIZE" 1); do
> vgchange -a n grub_test
> sleep 1
> ;;
> + xluks*)
> + cryptsetup close grub_luks_test
> + sleep 1
> + ;;
> esac
> case x"$fs" in
> x"tarfs" | x"cpio_"* | x"iso9660" | xrockridge | xjoliet |
> xrockridge_joliet | x"ziso9660" | x"romfs" | x"squash4_"* | x"iso9660_1999" |
> xrockridge_1999 | xjoliet_1999 | xrockridge_joliet_1999) ;;
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
