[PATCH v3 0/5] Various test fixes and improvements

[PATCH v3 0/5] Various test fixes and improvements

[Glenn Washburn]

v3 - Fix botched v2 udate
v2 - Updated with Daniel's suggestions.

Glenn

Glenn Washburn (5):
  tests: Do not remove image file on error in pata_test
  tests: Skip pata_test on i386-efi
  tests: Remove $((BASE#NUM)) bashism in grub-fs-tester
  tests: Ensure that mountpoints are unmounted before exiting
  tests: Ensure that loopback devices and zfs devices are cleaned up

 tests/pata_test.in           |  4 ++-
 tests/util/grub-fs-tester.in | 56 ++++++++++++++++++++++++++++++------
 2 files changed, 51 insertions(+), 9 deletions(-)

Range-diff against v2:
1:  24b2a4bfd = 1:  313168dd7 tests: Do not remove image file on error in pata_test
2:  a64ebe41a = 2:  53df676e7 tests: Skip pata_test on i386-efi
3:  d2248490b = 3:  401b227cb tests: Remove $((BASE#NUM)) bashism in grub-fs-tester
4:  410461b20 ! 4:  8fb4d98fd tests: Ensure that mountpoints are unmounted before exiting
    @@ tests/util/grub-fs-tester.in: tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX
      XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
      
     +MOUNTS=
    -+umount_all() {
    -+    for MOUNT in $MOUNTS; do
    -+	umount "$MOUNT" &&
    -+	MOUNTS="$(echo ${MOUNTS} | sed "s|$MOUNT||g;")"
    ++cleanup() {
    ++    for i in $MOUNTS; do
    ++	umount "$i" || :
     +    done
     +}
    -+trap umount_all EXIT INT
    ++trap cleanup EXIT INT
     +# This is for bash, dash and ash do not recognize ERR
    -+trap umount_all ERR || :
    ++trap cleanup ERR || :
     +
      # This wrapper is to ease insertion of valgrind or time statistics
      run_it () {
5:  61bd6959b ! 5:  8b05a80a1 tests: Ensure that loopback devices and zfs devices are cleaned up
    @@ tests/util/grub-fs-tester.in: tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX
      XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
      
      MOUNTS=
    --umount_all() {
     +LODEVICES=
    -+cleanup() {
    + cleanup() {
     +    if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
     +	zpool list "$FSLABEL" 2>/dev/null &&
     +	while ! zpool export "$FSLABEL" ; do
    @@ tests/util/grub-fs-tester.in: tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX
     +	done
     +    fi
     +
    -     for MOUNT in $MOUNTS; do
    - 	umount "$MOUNT" &&
    - 	MOUNTS="$(echo ${MOUNTS} | sed "s|$MOUNT||g;")"
    +     for i in $MOUNTS; do
    + 	umount "$i" || :
          done
     +
     +    for lodev in $LODEVICES; do
    @@ tests/util/grub-fs-tester.in: tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX
     +    done
     +    return 0
      }
    --trap umount_all EXIT INT
    -+trap cleanup EXIT INT
    + trap cleanup EXIT INT
      # This is for bash, dash and ash do not recognize ERR
    --trap umount_all ERR || :
    -+trap cleanup ERR || :
    - 
    - # This wrapper is to ease insertion of valgrind or time statistics
    - run_it () {
-- 
2.27.0

[PATCH v3 1/5] tests: Do not remove image file on error in pata_test

[Glenn Washburn]

The image file can be useful in debugging an issue when the test fails.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 tests/pata_test.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/pata_test.in b/tests/pata_test.in
index 4fee0b0fb..27dccec19 100644
--- a/tests/pata_test.in
+++ b/tests/pata_test.in
@@ -47,7 +47,6 @@ tar cf "$imgfile" "$outfile"
 
 v=$(echo "nativedisk; source '($indisk)/$outfile';" | "${grubshell}" --qemu-opts="-$disk $imgfile")
 if [ "$v" != "Hello World" ]; then
-   rm "$imgfile"
    rm "$outfile"
    exit 1
 fi
-- 
2.27.0

[PATCH v3 2/5] tests: Skip pata_test on i386-efi

[Glenn Washburn]

In comparison to other i386 targets, on i386-efi the Q35 QEMU machine type
is used to do the testing to be able to make use of the EFI firmware in
QEMU. On the Q35 machine type there is no way to use ATA to communicate with
an IDE, only AHCI.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 tests/pata_test.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/pata_test.in b/tests/pata_test.in
index 27dccec19..31144a8fd 100644
--- a/tests/pata_test.in
+++ b/tests/pata_test.in
@@ -29,6 +29,9 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
     # PLATFORM: no ATA on ARC platforms (they use SCSI)
     *-arc)
 	exit 77;;
+    # QEMU: no ATA on Q35 machine type (they use AHCI)
+    i386-efi)
+	exit 77;;
     # FIXME: No native drivers are available for those
     powerpc-ieee1275 | sparc64-ieee1275 | arm*-efi)
 	exit 77;;
-- 
2.27.0

[PATCH v3 3/5] tests: Remove $((BASE#NUM)) bashism in grub-fs-tester

[Glenn Washburn]

This bashism allows converting NUM in base BASE to decimal. Its not needed
because the only place its used is to convert from hexidecimal and this can
also be done with the more portable $((0xHEXNUM)) syntax.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 tests/util/grub-fs-tester.in | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index aa72b2174..a1f3f299b 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -459,7 +459,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    # FIXME: not so sure about AFFS
 		    # OS LIMITATION: minix2/minix3 could be formatted in a way to permit more.
 		x"minix3" | x"minix2" | x"hfs"| x"affs" | xaffs_intl | xreiserfs_old | xext2_old)
-		    BIGBLOCKCNT=$((16#7fffffff));;
+		    BIGBLOCKCNT=$((0x7fffffff));;
 
 		    # FS LIMITATION: redundant storage
 		    # We have only limited space. Mirroring multiplies it very effectively.
@@ -669,18 +669,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 	# mkfs.hfs and mkfs.hfsplus don't fill UUID.
 		x"hfsplus")
 		    "mkfs.hfsplus" -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
-		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((16#468)) conv=notrunc count=8 ;;
+		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468)) conv=notrunc count=8 ;;
 		x"hfsplus_wrap")
 		    "mkfs.hfsplus" -w -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
-		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((16#468)) conv=notrunc count=8
+		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468)) conv=notrunc count=8
 		    MOUNTFS="hfsplus";;
 		x"hfsplus_casesens")
 		    "mkfs.hfsplus" -s -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
-		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((16#468)) conv=notrunc count=8
+		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468)) conv=notrunc count=8
 		    MOUNTFS="hfsplus";;
 		x"hfs")
 		    "mkfs.hfs" -b $BLKSIZE -v "`echo $FSLABEL |recode utf8..macroman`" -h "${MOUNTDEVICE}"
-		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((16#474)) conv=notrunc count=8
+		    dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x474)) conv=notrunc count=8
 		    MOUNTOPTS="iocharset=utf8,codepage=macroman,"
 		    ;;
 		x"vfat"*|xmsdos*)
-- 
2.27.0

[PATCH v3 4/5] tests: Ensure that mountpoints are unmounted before exiting

[Glenn Washburn]

When all tests complete successfully, filesystems mounted by grub-fs-tester
will be unmounted before exiting. However, on certain test failures the
tester will exit with a failure code and not unmount previously mounted
filesystems. Now keep track of mounts and umounts and run an exit handler
on exit or process interruption that will umount all mounts that haven't
already been unmounted.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-fs-tester.in | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index a1f3f299b..5c722209d 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -13,6 +13,16 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` ||
 # FSLABEL. This is especially needed for the conversion to Joliet UCS-2.
 XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
 
+MOUNTS=
+cleanup() {
+    for i in $MOUNTS; do
+	umount "$i" || :
+    done
+}
+trap cleanup EXIT INT
+# This is for bash, dash and ash do not recognize ERR
+trap cleanup ERR || :
+
 # This wrapper is to ease insertion of valgrind or time statistics
 run_it () {
     LC_ALL=C "$GRUBFSTEST" "$@"
@@ -934,6 +944,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 			done
 			exit 99;
 		    fi
+		    MOUNTS="$MOUNTS $MNTPOINTRW"
 		    ;;
 	    esac
 	    case x"$fs" in
@@ -1058,11 +1069,13 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		x"bfs")
 		    sleep 1
 		    fusermount -u "$MNTPOINTRW"
+		    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
 		    ;;
 		xlvm*)
 		    sleep 1
 		    for try in $(range 0 20 1); do
 			if umount "$MNTPOINTRW" ; then
+			    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
 			    break;
 			fi
 			sleep 1;
@@ -1075,6 +1088,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    sleep 1
 		    for try in $(range 0 20 1); do
 			if umount "$MNTPOINTRW" ; then
+			    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
 			    break;
 			fi
 			sleep 1;
@@ -1087,6 +1101,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    sleep 1
 		    for try in $(range 0 20 1); do
 			if umount "$MNTPOINTRW" ; then
+			    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
 			    break;
 			fi
 			sleep 1;
@@ -1116,13 +1131,19 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		xlvm*)
 		    vgchange -a y grub_test
 		    sleep 1
-		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
+		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro
+		    MOUNTS="$MOUNTS $MNTPOINTRO"
+		    ;;
 		xmdraid*)
 		    mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
 		    sleep 1
-		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
+		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro
+		    MOUNTS="$MOUNTS $MNTPOINTRO"
+		    ;;
 		*)
-		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro ;;
+		    mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o ${MOUNTOPTS}${SELINUXOPTS}ro
+		    MOUNTS="$MOUNTS $MNTPOINTRO"
+		    ;;
 	    esac
 
 	    run_grubfstest ls -- -la
@@ -1547,6 +1568,8 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
 		    sleep 1
 		    umount "$MNTPOINTRO"  || true
 		    umount "$MNTPOINTRW" || true
+		    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRO||g;")"
+		    MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
 	    esac
 	    sleep 1
 	    case x"$fs" in
-- 
2.27.0

Re: [PATCH v3 4/5] tests: Ensure that mountpoints are unmounted before exiting

[Daniel Kiper]

On Sun, Feb 06, 2022 at 04:00:11PM -0600, Glenn Washburn wrote:
> When all tests complete successfully, filesystems mounted by grub-fs-tester
> will be unmounted before exiting. However, on certain test failures the
> tester will exit with a failure code and not unmount previously mounted
> filesystems. Now keep track of mounts and umounts and run an exit handler
> on exit or process interruption that will umount all mounts that haven't
> already been unmounted.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Sorry, somehow I have missed this and next patch from the series.

For both of them Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH v3 5/5] tests: Ensure that loopback devices and zfs devices are cleaned up

[Glenn Washburn]

ZFS file systems are not unmounted using umount, but instead by exporting
them. So export the ZFS file system that has the same label as the one that
was created during the test, if such one exists. This is required to delete
the loopback device that uses the ZFS image file. Otherwise the added code
to delete all loopback devices setup during the test run will never be able
to finish because the loopback device can not be deleted while in use.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-fs-tester.in | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 5c722209d..3bc3006de 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -14,10 +14,27 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` ||
 XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
 
 MOUNTS=
+LODEVICES=
 cleanup() {
+    if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
+	zpool list "$FSLABEL" 2>/dev/null &&
+	while ! zpool export "$FSLABEL" ; do
+	    sleep 1;
+	done
+    fi
+
     for i in $MOUNTS; do
 	umount "$i" || :
     done
+
+    for lodev in $LODEVICES; do
+	local i=600
+	while losetup -l -O NAME | grep -q "^$lodev\$"; do
+	    losetup -d "$lodev" || sleep 1
+	    [ "$((i--))" = "0" ] && break
+	done
+    done
+    return 0
 }
 trap cleanup EXIT INT
 # This is for bash, dash and ash do not recognize ERR
-- 
2.27.0

Re: [PATCH v3 5/5] tests: Ensure that loopback devices and zfs devices are cleaned up

[Daniel Kiper]

On Sun, Feb 06, 2022 at 04:00:12PM -0600, Glenn Washburn wrote:
> ZFS file systems are not unmounted using umount, but instead by exporting
> them. So export the ZFS file system that has the same label as the one that
> was created during the test, if such one exists. This is required to delete
> the loopback device that uses the ZFS image file. Otherwise the added code
> to delete all loopback devices setup during the test run will never be able
> to finish because the loopback device can not be deleted while in use.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  tests/util/grub-fs-tester.in | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> index 5c722209d..3bc3006de 100644
> --- a/tests/util/grub-fs-tester.in
> +++ b/tests/util/grub-fs-tester.in
> @@ -14,10 +14,27 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` ||
>  XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
>
>  MOUNTS=
> +LODEVICES=
>  cleanup() {
> +    if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
> +	zpool list "$FSLABEL" 2>/dev/null &&
> +	while ! zpool export "$FSLABEL" ; do
> +	    sleep 1;
> +	done
> +    fi
> +
>      for i in $MOUNTS; do
>  	umount "$i" || :
>      done
> +
> +    for lodev in $LODEVICES; do
> +	local i=600

Why 600? How did you come up with this number? Do not we have better
mechanism to break this loop? If not please add a comment here.

And if we want to be perfect I would do "local i" at the beginning of
this function in the previous patch. Then we should initialize i before
this loop.

> +	while losetup -l -O NAME | grep -q "^$lodev\$"; do
> +	    losetup -d "$lodev" || sleep 1
> +	    [ "$((i--))" = "0" ] && break

This is a number not a string. Then: [ $((i--)) -eq 0 ] && break

Daniel

Re: [PATCH v3 5/5] tests: Ensure that loopback devices and zfs devices are cleaned up

[Glenn Washburn]

On Mon, 7 Feb 2022 19:36:14 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sun, Feb 06, 2022 at 04:00:12PM -0600, Glenn Washburn wrote:
> > ZFS file systems are not unmounted using umount, but instead by exporting
> > them. So export the ZFS file system that has the same label as the one that
> > was created during the test, if such one exists. This is required to delete
> > the loopback device that uses the ZFS image file. Otherwise the added code
> > to delete all loopback devices setup during the test run will never be able
> > to finish because the loopback device can not be deleted while in use.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  tests/util/grub-fs-tester.in | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> > index 5c722209d..3bc3006de 100644
> > --- a/tests/util/grub-fs-tester.in
> > +++ b/tests/util/grub-fs-tester.in
> > @@ -14,10 +14,27 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` ||
> >  XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
> >
> >  MOUNTS=
> > +LODEVICES=
> >  cleanup() {
> > +    if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
> > +	zpool list "$FSLABEL" 2>/dev/null &&
> > +	while ! zpool export "$FSLABEL" ; do
> > +	    sleep 1;
> > +	done
> > +    fi
> > +
> >      for i in $MOUNTS; do
> >  	umount "$i" || :
> >      done
> > +
> > +    for lodev in $LODEVICES; do
> > +	local i=600
> 
> Why 600? How did you come up with this number? Do not we have better
> mechanism to break this loop? If not please add a comment here.

600 is just 5 minutes since we're doing a 1 second sleep. I think
that's a pertty conservative number, none the less it is some what
arbitrary. Its meant to allow for unknown reasons why the loop back
device is still being used. Hopefully whatever is using it is trying to
stop use it. I can't remember if this is actually needed or if I put it
in just to allow some leeway. I'd rather wait an extra 5 minutes in
hopes that this will remove the loopback than have the script end with
a loopback device tht hasn't been cleaned up.

An alternative is to just loop forever, but that's not a god option for
automated CI where you'd rather have the test fail and move on than
have the CI system just kill the whole thing when its alotted time has
been exceeded.

I'd like to get a message to the user that the script finished with
some things not cleaned up, but don't see a great way of doing that. I
could write a message to /dev/tty, but there's a good change that will
be get in all the other output. And it wouldn't get saved to a log
file.

I'm opened to suggestions on any part of this. I'll await your response
before sending out an updated series.

> And if we want to be perfect I would do "local i" at the beginning of
> this function in the previous patch. Then we should initialize i before
> this loop.

Sure, I'll do that.

> > +	while losetup -l -O NAME | grep -q "^$lodev\$"; do
> > +	    losetup -d "$lodev" || sleep 1
> > +	    [ "$((i--))" = "0" ] && break
> 
> This is a number not a string. Then: [ $((i--)) -eq 0 ] && break

Sure.

Glenn

Re: [PATCH v3 0/5] Various test fixes and improvements

[Daniel Kiper]

On Sun, Feb 06, 2022 at 04:00:07PM -0600, Glenn Washburn wrote:
> v3 - Fix botched v2 udate
> v2 - Updated with Daniel's suggestions.
>
> Glenn
>
> Glenn Washburn (5):
>   tests: Do not remove image file on error in pata_test
>   tests: Skip pata_test on i386-efi
>   tests: Remove $((BASE#NUM)) bashism in grub-fs-tester

I have just taken these three patches from this series.

Daniel